On Thu, 2017-03-02 at 14:54 +0000, Daniel P. Berrange wrote:
> On Thu, Mar 02, 2017 at 08:40:23AM -0600, Jonathon Jongsma wrote:
> > On Thu, 2017-03-02 at 10:36 +0100, Christophe de Dinechin wrote:
> > > > On 1 Mar 2017, at 22:36, Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> > > > wrote:
> > > >
> > > > On Fri, 2017-02-24 at 15:57 +0100, Pavel Grunt wrote:
> > > > > Theoretically a VM name can be a valid VM id or uuid. In that
> > > > > case
> > > > > connecting to the VMs may be problematic since virt-viewer
> > > > > selects
> > > > > the VM by its id then by uuid if not found then by its name.
> > > > >
> > > > > Introduce new command line options to cover this situation:
> > > > > "--domain-name" to connect to the VM by its name
> > > > > "--id" to connect to the VM by its id
> > > > > "--uuid" to connect to the VM by its uuid
> > > > > The options are mutually exclusive
> > > > >
> > > > > Resolves: rhbz#1399077
> > > > > ---
> > > > > man/virt-viewer.pod | 14 ++++++++
> > > > > src/virt-viewer.c | 97
> > > > > +++++++++++++++++++++++++++++++++++++++++++++++------
> > > > > 2 files changed, 101 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/man/virt-viewer.pod b/man/virt-viewer.pod
> > > > > index 9abf03c..30af8db 100644
> > > > > --- a/man/virt-viewer.pod
> > > > > +++ b/man/virt-viewer.pod
> > > > > @@ -122,6 +122,20 @@ kiosk-quit option to "on-disconnect"
> > > > > value,
> > > > > virt-viewer will quit
> > > > > instead. Please note that --reconnect takes precedence over
> > > > > this
> > > > > option, and will attempt to do a reconnection before it
> > > > > quits.
> > > > >
> > > > > +=item --id, --uuid, --domain-name
> > > > > +
> > > > > +Connect to the virtual machine by its id, uuid or name.
> > > > > These
> > > > > options
> > > > > +are mutual exclusive. For example the following command may
> > > > > sometimes
> > > > > +connect to a virtual machine with the id 2 or with the name
> > > > > 2
> > > > > (depending
> > > > > +on the number of running machines):
> > > > > +
> > > > > + virt-viewer 2
> > > > > +
> > > > > +To always connect to the virtual machine with the name "2"
> > > > > use
> > > > > the
> > > > > +"--domain-name" option:
> > > > > +
> > > > > + virt-viewer --domain-name 2
> > > > > +
> > > > > =back
> > > > >
> > > > > =head1 CONFIGURATION
> > > > > diff --git a/src/virt-viewer.c b/src/virt-viewer.c
> > > > > index 1f99552..6bc9b6d 100644
> > > > > --- a/src/virt-viewer.c
> > > > > +++ b/src/virt-viewer.c
> > > > > @@ -82,6 +82,46 @@ static gboolean opt_attach = FALSE;
> > > > > static gboolean opt_waitvm = FALSE;
> > > > > static gboolean opt_reconnect = FALSE;
> > > > >
> > > > > +typedef enum {
> > > > > + DOMAIN_SELECTION_NONE,
> > > > > + DOMAIN_SELECTION_NAME,
> > > > > + DOMAIN_SELECTION_ID,
> > > > > + DOMAIN_SELECTION_UUID,
> > > > > +} DomainSelection;
> > > > > +
> > > > > +static const gchar* domain_selection_to_opt[] = {
> > > > > + [DOMAIN_SELECTION_NONE] = NULL,
> > > > > + [DOMAIN_SELECTION_NAME] = "--domain-name",
> > > > > + [DOMAIN_SELECTION_ID] = "--id",
> > > > > + [DOMAIN_SELECTION_UUID] = "--uuid",
> > > > > +};
> > > > > +
> > > > > +static DomainSelection domain_selection_type =
> > > > > DOMAIN_SELECTION_NONE;
> > > > > +
> > > > > +static gboolean
> > > > > +opt_domain_selection_cb(const gchar *option_name,
> > > > > + const gchar *value G_GNUC_UNUSED,
> > > > > + gpointer data G_GNUC_UNUSED,
> > > > > + GError **error)
> > > > > +{
> > > > > + guint i;
> > > > > + if (domain_selection_type != DOMAIN_SELECTION_NONE) {
> > > > > + g_set_error(error, G_OPTION_ERROR,
> > > > > G_OPTION_ERROR_FAILED,
> > > > > + "selection type has been already set");
> > > > > + return FALSE;
> > > > > + }
> > > > > +
> > > > > + for (i = DOMAIN_SELECTION_NAME; i <=
> > > > > G_N_ELEMENTS(domain_selection_to_opt); i++) {
> > > > > + if (g_strcmp0(option_name,
> > > > > domain_selection_to_opt[i])
> > > > > == 0)
> > > > > {
> > > > > + domain_selection_type = i;
> > > > > + return TRUE;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + g_assert_not_reached();
> > > > > + return FALSE;
> > > > > +}
> > > > > +
> > > > > static void
> > > > > virt_viewer_add_option_entries(VirtViewerApp *self,
> > > > > GOptionContext
> > > > > *context, GOptionGroup *group)
> > > > > {
> > > > > @@ -96,6 +136,12 @@
> > > > > virt_viewer_add_option_entries(VirtViewerApp
> > > > > *self, GOptionContext *context, GOp
> > > > > N_("Wait for domain to start"), NULL },
> > > > > { "reconnect", 'r', 0, G_OPTION_ARG_NONE,
> > > > > &opt_reconnect,
> > > > > N_("Reconnect to domain upon restart"), NULL },
> > > > > + { "domain-name", '\0', G_OPTION_FLAG_NO_ARG,
> > > > > G_OPTION_ARG_CALLBACK, opt_domain_selection_cb,
> > > > > + N_("Select the virtual machine only by its name"),
> > > > > NULL },
> > > > > + { "id", '\0', G_OPTION_FLAG_NO_ARG,
> > > > > G_OPTION_ARG_CALLBACK,
> > > > > opt_domain_selection_cb,
> > > > > + N_("Select the virtual machine only by its id"),
> > > > > NULL
> > > > > },
> > > > > + { "uuid", '\0', G_OPTION_FLAG_NO_ARG,
> > > > > G_OPTION_ARG_CALLBACK,
> > > > > opt_domain_selection_cb,
> > > > > + N_("Select the virtual machine only by its uuid"),
> > > > > NULL },
> > > > > { G_OPTION_REMAINING, '\0', 0,
> > > > > G_OPTION_ARG_STRING_ARRAY,
> > > > > &opt_args,
> > > > > NULL, "-- DOMAIN-NAME|ID|UUID" },
> > > > > { NULL, 0, 0, G_OPTION_ARG_NONE, NULL, NULL, NULL }
> > > > > @@ -114,6 +160,7 @@ virt_viewer_local_command_line
> > > > > (GApplication *gapp,
> > > > > gboolean ret = FALSE;
> > > > > VirtViewer *self = VIRT_VIEWER(gapp);
> > > > > VirtViewerApp *app = VIRT_VIEWER_APP(gapp);
> > > > > + const gchar *missing_domkey_fmt = _("\nNo DOMAIN-
> > > > > NAME|ID|UUID
> > > > > was specified for '%s'\n\n");
> > > > >
> > > > > ret = G_APPLICATION_CLASS(virt_viewer_parent_class)-
> > > > > > local_command_line(gapp, args, status);
> > > > >
> > > > > if (ret)
> > > > > @@ -133,7 +180,7 @@ virt_viewer_local_command_line
> > > > > (GApplication *gapp,
> > > > >
> > > > > if (opt_waitvm) {
> > > > > if (!self->priv->domkey) {
> > > > > - g_printerr(_("\nNo DOMAIN-NAME|ID|UUID was
> > > > > specified
> > > > > for
> > > > > '--wait'\n\n"));
> > > > > + g_printerr(missing_domkey_fmt, "--wait");
> > > > > ret = TRUE;
> > > > > *status = 1;
> > > > > goto end;
> > > > > @@ -142,6 +189,15 @@ virt_viewer_local_command_line
> > > > > (GApplication *gapp,
> > > > > self->priv->waitvm = TRUE;
> > > > > }
> > > > >
> > > > > + if (domain_selection_type != DOMAIN_SELECTION_NONE) {
> > > > > + if (!self->priv->domkey) {
> > > > > + g_printerr(missing_domkey_fmt,
> > > > > domain_selection_to_opt[domain_selection_type]);
> > > > > + ret = TRUE;
> > > > > + *status = 1;
> > > > > + goto end;
> > > > > + }
> > > > > + }
> > > > > +
> > > >
> > > > This feels a bit like excessive duplication. Could we maybe
> > > > combine
> > > > it
> > > > with the previous block? Something like (just brainstorming):
> > > >
> > > > if (opt_waitvm || domain_selection_type !=
> > > > DOMAIN_SELECTION_NONE) {
> > > > if (!domkey) {
> > > > // report error
> > > > goto end;
> > > > }
> > > > }
> > > >
> > > > self->priv->waitvm = opt_waitvm;
> > > >
> > > >
> > > > > virt_viewer_app_set_direct(app, opt_direct);
> > > > > virt_viewer_app_set_attach(app, opt_attach);
> > > > > self->priv->reconnect = opt_reconnect;
> > > > > @@ -311,16 +367,31 @@ virt_viewer_lookup_domain(VirtViewer
> > > > > *self)
> > > > > return NULL;
> > > > > }
> > > > >
> > > > > - id = strtol(priv->domkey, &end, 10);
> > > > > - if (id >= 0 && end && !*end) {
> > > > > - dom = virDomainLookupByID(priv->conn, id);
> > > > > - }
> > > > > - if (!dom && virt_viewer_parse_uuid(priv->domkey, uuid)
> > > > > == 0)
> > > > > {
> > > > > - dom = virDomainLookupByUUID(priv->conn, uuid);
> > > > > - }
> > > > > - if (!dom) {
> > > > > - dom = virDomainLookupByName(priv->conn, priv-
> > > > > >domkey);
> > > > > + switch (domain_selection_type) {
> > > > > + default: /* DOMAIN_SELECTION_NONE */
> > > > > + case DOMAIN_SELECTION_ID:
> > > > > + id = strtol(priv->domkey, &end, 10);
> > > >
> > > > 'id' variable is now technically local to this scope
> > > >
> > > > > + if (id >= 0 && end && !*end) {
> > > > > + dom = virDomainLookupByID(priv->conn, id);
> > > > > + }
> > > > > + if (domain_selection_type != DOMAIN_SELECTION_NONE)
> > > > > {
> > > > > + break;
> > > > > + }
> > > > > + /* fallthrough */
> > > > > + case DOMAIN_SELECTION_UUID:
> > > > > + if (!dom && virt_viewer_parse_uuid(priv->domkey,
> > > > > uuid)
> > > > > == 0)
> > > >
> > > > Same with uuid.
> > > >
> > > > > {
> > > > > + dom = virDomainLookupByUUID(priv->conn, uuid);
> > > > > + }
> > > > > + if (domain_selection_type != DOMAIN_SELECTION_NONE)
> > > > > {
> > > > > + break;
> > > > > + }
> > > > > + /* fallthrough */
> > > > > + case DOMAIN_SELECTION_NAME:
> > > > > + if (!dom) {
> > > > > + dom = virDomainLookupByName(priv->conn, priv-
> > > > > > domkey);
> > > > >
> > > > > + }
> > > > > }
> > > > > +
> > > > > return dom;
> > > > > }
> > > > >
> > > > > @@ -816,6 +887,12 @@
> > > > > virt_viewer_initial_connect(VirtViewerApp
> > > > > *app,
> > > > > GError **error)
> > > > > if (priv->waitvm) {
> > > > > virt_viewer_app_show_status(app, _("Waiting for
> > > > > guest
> > > > > domain to be created"));
> > > > > goto wait;
> > > > > + } else if (domain_selection_type !=
> > > > > DOMAIN_SELECTION_NONE) {
> > > > > + g_set_error(&err, VIRT_VIEWER_ERROR,
> > > > > VIRT_VIEWER_ERROR_FAILED,
> > > > > + _("Couldn't find domain '%s'
> > > > > specified
> > > > > by
> > > > > '%s'"),
> > > > > + priv->domkey,
> > > > > + domain_selection_to_opt[domain_selec
> > > > > tion
> > > > > _typ
> > > > > e]);
> > > > > + goto cleanup;
> > > >
> > > > I'm not sure that this section is necessary. After this change,
> > > > the
> > > > following commands would behave differently, which I think is a
> > > > bit
> > > > unexpected:
> > > >
> > > > $ virt-viewer nonexistent # opens dialog to choose a vm
> > > > $ virt-viewer --domain-name nonexistent # exits with error
> > >
> > > Actually, this may be a nice feature for scripting. If you do
> > > things
> > > manually, you’ll probably use option 1. If you script something,
> > > you
> > > may use option 2, and then you probably prefer to get an exit
> > > code
> > > than a dialog box.
> >
> > Yes, possibly. But neither one is really ideal for scripting at the
> > moment because the second scenario shows an error dialog which
> > needs to
> > be manually closed. And I still think that it would be least
> > surprising
> > if they both behaved the same (and maybe that means they should
> > *both*
> > exit with an error and only display the dialog if no domain is
> > specified on the command line?).
>
> If we want to surpress interactive prompting of the user when they
> give a bogus URI or domain identifier, we should have explicit CLI
> options todo that, not try to overload this behaviour onto another
> option
>
I agree. But just to be clear, when I said "only display the dialog if
no domain is specified", I was talking only about the "select a vm"
dialog. I was not suggesting to remove the error dialog. I think adding
an extra option which disables interactive error reporting and only
reports errors on stderr would be a fairly nice solution to the
scripting problem. But obviously, that would be done in a separate
patch and would not affect this one.
Jonathon
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
