[virt-tools-list] [PATCH virt-viewer 1/2] virt-viewer: Allow more precise VM selection

[Daniel P. Berrange]

On Fri, Feb 24, 2017 at 03:57:23PM +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;


> +    switch (domain_selection_type) {
> +    default: /* DOMAIN_SELECTION_NONE */
> +    case DOMAIN_SELECTION_ID:
> +        id = strtol(priv->domkey, &end, 10);
> +        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) {
> +            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;
>  }

IMHO using a switch here isn't really helping particularly with the
tricks to allow fallthrough, except when you don't want to allow
fallthrough.

I think this can be made nicer with a different approach that uses
the enum as a bitfield. eg psuedo code...

   int selection = 0;

   if (--domain is set) {
     selection |= DOMAIN_SELECTION_DOMAIN;
   } else if (--uuid is set) {
     selection |= DOMAIN_SELECTION_UUID;
   } else if (--id is set) {
     selection |= DOMAIN_SELECTION_ID;
   }

   if (!selection) {
     selection = DOMAIN_SELECTION_DOMAIN |
                 DOMAIN_SELECTION_UUID |
		 DOMAIN_SELECTION_ID;
   }


   if (selection & DOMAIN_ID) {
       dom = ...lookup by id...
       if (dom)
         selection = 0;
   }
   if (selection & DOMAIN_UUID) {
       dom = ...lookup by uuid...
       if (dom)
         selection = 0;
   }
   if (selection & DOMAIN_NAME) {
       dom = ...lookup by name...
       if (dom)
         selection = 0;
   }

This also avoids the issue with the switch potentially generating
fallthrough warnings under gcc7.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|