[virt-tools-list] [PATCH 1/2] RFC: remote-viewer: learn '-' as URI for standard input connection file
Marc-André Lureau
marcandre.lureau at redhat.com
Tue Nov 14 15:25:21 UTC 2017
----- Original Message -----
> Hi,
>
> On Fri, Jul 28, 2017 at 08:43:44PM +0200, marcandre.lureau at redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau at redhat.com>
> >
> > 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 at redhat.com>
>
> Small comments below but okay with the idea
>
> > ---
> > man/remote-viewer.pod | 3 +++
> > src/remote-viewer.c | 32 +++++++++++++++++++++++++++-----
> > src/virt-viewer-file.c | 31 +++++++++++++++++++++++++------
> > src/virt-viewer-file.h | 2 ++
> > 4 files changed, 57 insertions(+), 11 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 b132214..39fe9c3 100644
> > --- a/src/remote-viewer.c
> > +++ b/src/remote-viewer.c
> > @@ -1080,6 +1080,18 @@ remote_viewer_session_connected(VirtViewerSession
> > *session,
> > g_free(uri);
> > }
> >
> > +static gchar *
> > +read_all_stdin(gsize *len)
>
> I would suggest to pass an GError ** as parameter...
>
> > +{
> > + GIOChannel *ioc = g_io_channel_unix_new(fileno(stdin));
>
> Docs says we should avoid using this function on Windows but I guess it
> should be fine with plain stdin.
>
> > + gchar *content = NULL;
> > +
> > + g_io_channel_read_to_end(ioc, &content, len, NULL);
>
> Pass the GError here and return NULL if return value is different then
> G_IO_STATUS_NORMAL
ok
> > + g_io_channel_unref(ioc);
> > +
> > + return content;
> > +}
> > +
> > static gboolean
> > remote_viewer_start(VirtViewerApp *app, GError **err)
> > {
> > @@ -1134,11 +1146,21 @@ 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);
> > + vvfile = virt_viewer_file_new_from_buffer(buf, len, &error);
> > + g_free(buf);
>
> I guess that in case of errors reading stdin, we might want to
> goto cleanup that could happen below...
>
> > + } 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 (vvfile) {
>
> Before this patch, in case of error we would goto cleanup, maybe its
> better to keep it that way.
yes
>
> if (error) {
> } else if (vvfile) {
> } else if (virt_viewer_util_extract_host(...) < 0 || type == NULL) {
> }
>
> > if (error) {
> > g_prefix_error(&error, _("Invalid file %s: "), guri);
> > g_warning("%s", error->message);
> > diff --git a/src/virt-viewer-file.c b/src/virt-viewer-file.c
> > index 9ff2a05..a511f21 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);
> > -
>
> g_return_val_if_fail (data != NULL, NULL);
added
>
> > 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_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.14.0.rc0.1.g40ca67566
>
> Reviewed-by: Victor Toso <victortoso at redhat.com>
Thanks
More information about the virt-tools-list
mailing list