On Thu, 2017-03-02 at 10:31 +0100, Christophe de Dinechin wrote:On 2 Mar 2017, at 09:56, Pavel Grunt <pgrunt@xxxxxxxxxx> 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 complainabout "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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
_______________________________________________virt-tools-list mailing listvirt-tools-list@xxxxxxxxxxhttps://www.redhat.com/mailman/listinfo/virt-tools-list
|
