From: Alban Lecorps <alban.lecorps(a)ubisoft.com&gt; The "-oo" option will be useful primarily for the openstack output mode (currently disabled, from commit b74c126629e3, "Ignore 'openstack' driver", 2020-03-16). Use a ConfigStringList knob for "-oo", and for each OPTION=VALUE element in that list, create a separate "-oo OPTION=VALUE" option. Do this because there are too many "-oo" options, and because they are output-dependent, and because even virt-v2v "blindly" forwards some of those (such as "-oo os-*=*" for openstack). The "p2v.oo" shorthand is not needed for compatibility, but it's still a good idea for user convenience. (Various tweaks, documentation, and test case update, by Laszlo.) Cc: Alban Lecorps <alban.lecorps(a)ubisoft.com&gt; Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1792141 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- generate-p2v-config.pl | 10 ++++++++++ conversion.c | 9 +++++++++ test-virt-p2v-cmdline.sh | 2 ++ 3 files changed, 21 insertions(+) diff --git a/generate-p2v-config.pl b/generate-p2v-config.pl index 47487f7b0f35..5e1d8519bd91 100755 --- a/generate-p2v-config.pl +++ b/generate-p2v-config.pl @@ -147,6 +147,7 @@ my @fields = [ ConfigString->new(name => 'connection'), ConfigString->new(name => 'format'), ConfigString->new(name => 'storage'), + ConfigStringList->new(name => 'misc'), ], ), ]; @@ -168,6 +169,7 @@ my @cmdline_aliases = ( ["p2v.output.connection", "p2v.oc"], ["p2v.output.format", "p2v.of"], ["p2v.output.storage", "p2v.os"], + ["p2v.output.misc", "p2v.oo"], ); # Some config entries are not exposed on the kernel command line. @@ -379,6 +381,14 @@ option. See L<virt-v2v(1)/OPTIONS>. If not specified, the default is F</var/tmp> (on the conversion server).", ), + "p2v.output.misc" => manual_entry->new( + shortopt => "OPTION=VALUE,...", + description => " +Set miscellaneous output option(s) related to the selected output mode. +This is the same as the virt-v2v I<-oo> option; each C<OPTION=VALUE> +element in the list will be turned into a separate I<-oo OPTION=VALUE> +option on the virt-v2v command line. See L<virt-v2v(1)/OPTIONS>.", + ), ); # Clean up the program name. diff --git a/conversion.c b/conversion.c index b9af47deda74..cc6387c88af8 100644 --- a/conversion.c +++ b/conversion.c @@ -526,6 +526,15 @@ generate_wrapper_script (struct config *config, const char *remote_dir, print_quoted (fp, config->output.storage); } + if (config->output.misc) { /* -oo */ + size_t i; + + for (i = 0; config->output.misc[i]; ++i) { + fprintf (fp, " -oo "); + print_quoted (fp, config->output.misc[i]); + } + } + fprintf (fp, " --root first"); fprintf (fp, " physical.xml"); fprintf (fp, " </dev/null"); /* no stdin */ diff --git a/test-virt-p2v-cmdline.sh b/test-virt-p2v-cmdline.sh index 7d5434cf8428..bcec86089b00 100755 --- a/test-virt-p2v-cmdline.sh +++ b/test-virt-p2v-cmdline.sh @@ -43,6 +43,7 @@ P2V_OPTS=( p2v.oc=qemu:///session p2v.of=raw p2v.os=/var/tmp + p2v.oo=opt1=val1,opt2=val2 p2v.network=em1:wired,other p2v.dump_config_and_exit ) @@ -69,5 +70,6 @@ grep "^output\.allocation.*sparse" $out grep "^output\.connection.*qemu:///session" $out grep "^output\.format.*raw" $out grep "^output\.storage.*/var/tmp" $out +grep "^output\.misc.*opt1=val1 opt2=val2" $out rm $out

