[virt-tools-list] [PATCH virt-viewer v2] virt-viewer: Allow more precise VM selection
Christophe de Dinechin
cdupontd at redhat.com
Thu Mar 2 09:43:09 UTC 2017
> On 2 Mar 2017, at 10:37, Pavel Grunt <pgrunt at redhat.com> wrote:
>
> On Thu, 2017-03-02 at 10:31 +0100, Christophe de Dinechin wrote:
>>> On 2 Mar 2017, at 09:56, Pavel Grunt <pgrunt at redhat.com> 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)
Curious… I would assume it’s more a bug than a feature and add the comment anyways…
> tbh I don't understand why it was added to -Wall :)
Because -Wall means “I want to hit a wall everytime there is an update to GCC”. But don’t worry, you can make it worse by using -Wall -Werrors.
Seriously, because fall-through in switch statements is a common mistakes among beginners. Unlike a missing closing parenthesis, brace or ; it is silent and potentially harmful. I ran into a crazy bug due to this last year ;-)
>>
>>
>>> + 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 at redhat.com
>>> https://www.redhat.com/mailman/listinfo/virt-tools-list
>>
>>
>
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list at redhat.com <mailto:virt-tools-list at redhat.com>
> https://www.redhat.com/mailman/listinfo/virt-tools-list <https://www.redhat.com/mailman/listinfo/virt-tools-list>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20170302/475b4eb1/attachment.htm>
More information about the virt-tools-list
mailing list