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 itwith 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, thefollowing commands would behave differently, which I think is a bitunexpected:$ 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.
|