what the heck, I'm just noticing I had sent this to Rich only earlier, and not to the list! Resending. FWIW, Rich had accepted my argument (also in an off-list message -- but that's totally my fault; I had gone off-list first) Laszlo. On 1/30/23 19:12, Richard W.M. Jones wrote: I had pondered that for an unreasonably long time :), and then I decided against it. My thinking was, "output.connection" and "output.format" and "output.storage" were already output options. So "output.options" felt like "other output option options". So I stuck with "misc". The second "o" in "-oo" can be read as "-o<other>", not necessarily as "-o<options>", so the second "o" in "-oo" didn't convince me otherwise. I can rename the field if you feel strongly about it. I'd rather like not to, TBH; it's going to create a bunch of conflicts, and, again, the other options that aren't called "options" are still options. Laszlo

Some of the overlong lines in "gui.c" originate from function calls like config->whatever = strdup (gtk_entry_get_text (GTK_ENTRY (entry))); These are hard to wrap -- we generally break the second and further arguments in a function call to new lines, and start those new lines aligned with the opening parenthesis of the function call. However, in the above expression, each function only takes one parameter -- the problem is the deep nesting, and our wrapping style generally strives to reflect nesting. Extract the expression to a small (somewhat accidental) helper function. This function is then used from multiple dialogs (the connection dialog and the conversion dialog). Cc: Alban Lecorps <alban.lecorps(a)ubisoft.com&gt; Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1792141 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- gui.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/gui.c b/gui.c index 3068c97c3ba3..5c53883ff2ae 100644 --- a/gui.c +++ b/gui.c @@ -150,6 +150,16 @@ gui_conversion (struct config *config, gtk_main (); } +/** + * Trivial helper (shorthand) function for duplicating the contents of a + * GTK_ENTRY. + */ +static char * +entry_text_dup (GtkWidget *entry) +{ + return strdup (gtk_entry_get_text (GTK_ENTRY (entry))); +} + /*----------------------------------------------------------------------*/ /* Connection dialog. */ @@ -426,7 +436,7 @@ test_connection_clicked (GtkWidget *w, gpointer data) /* Get the fields from the various widgets. */ free (config->remote.server); - config->remote.server = strdup (gtk_entry_get_text (GTK_ENTRY (server_entry))); + config->remote.server = entry_text_dup (server_entry); if (STREQ (config->remote.server, "")) { gtk_label_set_text (GTK_LABEL (spinner_message), _("error: No conversion server given.")); @@ -442,7 +452,7 @@ test_connection_clicked (GtkWidget *w, gpointer data) errors++; } free (config->auth.username); - config->auth.username = strdup (gtk_entry_get_text (GTK_ENTRY (username_entry))); + config->auth.username = entry_text_dup (username_entry); if (STREQ (config->auth.username, "")) { gtk_label_set_text (GTK_LABEL (spinner_message), _("error: No user name. If in doubt, use \"root\".")); @@ -450,7 +460,7 @@ test_connection_clicked (GtkWidget *w, gpointer data) errors++; } free (config->auth.password); - config->auth.password = strdup (gtk_entry_get_text (GTK_ENTRY (password_entry))); + config->auth.password = entry_text_dup (password_entry); free (config->auth.identity.url); identity_str = gtk_entry_get_text (GTK_ENTRY (identity_entry)); @@ -2064,7 +2074,7 @@ start_conversion_clicked (GtkWidget *w, gpointer data) /* Unpack dialog fields and check them. */ free (config->guestname); - config->guestname = strdup (gtk_entry_get_text (GTK_ENTRY (guestname_entry))); + config->guestname = entry_text_dup (guestname_entry); if (STREQ (config->guestname, "")) { dlg = gtk_message_dialog_new (GTK_WINDOW (conv_dlg),

At this point, the get_phys_topo_from_conv_dlg() function has become useless -- it doesn't save any work, so flatten (inline) its definition at all call sites, and remove the declaration and the definition. Cc: Alban Lecorps <alban.lecorps(a)ubisoft.com&gt; Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1792141 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- gui.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/gui.c b/gui.c index 3c0f21ca87d0..e29a9534a195 100644 --- a/gui.c +++ b/gui.c @@ -686,7 +686,6 @@ static void start_conversion_clicked (GtkWidget *w, gpointer data); static void vcpu_topo_toggled (GtkWidget *w, gpointer data); static void vcpus_or_memory_check_callback (GtkWidget *w, gpointer data); static void notify_ui_callback (int type, const char *data); -static bool get_phys_topo_from_conv_dlg (void); static int get_vcpus_from_conv_dlg (void); static uint64_t get_memory_from_conv_dlg (void); @@ -1662,7 +1661,7 @@ vcpu_topo_toggled (GtkWidget *w, gpointer data) unsigned vcpus; char vcpus_str[64]; - phys_topo = get_phys_topo_from_conv_dlg (); + phys_topo = tgl_btn_is_act (vcpu_topo); if (phys_topo) { struct cpu_topo topo; @@ -1720,12 +1719,6 @@ vcpus_or_memory_check_callback (GtkWidget *w, gpointer data) gtk_label_set_text (GTK_LABEL (target_warning_label), ""); } -static bool -get_phys_topo_from_conv_dlg (void) -{ - return tgl_btn_is_act (vcpu_topo); -} - static int get_vcpus_from_conv_dlg (void) { @@ -2099,7 +2092,7 @@ start_conversion_clicked (GtkWidget *w, gpointer data) return; } - config->vcpu.phys_topo = get_phys_topo_from_conv_dlg (); + config->vcpu.phys_topo = tgl_btn_is_act (vcpu_topo); config->vcpu.cores = get_vcpus_from_conv_dlg (); config->memory = get_memory_from_conv_dlg ();

In "gui.c", we use many long string literals, for labels, warnings, tooltips etc. Wrap them so the source code never exceeds 80 characters in width. Cc: Alban Lecorps <alban.lecorps(a)ubisoft.com&gt; Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1792141 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- gui.c | 58 ++++++++++++++++---- 1 file changed, 46 insertions(+), 12 deletions(-) diff --git a/gui.c b/gui.c index f3dbea7d725a..f86cab649785 100644 --- a/gui.c +++ b/gui.c @@ -215,7 +215,8 @@ create_connection_dialog (struct config *config) gtk_window_set_resizable (GTK_WINDOW (conn_dlg), FALSE); /* The main dialog area. */ - intro = gtk_label_new (_("Connect to a virt-v2v conversion server over SSH:")); + intro = gtk_label_new (_("Connect to a virt-v2v conversion server over " + "SSH:")); gtk_label_set_line_wrap (GTK_LABEL (intro), TRUE); set_padding (intro, 10, 10); @@ -457,7 +458,8 @@ test_connection_clicked (GtkWidget *w, gpointer data) if (sscanf (port_str, "%d", &config->remote.port) != 1 || config->remote.port <= 0 || config->remote.port >= 65536) { gtk_label_set_text (GTK_LABEL (spinner_message), - _("error: Invalid port number. If in doubt, use \"22\".")); + _("error: Invalid port number. If in doubt, use " + "\"22\".")); gtk_widget_grab_focus (port_entry); errors++; } @@ -838,7 +840,17 @@ create_conversion_dialog (struct config *config, set_alignment (o_label, 1., 0.5); o_combo = gtk_combo_box_text_new (); gtk_label_set_mnemonic_widget (GTK_LABEL (o_label), o_combo); - gtk_widget_set_tooltip_markup (o_combo, _("<b>libvirt</b> means send the converted guest to libvirt-managed KVM on the conversion server. <b>local</b> means put it in a directory on the conversion server. <b>rhv</b> means write it to RHV-M/oVirt. <b>glance</b> means write it to OpenStack Glance. See the virt-v2v(1) manual page for more information about output options.")); + gtk_widget_set_tooltip_markup (o_combo, + _("<b>libvirt</b> means send the converted " + "guest to libvirt-managed KVM on the " + "conversion server. " + "<b>local</b> means put it in a directory " + "on the conversion server. " + "<b>rhv</b> means write it to RHV-M/oVirt. " + "<b>glance</b> means write it to OpenStack " + "Glance. " + "See the virt-v2v(1) manual page for more " + "information about output options.")); repopulate_output_combo (config); table_attach (output_tbl, o_combo, 1, 2, row, GTK_FILL, GTK_FILL, 1, 1); @@ -850,7 +862,12 @@ create_conversion_dialog (struct config *config, set_alignment (oc_label, 1., 0.5); oc_entry = gtk_entry_new (); gtk_label_set_mnemonic_widget (GTK_LABEL (oc_label), oc_entry); - gtk_widget_set_tooltip_markup (oc_entry, _("For <b>libvirt</b> only, the libvirt connection URI, or leave blank to add the guest to the default libvirt instance on the conversion server. For others, leave this field blank.")); + gtk_widget_set_tooltip_markup (oc_entry, + _("For <b>libvirt</b> only, the libvirt " + "connection URI, or leave blank to add the " + "guest to the default libvirt instance on " + "the conversion server. " + "For others, leave this field blank.")); if (config->output.connection != NULL) gtk_entry_set_text (GTK_ENTRY (oc_entry), config->output.connection); table_attach (output_tbl, oc_entry, @@ -863,7 +880,12 @@ create_conversion_dialog (struct config *config, set_alignment (os_label, 1., 0.5); os_entry = gtk_entry_new (); gtk_label_set_mnemonic_widget (GTK_LABEL (os_label), os_entry); - gtk_widget_set_tooltip_markup (os_entry, _("For <b>local</b>, put the directory name on the conversion server. For <b>rhv</b>, put the Export Storage Domain (server:/mountpoint). For others, leave this field blank.")); + gtk_widget_set_tooltip_markup (os_entry, + _("For <b>local</b>, put the directory name " + "on the conversion server. " + "For <b>rhv</b>, put the Export Storage " + "Domain (server:/mountpoint). " + "For others, leave this field blank.")); if (config->output.storage != NULL) gtk_entry_set_text (GTK_ENTRY (os_entry), config->output.storage); table_attach (output_tbl, os_entry, @@ -876,7 +898,10 @@ create_conversion_dialog (struct config *config, set_alignment (of_label, 1., 0.5); of_entry = gtk_entry_new (); gtk_label_set_mnemonic_widget (GTK_LABEL (of_label), of_entry); - gtk_widget_set_tooltip_markup (of_entry, _("The output disk format, typically <b>raw</b> or <b>qcow2</b>. If blank, defaults to <b>raw</b>.")); + gtk_widget_set_tooltip_markup (of_entry, + _("The output disk format, typically " + "<b>raw</b> or <b>qcow2</b>. " + "If blank, defaults to <b>raw</b>.")); if (config->output.format != NULL) gtk_entry_set_text (GTK_ENTRY (of_entry), config->output.format); table_attach (output_tbl, of_entry, @@ -947,7 +972,9 @@ create_conversion_dialog (struct config *config, /* See maybe_identify_click below for what we're doing. */ g_signal_connect (interfaces_list, "button-press-event", G_CALLBACK (maybe_identify_click), NULL); - gtk_widget_set_tooltip_markup (interfaces_list, _("Left click on an interface name to flash the light on the physical interface.")); + gtk_widget_set_tooltip_markup (interfaces_list, + _("Left click on an interface name to flash " + "the light on the physical interface.")); populate_interfaces (GTK_TREE_VIEW (interfaces_list)); scrolled_window_add_with_viewport (interfaces_sw, interfaces_list); gtk_container_add (GTK_CONTAINER (interfaces_frame), interfaces_sw); @@ -1273,7 +1300,9 @@ populate_interfaces (GtkTreeView *interfaces_list_p) "%s\n" "%s" "</small>\n" - "<small><u><span foreground=\"blue\">Identify interface</span></u></small>", + "<small><u><span foreground=\"blue\">" + "Identify interface" + "</span></u></small>", if_name, if_addr ? : _("Unknown"), if_vendor ? : _("Unknown")) == -1) @@ -1699,7 +1728,8 @@ vcpus_or_memory_check_callback (GtkWidget *w, gpointer data) gtk_widget_show (vcpus_warning); warning = concat_warning (warning, - _("Number of virtual CPUs is larger than what is supported for KVM (max: %d)."), + _("Number of virtual CPUs is larger than what is " + "supported for KVM (max: %d)."), MAX_SUPPORTED_VCPUS); } else @@ -1709,7 +1739,8 @@ vcpus_or_memory_check_callback (GtkWidget *w, gpointer data) gtk_widget_show (memory_warning); warning = concat_warning (warning, - _("Memory size is larger than what is supported for KVM (max: %" PRIu64 ")."), + _("Memory size is larger than what is supported " + "for KVM (max: %" PRIu64 ")."), MAX_SUPPORTED_MEMORY_MB); } else @@ -1717,7 +1748,9 @@ vcpus_or_memory_check_callback (GtkWidget *w, gpointer data) if (warning != NULL) { warning = concat_warning (warning, - _("If you ignore this warning, conversion can still succeed, but the guest may not work or may not be supported on the target.")); + _("If you ignore this warning, conversion can " + "still succeed, but the guest may not work or " + "may not be supported on the target.")); gtk_label_set_text (GTK_LABEL (target_warning_label), warning); } else @@ -2115,7 +2148,8 @@ start_conversion_clicked (GtkWidget *w, gpointer data) GTK_MESSAGE_ERROR, GTK_BUTTONS_OK, _("No disks were selected for conversion.\n" - "At least one fixed hard disk must be selected.\n")); + "At least one fixed hard disk must be " + "selected.\n")); gtk_window_set_title (GTK_WINDOW (dlg), _("Error")); gtk_dialog_run (GTK_DIALOG (dlg)); gtk_widget_destroy (dlg);

On Mon, Jan 30, 2023 at 03:22:26PM +0100, Laszlo Ersek wrote: My concern here is that someone is going to put "foo=bar, baz=1" in this field, and it will improperly split above. So maybe a bit of trimming on the resulting array? Be kind of nice to add a tiny bit more documentation here, especially as there's only a tool tip telling you what the field should contain. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW

With "-oo" exposed on the GUI, the reason for disabling the openstack driver (from commit b74c126629e3, "Ignore 'openstack' driver", 2020-03-16) is gone. Reenable the driver. At the same time, add two small openstack-related hints to the GUI. Cc: Alban Lecorps <alban.lecorps(a)ubisoft.com&gt; Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1792141 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- gui.c | 5 ++++- ssh.c | 5 ++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/gui.c b/gui.c index 040b43dd235d..d61069dc0404 100644 --- a/gui.c +++ b/gui.c @@ -848,6 +848,8 @@ create_conversion_dialog (struct config *config, "<b>local</b> means put it in a directory " "on the conversion server. " "<b>rhv</b> means write it to RHV-M/oVirt. " + "<b>openstack</b> means write it to " + "OpenStack. " "<b>glance</b> means write it to OpenStack " "Glance. " "See the virt-v2v(1) manual page for more " @@ -942,7 +944,8 @@ create_conversion_dialog (struct config *config, "<b>OPTION=VALUE</b> output-specific " "options. Each option is passed to " "virt-v2v as the argument of a separate " - "<b>-oo</b> option.")); + "<b>-oo</b> option. Mainly useful for " + "<b>openstack</b>.")); populate_misc_opts (GTK_ENTRY (oo_entry), config->output.misc); table_attach (output_tbl, oo_entry, 1, 2, row, GTK_FILL, GTK_FILL, 1, 1); diff --git a/ssh.c b/ssh.c index 513a20318359..ec44e77895a5 100644 --- a/ssh.c +++ b/ssh.c @@ -1015,11 +1015,10 @@ static void add_output_driver (const char *name) { /* Ignore the 'vdsm' driver, since that should only be used by VDSM. - * Ignore the 'openstack' and 'rhv-upload' drivers, since we do not - * support passing all the options for them. + * Ignore the 'rhv-upload' driver, since we do not support passing all the + * options for it. */ if (STRNEQ (name, "vdsm") && - STRNEQ (name, "openstack") && STRNEQ (name, "rhv-upload")) add_option ("output", &output_drivers, name); }