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

[Pavel Grunt]

On Thu, 2017-03-02 at 09:44 +0000, Daniel P. Berrange wrote:
> 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;
>    }
> 
thanks, it looks much more clean

Pavel


> This also avoids the issue with the switch potentially generating
> fallthrough warnings under gcc7.
> 
> Regards,
> Daniel