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
_______________________________________________
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
