On Thu, 2017-03-02 at 10:31 +0100, Christophe de Dinechin wrote:
> > On 2 Mar 2017, at 09:56, Pavel Grunt <pgrunt@xxxxxxxxxx> 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
> > ---
> > v2: changes per Jonathon's review
> > - use common code for checking domkey (--wait and the new options)
> > - move variables to local scope
> > - do not fail on `virt-viewer --domain-name nonexistent`
> > ---
> > man/virt-viewer.pod | 14 +++++++++
> > src/virt-viewer.c | 90
> > ++++++++++++++++++++++++++++++++++++++++++++++-------
> > 2 files changed, 92 insertions(+), 12 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..b577f0a 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”);
>
> I would either make an array indexed by the domain_selection_type
> with three messages, or write “No argument was specified for %s”. As
> is, I find it a bit unreadable. I know this is how it was spelled
> before, but still ;-)
>
> >
> > ret = G_APPLICATION_CLASS(virt_viewer_parent_class)-
> > >local_command_line(gapp, args, status);
> > if (ret)
> > @@ -131,15 +178,16 @@ virt_viewer_local_command_line
> > (GApplication *gapp,
> > }
> >
> >
> > - if (opt_waitvm) {
> > + if (opt_waitvm || domain_selection_type !=
> > DOMAIN_SELECTION_NONE) {
> > if (!self->priv->domkey) {
> > - g_printerr(_("\nNo DOMAIN-NAME|ID|UUID was specified
> > for '--wait'\n\n"));
> > + g_printerr(missing_domkey_fmt,
> > + opt_waitvm ? "--wait" :
> > domain_selection_to_opt[domain_selection_type]);
> > ret = TRUE;
> > *status = 1;
> > goto end;
> > }
> >
> > - self->priv->waitvm = TRUE;
> > + self->priv->waitvm = opt_waitvm;
> > }
> >
> > virt_viewer_app_set_direct(app, opt_direct);
> > @@ -303,24 +351,42 @@ virt_viewer_lookup_domain(VirtViewer *self)
> > {
> > char *end;
> > VirtViewerPrivate *priv = self->priv;
> > - int id;
> > virDomainPtr dom = NULL;
> > - unsigned char uuid[16];
> >
> > if (priv->domkey == NULL) {
> > return NULL;
> > }
> >
> > - id = strtol(priv->domkey, &end, 10);
> > - if (id >= 0 && end && !*end) {
> > - dom = virDomainLookupByID(priv->conn, id);
> > + switch (domain_selection_type) {
> > + default: /* DOMAIN_SELECTION_NONE */
>
> I would add “fallthrough” to the comment as for the next case. GCC
> actually takes this as a hint with (see https://gcc.gnu.org/onlinedo
> cs/gcc/Warning-Options.html, search for -Wimplicit-fallthrough). We
> could also use
>
> __attribute__ ((fallthrough));
>
> I don’t know if we every build with -Wextra or -Wall?
-Wall, I added “fallthrough” where gcc complained (it didn't complain
about "default" case) tbh I don't understand why it was added to -Wall
:)
>
>
> > + case DOMAIN_SELECTION_ID:
> > + {
> > + long int id = strtol(priv->domkey, &end, 10);
> > + if (id >= 0 && end && !*end) {
> > + dom = virDomainLookupByID(priv->conn, id);
> > + }
> > + if (domain_selection_type != DOMAIN_SELECTION_NONE) {
> > + break;
> > + }
> > }
> > - if (!dom && virt_viewer_parse_uuid(priv->domkey, uuid) == 0)
> > {
> > - dom = virDomainLookupByUUID(priv->conn, uuid);
> > + /* fallthrough */
> > + case DOMAIN_SELECTION_UUID:
> > + {
> > + unsigned char uuid[16];
> > + if (!dom && virt_viewer_parse_uuid(priv->domkey, uuid) ==
> > 0) {
> > + dom = virDomainLookupByUUID(priv->conn, uuid);
> > + }
> > + if (domain_selection_type != DOMAIN_SELECTION_NONE) {
> > + break;
> > + }
> > }
> > - if (!dom) {
> > - dom = virDomainLookupByName(priv->conn, priv->domkey);
> > + /* fallthrough */
> > + case DOMAIN_SELECTION_NAME:
> > + if (!dom) {
> > + dom = virDomainLookupByName(priv->conn, priv-
> > >domkey);
> > + }
> > }
> > +
> > return dom;
> > }
> >
> > --
> > 2.12.0
> >
> > _______________________________________________
> > virt-tools-list mailing list
> > virt-tools-list@xxxxxxxxxx
> > https://www.redhat.com/mailman/listinfo/virt-tools-list
>
>
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
