On Tue, Nov 14, 2017 at 5:46 PM, Victor Toso <victortoso@xxxxxxxxxx> wrote:
> Hi,
>
> On Tue, Nov 14, 2017 at 11:39:54AM -0500, Marc-André Lureau wrote:
>> Hi
>>
>> ----- Original Message -----
>> > Hi,
>> >
>> > On Tue, Nov 14, 2017 at 04:56:01PM +0100, marcandre.lureau@xxxxxxxxxx wrote:
>> > > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
>> > >
>> > > Some users want the ability to set connection details without writing
>> > > to a file or passing command line arguments.
>> > >
>> > > Learn to read the connection file from standard input for such use
>> > > case.
>> > >
>> > > This allows a parent process to set the connection details without
>> > > intermediary file.
>> > >
>> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
>> > > Reviewed-by: Victor Toso <victortoso@xxxxxxxxxx>
>> > > ---
>> > > man/remote-viewer.pod | 3 +++
>> > > src/remote-viewer.c | 46 ++++++++++++++++++++++++++++++++++++++++------
>> > > src/virt-viewer-file.c | 31 +++++++++++++++++++++++++------
>> > > src/virt-viewer-file.h | 2 ++
>> > > 4 files changed, 70 insertions(+), 12 deletions(-)
>> > >
>> > > diff --git a/man/remote-viewer.pod b/man/remote-viewer.pod
>> > > index 60ea3a6..8428c80 100644
>> > > --- a/man/remote-viewer.pod
>> > > +++ b/man/remote-viewer.pod
>> > > @@ -18,6 +18,9 @@ entry and a list of previously successfully accessed URI.
>> > > The URI can also point to a connection settings file, see the CONNECTION
>> > > FILE
>> > > section for a description of the format.
>> > >
>> > > +If URI is '-', then remote-viewer will read the standard input as a
>> > > +connection settings file and attempt to connect using it.
>> > > +
>> > > =head1 OPTIONS
>> > >
>> > > The following options are accepted when running C<remote-viewer>:
>> > > diff --git a/src/remote-viewer.c b/src/remote-viewer.c
>> > > index fb5376c..e082178 100644
>> > > --- a/src/remote-viewer.c
>> > > +++ b/src/remote-viewer.c
>> > > @@ -1084,6 +1084,22 @@ remote_viewer_session_connected(VirtViewerSession
>> > > *session,
>> > > g_free(uri);
>> > > }
>> > >
>> > > +static gchar *
>> > > +read_all_stdin(gsize *len, GError **err)
>> > > +{
>> > > + GIOChannel *ioc = g_io_channel_unix_new(fileno(stdin));
>> > > + gchar *content = NULL;
>> > > + GIOStatus status;
>> > > +
>> > > + status = g_io_channel_read_to_end(ioc, &content, len, err);
>> > > + g_assert(status != G_IO_STATUS_AGAIN && status != G_IO_STATUS_EOF);
>> >
>> > "G_IO_STATUS_NORMAL on success. This function never returns G_IO_STATUS_EOF"
>>
>> Yes, I also want to assert we don't get AGAIN (or should we loop?) I
>> don't think it's necessary, stdin should be blocking and this would be
>> a weird case, wouldn't it?
>
> It would. I just don't see the point on checking with G_IO_STATUS_EOF if
> documentation says it should never return it. IMHO, checking
> G_IO_STATUS_AGAIN should be fine.
ok
>
>> > > +
>> > > + g_io_channel_unref(ioc);
>> > > + g_assert((content && !*err) || (!content && *err));
>> >
>> > This seems that you are sanity checking g_io_channel_read_to_end() ?
>>
>> I want to make sure that either we get an error or we get content, not
>> both. It's not clear for the API (for ex with AGAIN).
>
> I agree
>
>> >
>> > If you want to assert here, I guess just use g_assert_no_error()
>> > instead ?
>>
>> That was basically the approach before, let the function return NULL,
>> and fail later when reading from NULL buffer with keyfile.
>>
>> Now if we want to produce a error message instead, we should propagate
>> GError.
>>
>> Other solution is to pring the GError immediately if any, and return
>> NULL in that case. Anything works for me.
>
> Feel free to keep it as is.
>
thanks
>> > > +
>> > > + return content;
>> > > +}
>> > > +
>> > > static gboolean
>> > > remote_viewer_initial_connect(RemoteViewer *self, const gchar *type,
>> > > VirtViewerFile *vvfile, GError **error)
>> > > @@ -1174,16 +1190,34 @@ retry_dialog:
>> > >
>> > > g_debug("Opening display to %s", guri);
>> > >
>> > > - file = g_file_new_for_commandline_arg(guri);
>> > > - if (g_file_query_exists(file, NULL)) {
>> > > - gchar *path = g_file_get_path(file);
>> > > - vvfile = virt_viewer_file_new(path, &error);
>> > > - g_free(path);
>> > > + if (!g_strcmp0(guri, "-")) {
>> > > + gsize len = 0;
>> > > + gchar *buf = read_all_stdin(&len, &error);
>> > > +
>> > > if (error) {
>> > > - g_prefix_error(&error, _("Invalid file %s: "), guri);
>> > > + g_prefix_error(&error, _("Failed to read stdin: "));
>> > > g_warning("%s", error->message);
>> > > goto cleanup;
>> > > }
>> > > +
>> > > + vvfile = virt_viewer_file_new_from_buffer(buf, len, &error);
>> > > + g_free(buf);
>> > > + } else {
>> > > + file = g_file_new_for_commandline_arg(guri);
>> > > + if (g_file_query_exists(file, NULL)) {
>> > > + gchar *path = g_file_get_path(file);
>> > > + vvfile = virt_viewer_file_new(path, &error);
>> > > + g_free(path);
>> > > + }
>> > > + }
>> > > +
>> > > + if (error) {
>> > > + g_prefix_error(&error, _("Invalid file %s: "), guri);
>> > > + g_warning("%s", error->message);
>> > > + goto cleanup;
>> > > + }
>> > > +
>> > > + if (vvfile) {
>> > > g_object_get(G_OBJECT(vvfile), "type", &type, NULL);
>> > > } else if (virt_viewer_util_extract_host(guri, &type, NULL, NULL,
>> > > NULL, NULL) < 0 || type == NULL) {
>> > > g_set_error_literal(&error,
>> > > diff --git a/src/virt-viewer-file.c b/src/virt-viewer-file.c
>> > > index 9ff2a05..1b0c310 100644
>> > > --- a/src/virt-viewer-file.c
>> > > +++ b/src/virt-viewer-file.c
>> > > @@ -139,16 +139,15 @@ enum {
>> > > };
>> > >
>> > > VirtViewerFile*
>> > > -virt_viewer_file_new(const gchar* location, GError** error)
>> > > +virt_viewer_file_new_from_buffer(const gchar* data, gsize len,
>> > > + GError** error)
>> > > {
>> > > GError* inner_error = NULL;
>> > > -
>> > > - g_return_val_if_fail (location != NULL, NULL);
>> > > -
>> > > VirtViewerFile* self =
>> > > VIRT_VIEWER_FILE(g_object_new(VIRT_VIEWER_TYPE_FILE, NULL));
>> > > GKeyFile* keyfile = self->priv->keyfile;
>> > >
>> > > - g_key_file_load_from_file(keyfile, location,
>> > > + g_return_val_if_fail(data != NULL, NULL);
>> > > + g_key_file_load_from_data(keyfile, data, len,
>> > > G_KEY_FILE_KEEP_COMMENTS |
>> > > G_KEY_FILE_KEEP_TRANSLATIONS,
>> > > &inner_error);
>> > > if (inner_error != NULL) {
>> > > @@ -166,7 +165,27 @@ virt_viewer_file_new(const gchar* location, GError**
>> > > error)
>> > > return NULL;
>> > > }
>> > >
>> > > - if (virt_viewer_file_get_delete_this_file(self) &&
>> > > + return self;
>> > > +}
>> > > +
>> > > +VirtViewerFile*
>> > > +virt_viewer_file_new(const gchar* location, GError** error)
>> > > +{
>> > > + VirtViewerFile *self;
>> > > + gchar *buf;
>> > > + gsize len;
>> > > +
>> > > + g_return_val_if_fail (location != NULL, NULL);
>> > > +
>> > > + if (!g_file_get_contents(location, &buf, &len, error)) {
>> > > + return NULL;
>> > > + }
>> > > +
>> > > + self = virt_viewer_file_new_from_buffer(buf, len, error);
>> > > + g_free(buf);
>> > > +
>> > > +
>> > > + if (self && virt_viewer_file_get_delete_this_file(self) &&
>> > > !g_getenv("VIRT_VIEWER_KEEP_FILE")) {
>> > > if (g_unlink(location) != 0)
>> > > g_warning("failed to remove %s", location);
>> > > diff --git a/src/virt-viewer-file.h b/src/virt-viewer-file.h
>> > > index 6b783f9..15c61d0 100644
>> > > --- a/src/virt-viewer-file.h
>> > > +++ b/src/virt-viewer-file.h
>> > > @@ -50,6 +50,8 @@ struct _VirtViewerFileClass
>> > > GType virt_viewer_file_get_type(void);
>> > >
>> > > VirtViewerFile* virt_viewer_file_new(const gchar* path, GError** error);
>> > > +VirtViewerFile* virt_viewer_file_new_from_buffer(const gchar* buf, gsize
>> > > len,
>> > > + GError** error);
>> > > gboolean virt_viewer_file_is_set(VirtViewerFile* self, const gchar* key);
>> > >
>> > > gchar* virt_viewer_file_get_ca(VirtViewerFile* self);
>> > > --
>> > > 2.15.0.125.g8f49766d64
>> > >
>> >
>
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/virt-tools-list
--
Marc-André Lureau
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
