<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>