<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On 2 Mar 2017, at 10:37, Pavel Grunt <<a href="mailto:pgrunt@redhat.com" class="">pgrunt@redhat.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">On Thu, 2017-03-02 at 10:31 +0100, Christophe de Dinechin wrote:</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" class="">On 2 Mar 2017, at 09:56, Pavel Grunt <<a href="mailto:pgrunt@redhat.com" class="">pgrunt@redhat.com</a>> wrote:<br class=""><br class="">Theoretically a VM name can be a valid VM id or uuid. In that case<br class="">connecting to the VMs may be problematic since virt-viewer selects<br class="">the VM by its id then by uuid if not found then by its name.<br class=""><br class="">Introduce new command line options to cover this situation:<br class="">"--domain-name" to connect to the VM by its name<br class="">"--id" to connect to the VM by its id<br class="">"--uuid" to connect to the VM by its uuid<br class="">The options are mutually exclusive<br class=""><br class="">Resolves: rhbz#1399077<br class="">---<br class="">v2: changes per Jonathon's review<br class="">- use common code for checking domkey (--wait and the new options)<br class="">- move variables to local scope<br class="">- do not fail on `virt-viewer --domain-name nonexistent`<br class="">---<br class="">man/virt-viewer.pod | 14 +++++++++<br class="">src/virt-viewer.c   | 90<br class="">++++++++++++++++++++++++++++++++++++++++++++++-------<br class="">2 files changed, 92 insertions(+), 12 deletions(-)<br class=""><br class="">diff --git a/man/virt-viewer.pod b/man/virt-viewer.pod<br class="">index 9abf03c..30af8db 100644<br class="">--- a/man/virt-viewer.pod<br class="">+++ b/man/virt-viewer.pod<br class="">@@ -122,6 +122,20 @@ kiosk-quit option to "on-disconnect" value,<br class="">virt-viewer will quit<br class="">instead. Please note that --reconnect takes precedence over this<br class="">option, and will attempt to do a reconnection before it quits.<br class=""><br class="">+=item --id, --uuid, --domain-name<br class="">+<br class="">+Connect to the virtual machine by its id, uuid or name. These<br class="">options<br class="">+are mutual exclusive. For example the following command may<br class="">sometimes<br class="">+connect to a virtual machine with the id 2 or with the name 2<br class="">(depending<br class="">+on the number of running machines):<br class="">+<br class="">+    virt-viewer 2<br class="">+<br class="">+To always connect to the virtual machine with the name "2" use<br class="">the<br class="">+"--domain-name" option:<br class="">+<br class="">+    virt-viewer --domain-name 2<br class="">+<br class="">=back<br class=""><br class="">=head1 CONFIGURATION<br class="">diff --git a/src/virt-viewer.c b/src/virt-viewer.c<br class="">index 1f99552..b577f0a 100644<br class="">--- a/src/virt-viewer.c<br class="">+++ b/src/virt-viewer.c<br class="">@@ -82,6 +82,46 @@ static gboolean opt_attach = FALSE;<br class="">static gboolean opt_waitvm = FALSE;<br class="">static gboolean opt_reconnect = FALSE;<br class=""><br class="">+typedef enum {<br class="">+    DOMAIN_SELECTION_NONE,<br class="">+    DOMAIN_SELECTION_NAME,<br class="">+    DOMAIN_SELECTION_ID,<br class="">+    DOMAIN_SELECTION_UUID,<br class="">+} DomainSelection;<br class="">+<br class="">+static const gchar* domain_selection_to_opt[] = {<br class="">+    [DOMAIN_SELECTION_NONE] = NULL,<br class="">+    [DOMAIN_SELECTION_NAME] = "--domain-name",<br class="">+    [DOMAIN_SELECTION_ID] = "--id",<br class="">+    [DOMAIN_SELECTION_UUID] = "--uuid",<br class="">+};<br class="">+<br class="">+static DomainSelection domain_selection_type =<br class="">DOMAIN_SELECTION_NONE;<br class="">+<br class="">+static gboolean<br class="">+opt_domain_selection_cb(const gchar *option_name,<br class="">+                        const gchar *value G_GNUC_UNUSED,<br class="">+                        gpointer data G_GNUC_UNUSED,<br class="">+                        GError **error)<br class="">+{<br class="">+    guint i;<br class="">+    if (domain_selection_type != DOMAIN_SELECTION_NONE) {<br class="">+        g_set_error(error, G_OPTION_ERROR, G_OPTION_ERROR_FAILED,<br class="">+                    "selection type has been already set");<br class="">+        return FALSE;<br class="">+    }<br class="">+<br class="">+    for (i = DOMAIN_SELECTION_NAME; i <=<br class="">G_N_ELEMENTS(domain_selection_to_opt); i++) {<br class="">+        if (g_strcmp0(option_name, domain_selection_to_opt[i]) ==<br class="">0) {<br class="">+            domain_selection_type = i;<br class="">+            return TRUE;<br class="">+        }<br class="">+    }<br class="">+<br class="">+    g_assert_not_reached();<br class="">+    return FALSE;<br class="">+}<br class="">+<br class="">static void<br class="">virt_viewer_add_option_entries(VirtViewerApp *self, GOptionContext<br class="">*context, GOptionGroup *group)<br class="">{<br class="">@@ -96,6 +136,12 @@ virt_viewer_add_option_entries(VirtViewerApp<br class="">*self, GOptionContext *context, GOp<br class="">         N_("Wait for domain to start"), NULL },<br class="">       { "reconnect", 'r', 0, G_OPTION_ARG_NONE, &opt_reconnect,<br class="">         N_("Reconnect to domain upon restart"), NULL },<br class="">+        { "domain-name", '\0', G_OPTION_FLAG_NO_ARG,<br class="">G_OPTION_ARG_CALLBACK, opt_domain_selection_cb,<br class="">+          N_("Select the virtual machine only by its name"), NULL<br class="">},<br class="">+        { "id", '\0', G_OPTION_FLAG_NO_ARG,<br class="">G_OPTION_ARG_CALLBACK, opt_domain_selection_cb,<br class="">+          N_("Select the virtual machine only by its id"), NULL<br class="">},<br class="">+        { "uuid", '\0', G_OPTION_FLAG_NO_ARG,<br class="">G_OPTION_ARG_CALLBACK, opt_domain_selection_cb,<br class="">+          N_("Select the virtual machine only by its uuid"), NULL<br class="">},<br class="">       { G_OPTION_REMAINING, '\0', 0, G_OPTION_ARG_STRING_ARRAY,<br class="">&opt_args,<br class="">         NULL, "-- DOMAIN-NAME|ID|UUID" },<br class="">       { NULL, 0, 0, G_OPTION_ARG_NONE, NULL, NULL, NULL }<br class="">@@ -114,6 +160,7 @@ virt_viewer_local_command_line<br class="">(GApplication   *gapp,<br class="">   gboolean ret = FALSE;<br class="">   VirtViewer *self = VIRT_VIEWER(gapp);<br class="">   VirtViewerApp *app = VIRT_VIEWER_APP(gapp);<br class="">+    const gchar *missing_domkey_fmt = _("\nNo DOMAIN-NAME|ID|UUID<span class="Apple-converted-space"> </span><br class="">was specified for '%s'\n\n”);<br class=""></blockquote><br class="">I would either make an array indexed by the domain_selection_type<br class="">with three messages, or write “No argument was specified for %s”. As<br class="">is, I find it a bit unreadable. I know this is how it was spelled<br class="">before, but still ;-)<br class=""><br class=""><blockquote type="cite" class=""><br class="">   ret = G_APPLICATION_CLASS(virt_viewer_parent_class)-<br class=""><blockquote type="cite" class="">local_command_line(gapp, args, status);<br class=""></blockquote>   if (ret)<br class="">@@ -131,15 +178,16 @@ virt_viewer_local_command_line<br class="">(GApplication   *gapp,<br class="">   }<br class=""><br class=""><br class="">-    if (opt_waitvm) {<br class="">+    if (opt_waitvm || domain_selection_type !=<br class="">DOMAIN_SELECTION_NONE) {<br class="">       if (!self->priv->domkey) {<br class="">-            g_printerr(_("\nNo DOMAIN-NAME|ID|UUID was specified<br class="">for '--wait'\n\n"));<br class="">+            g_printerr(missing_domkey_fmt,<br class="">+                       opt_waitvm ? "--wait" :<br class="">domain_selection_to_opt[domain_selection_type]);<br class="">           ret = TRUE;<br class="">           *status = 1;<br class="">           goto end;<br class="">       }<br class=""><br class="">-        self->priv->waitvm = TRUE;<br class="">+        self->priv->waitvm = opt_waitvm;<br class="">   }<br class=""><br class="">   virt_viewer_app_set_direct(app, opt_direct);<br class="">@@ -303,24 +351,42 @@ virt_viewer_lookup_domain(VirtViewer *self)<br class="">{<br class="">   char *end;<br class="">   VirtViewerPrivate *priv = self->priv;<br class="">-    int id;<br class="">   virDomainPtr dom = NULL;<br class="">-    unsigned char uuid[16];<br class=""><br class="">   if (priv->domkey == NULL) {<br class="">       return NULL;<br class="">   }<br class=""><br class="">-    id = strtol(priv->domkey, &end, 10);<br class="">-    if (id >= 0 && end && !*end) {<br class="">-        dom = virDomainLookupByID(priv->conn, id);<br class="">+    switch (domain_selection_type) {<br class="">+    default: /* DOMAIN_SELECTION_NONE */<br class=""></blockquote><br class="">I would add “fallthrough” to the comment as for the next case. GCC<br class="">actually takes this as a hint with (see <a href="https://gcc.gnu.org/onlinedo" class="">https://gcc.gnu.org/onlinedo</a><br class="">cs/gcc/Warning-Options.html, search for -Wimplicit-fallthrough). We<br class="">could also use<br class=""><br class="">   __attribute__ ((fallthrough));<br class=""><br class="">I don’t know if we every build with -Wextra or -Wall?<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">-Wall, I added “fallthrough” where gcc complained (it didn't complain</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">about "default" case)</span></div></blockquote><div><br class=""></div><div>Curious… I would assume it’s more a bug than a feature and add the comment anyways…</div><br class=""><blockquote type="cite" class=""><div class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class=""> tbh I don't understand why it was added to -Wall </span><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">:)</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></blockquote><div><br class=""></div><div>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.</div><div><br class=""></div><div>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 ;-)</div><br class=""><blockquote type="cite" class=""><div class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class=""><br class=""><br class=""><blockquote type="cite" class="">+    case DOMAIN_SELECTION_ID:<br class="">+    {<br class="">+        long int id = strtol(priv->domkey, &end, 10);<br class="">+        if (id >= 0 && end && !*end) {<br class="">+            dom = virDomainLookupByID(priv->conn, id);<br class="">+        }<br class="">+        if (domain_selection_type != DOMAIN_SELECTION_NONE) {<br class="">+            break;<br class="">+        }<br class="">   }<br class="">-    if (!dom && virt_viewer_parse_uuid(priv->domkey, uuid) == 0)<br class="">{<br class="">-        dom = virDomainLookupByUUID(priv->conn, uuid);<br class="">+        /* fallthrough */<br class="">+    case DOMAIN_SELECTION_UUID:<br class="">+    {<br class="">+        unsigned char uuid[16];<br class="">+        if (!dom && virt_viewer_parse_uuid(priv->domkey, uuid) ==<br class="">0) {<br class="">+            dom = virDomainLookupByUUID(priv->conn, uuid);<br class="">+        }<br class="">+        if (domain_selection_type != DOMAIN_SELECTION_NONE) {<br class="">+            break;<br class="">+        }<br class="">   }<br class="">-    if (!dom) {<br class="">-        dom = virDomainLookupByName(priv->conn, priv->domkey);<br class="">+        /* fallthrough */<br class="">+    case DOMAIN_SELECTION_NAME:<br class="">+        if (!dom) {<br class="">+            dom = virDomainLookupByName(priv->conn, priv-<br class=""><blockquote type="cite" class="">domkey);<br class=""></blockquote>+        }<br class="">   }<br class="">+<br class="">   return dom;<br class="">}<br class=""><br class="">-- <br class="">2.12.0<br class=""><br class="">_______________________________________________<br class="">virt-tools-list mailing list<br class=""><a href="mailto:virt-tools-list@redhat.com" class="">virt-tools-list@redhat.com</a><br class="">https://www.redhat.com/mailman/listinfo/virt-tools-list<br class=""></blockquote><br class=""><br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">_______________________________________________</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">virt-tools-list mailing list</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="mailto:virt-tools-list@redhat.com" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">virt-tools-list@redhat.com</a><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="https://www.redhat.com/mailman/listinfo/virt-tools-list" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">https://www.redhat.com/mailman/listinfo/virt-tools-list</a></div></blockquote></div><br class=""></body></html>