[virt-tools-list] [PATCH virt-viewer 1/2] virt-viewer: Allow more precise VM selection
Christophe de Dinechin
cdupontd at redhat.com
Thu Mar 2 09:36:13 UTC 2017
> On 1 Mar 2017, at 22:36, Jonathon Jongsma <jjongsma at redhat.com> wrote:
>
> On Fri, 2017-02-24 at 15:57 +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;
>> +
>> +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");
>>
>> ret = G_APPLICATION_CLASS(virt_viewer_parent_class)-
>>> local_command_line(gapp, args, status);
>> if (ret)
>> @@ -133,7 +180,7 @@ virt_viewer_local_command_line
>> (GApplication *gapp,
>>
>> if (opt_waitvm) {
>> if (!self->priv->domkey) {
>> - g_printerr(_("\nNo DOMAIN-NAME|ID|UUID was specified for
>> '--wait'\n\n"));
>> + g_printerr(missing_domkey_fmt, "--wait");
>> ret = TRUE;
>> *status = 1;
>> goto end;
>> @@ -142,6 +189,15 @@ virt_viewer_local_command_line
>> (GApplication *gapp,
>> self->priv->waitvm = TRUE;
>> }
>>
>> + if (domain_selection_type != DOMAIN_SELECTION_NONE) {
>> + if (!self->priv->domkey) {
>> + g_printerr(missing_domkey_fmt,
>> domain_selection_to_opt[domain_selection_type]);
>> + ret = TRUE;
>> + *status = 1;
>> + goto end;
>> + }
>> + }
>> +
>
>
> This feels a bit like excessive duplication. Could we maybe combine it
> with the previous block? Something like (just brainstorming):
>
> if (opt_waitvm || domain_selection_type != DOMAIN_SELECTION_NONE) {
> if (!domkey) {
> // report error
> goto end;
> }
> }
>
> self->priv->waitvm = opt_waitvm;
>
>
>> virt_viewer_app_set_direct(app, opt_direct);
>> virt_viewer_app_set_attach(app, opt_attach);
>> self->priv->reconnect = opt_reconnect;
>> @@ -311,16 +367,31 @@ virt_viewer_lookup_domain(VirtViewer *self)
>> return NULL;
>> }
>>
>> - id = strtol(priv->domkey, &end, 10);
>> - if (id >= 0 && end && !*end) {
>> - dom = virDomainLookupByID(priv->conn, id);
>> - }
>> - if (!dom && virt_viewer_parse_uuid(priv->domkey, uuid) == 0) {
>> - dom = virDomainLookupByUUID(priv->conn, uuid);
>> - }
>> - if (!dom) {
>> - dom = virDomainLookupByName(priv->conn, priv->domkey);
>> + switch (domain_selection_type) {
>> + default: /* DOMAIN_SELECTION_NONE */
>> + case DOMAIN_SELECTION_ID:
>> + id = strtol(priv->domkey, &end, 10);
>
> 'id' variable is now technically local to this scope
>
>> + 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)
>
> Same with uuid.
>
>> {
>> + 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;
>> }
>>
>> @@ -816,6 +887,12 @@ virt_viewer_initial_connect(VirtViewerApp *app,
>> GError **error)
>> if (priv->waitvm) {
>> virt_viewer_app_show_status(app, _("Waiting for guest
>> domain to be created"));
>> goto wait;
>> + } else if (domain_selection_type != DOMAIN_SELECTION_NONE) {
>> + g_set_error(&err, VIRT_VIEWER_ERROR,
>> VIRT_VIEWER_ERROR_FAILED,
>> + _("Couldn't find domain '%s' specified by
>> '%s'"),
>> + priv->domkey,
>> + domain_selection_to_opt[domain_selection_typ
>> e]);
>> + goto cleanup;
>
> I'm not sure that this section is necessary. After this change, the
> following commands would behave differently, which I think is a bit
> unexpected:
>
> $ virt-viewer nonexistent # opens dialog to choose a vm
> $ virt-viewer --domain-name nonexistent # exits with error
Actually, this may be a nice feature for scripting. If you do things manually, you’ll probably use option 1. If you script something, you may use option 2, and then you probably prefer to get an exit code than a dialog box.
>
>
>> } else {
>> VirtViewerWindow *main_window =
>> virt_viewer_app_get_main_window(app);
>> if (priv->domkey != NULL)
>
>
> Jonathon
>
> _______________________________________________
> 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/e999f406/attachment.htm>
More information about the virt-tools-list
mailing list