In set_from_ui_generic(), free the (*ret) string list on the common path. No functional change. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2124538 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- gui.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/gui.c b/gui.c index 4b98a44fbbe9..561ba64d2a71 100644 --- a/gui.c +++ b/gui.c @@ -1404,15 +1404,14 @@ set_from_ui_generic (char **all, char ***ret, GtkTreeView *list) gboolean b, v; size_t i, j; + guestfs_int_free_string_list (*ret); if (all == NULL) { - guestfs_int_free_string_list (*ret); *ret = NULL; return; } model = gtk_tree_view_get_model (list); - guestfs_int_free_string_list (*ret); *ret = malloc ((1 + guestfs_int_count_strings (all)) * sizeof (char *)); if (*ret == NULL) error (EXIT_FAILURE, errno, "malloc");

Currently "all_disks" and "all_removables" are used for both populating the conversion dialog and collecting the configuration updates from the conversion dialog. Going forward, we want to eliminate "all_disks" and "all_removables" as global variables. As one step in that direction, restrict the consumption of "all_disks" and "all_removables" to when we populate the conversion dialog. For this, introduce a new column to the list stores that back the "fixed disks" and "removable media drives" tree views, and copy "all_disks" and "all_removables" into the new columns, respectively. This new column is never displayed; it's only used as a lookaside buffer when we collect the disks / removables selections into the config structure. Note that both gtk_list_store_set() and gtk_tree_model_get() create deep copies of strings. No observable changes. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2124538 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- gui.c | 90 +++++++++++++------- 1 file changed, 60 insertions(+), 30 deletions(-) diff --git a/gui.c b/gui.c index c40ed31a8bbc..82c297b0be4b 100644 --- a/gui.c +++ b/gui.c @@ -702,12 +702,14 @@ static uint64_t get_memory_from_conv_dlg (void); enum { DISKS_COL_CONVERT = 0, + DISKS_COL_HW_NAME, DISKS_COL_DEVICE, NUM_DISKS_COLS, }; enum { REMOVABLE_COL_CONVERT = 0, + REMOVABLE_COL_HW_NAME, REMOVABLE_COL_DEVICE, NUM_REMOVABLE_COLS, }; @@ -1103,7 +1105,8 @@ populate_disks (GtkTreeView *disks_list_p) size_t i; disks_store = gtk_list_store_new (NUM_DISKS_COLS, - G_TYPE_BOOLEAN, G_TYPE_STRING); + G_TYPE_BOOLEAN, G_TYPE_STRING, + G_TYPE_STRING); if (all_disks != NULL) { for (i = 0; all_disks[i] != NULL; ++i) { uint64_t size; @@ -1134,6 +1137,7 @@ populate_disks (GtkTreeView *disks_list_p) gtk_list_store_append (disks_store, &iter); gtk_list_store_set (disks_store, &iter, DISKS_COL_CONVERT, TRUE, + DISKS_COL_HW_NAME, all_disks[i], DISKS_COL_DEVICE, device_descr, -1); } @@ -1174,7 +1178,8 @@ populate_removable (GtkTreeView *removable_list_p) size_t i; removable_store = gtk_list_store_new (NUM_REMOVABLE_COLS, - G_TYPE_BOOLEAN, G_TYPE_STRING); + G_TYPE_BOOLEAN, G_TYPE_STRING, + G_TYPE_STRING); if (all_removable != NULL) { for (i = 0; all_removable[i] != NULL; ++i) { CLEANUP_FREE char *device_descr = NULL; @@ -1185,6 +1190,7 @@ populate_removable (GtkTreeView *removable_list_p) gtk_list_store_append (removable_store, &iter); gtk_list_store_set (removable_store, &iter, REMOVABLE_COL_CONVERT, TRUE, + REMOVABLE_COL_HW_NAME, all_removable[i], REMOVABLE_COL_DEVICE, device_descr, -1); } @@ -1397,52 +1403,76 @@ maybe_identify_click (GtkWidget *interfaces_list_p, GdkEventButton *event, } static void -set_from_ui_generic (char **all, char ***ret, GtkTreeView *list) +set_from_ui_generic (char ***ret, GtkTreeView *list) { GtkTreeModel *model; - GtkTreeIter iter; - gboolean b, v; - size_t i, j; + enum { COUNT, COPY, DONE } mode; guestfs_int_free_string_list (*ret); - if (all == NULL) { - *ret = NULL; - return; - } + *ret = NULL; model = gtk_tree_view_get_model (list); - *ret = malloc ((1 + guestfs_int_count_strings (all)) * sizeof (char *)); - if (*ret == NULL) - error (EXIT_FAILURE, errno, "malloc"); - i = j = 0; - - b = gtk_tree_model_get_iter_first (model, &iter); - while (b) { - gtk_tree_model_get (model, &iter, 0 /* CONVERT */, &v, -1); - if (v) { - assert (all[i] != NULL); - (*ret)[j++] = strdup (all[i]); + for (mode = COUNT; mode != DONE; mode++) { + size_t found; + gboolean avail; + GtkTreeIter iter; + + found = 0; + avail = gtk_tree_model_get_iter_first (model, &iter); + while (avail) { + gboolean active; + + gtk_tree_model_get (model, &iter, 0 /* CONVERT */, &active, -1); + if (active) { + if (mode == COPY) { + gchar *hw_name; + + gtk_tree_model_get (model, &iter, 1 /* HW_NAME */, &hw_name, -1); + + /* gtk_tree_model_get() outputs a deep copy, so no need for an + * strdup() here + */ + (*ret)[found] = hw_name; + } + + found++; + } + + avail = gtk_tree_model_iter_next (model, &iter); + } /* iteration over entries of "model" */ + + switch (mode) { + case COUNT: + if (found == 0) + return; + + *ret = malloc ((found + 1) * sizeof (char *)); + if (*ret == NULL) + error (EXIT_FAILURE, errno, "malloc"); + + break; + + case COPY: + (*ret)[found] = NULL; + break; + + default: + assert (0); } - b = gtk_tree_model_iter_next (model, &iter); - ++i; - } - - (*ret)[j] = NULL; + } /* "mode" loop */ } static void set_disks_from_ui (struct config *config) { - set_from_ui_generic (all_disks, &config->disks, - GTK_TREE_VIEW (disks_list)); + set_from_ui_generic (&config->disks, GTK_TREE_VIEW (disks_list)); } static void set_removable_from_ui (struct config *config) { - set_from_ui_generic (all_removable, &config->removable, - GTK_TREE_VIEW (removable_list)); + set_from_ui_generic (&config->removable, GTK_TREE_VIEW (removable_list)); } static void

Give find_all_disks() external linkage, moving it to a new source file called "disks.c". Move the helpers called solely by find_all_disks() as well, such as device_contains() and partition_parent(). Move the global variables "all_disks" and "all_removable" too. compare_strings() is now referenced from "main.c" and "disks.c". No functional changes. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2124538 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- Makefile.am | 1 + p2v.h | 16 +- disks.c | 185 ++++++++++++++++++++ main.c | 157 ----------------- 4 files changed, 198 insertions(+), 161 deletions(-) diff --git a/Makefile.am b/Makefile.am index 19c5f04c2fab..4a47a97251e9 100644 --- a/Makefile.am +++ b/Makefile.am @@ -108,6 +108,7 @@ virt_p2v_SOURCES = \ libguestfs/libxml2-writer-macros.h \ conversion.c \ cpuid.c \ + disks.c \ gui.c \ gui-gtk2-compat.h \ gui-gtk3-compat.h \ diff --git a/p2v.h b/p2v.h index e91c47c36428..e7f0b9e467fd 100644 --- a/p2v.h +++ b/p2v.h @@ -40,11 +40,9 @@ # define P2V_GCC_VERSION 0 #endif -/* All disks / removable media / network interfaces discovered - * when the program started. Do not change these. +/* All network interfaces discovered when the program started. Do not change + * this. */ -extern char **all_disks; -extern char **all_removable; extern char **all_interfaces; /* True if running inside the virt-p2v ISO environment. Various @@ -68,6 +66,16 @@ struct cpu_topo { extern void get_cpu_topology (struct cpu_topo *topo); extern void get_cpu_config (struct cpu_config *); +/* disks.c + * + * All disks / removable media discovered (possibly with one call to + * find_all_disks()) when the program started. Do not change these, or call + * find_all_disks() more than once. + */ +extern char **all_disks; +extern char **all_removable; +extern void find_all_disks (void); + /* rtc.c */ extern void get_rtc_config (struct rtc_config *); diff --git a/disks.c b/disks.c new file mode 100644 index 000000000000..4f94719787ac --- /dev/null +++ b/disks.c @@ -0,0 +1,185 @@ +/* virt-p2v + * Copyright (C) 2009-2022 Red Hat Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <https://www.gnu.org/licenses/>;. + */ + +#include <config.h> + +#include <dirent.h> +#include <errno.h> +#include <error.h> +#include <inttypes.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <sys/stat.h> +#include <sys/types.h> + +#if MAJOR_IN_MKDEV +#include <sys/mkdev.h> +#elif MAJOR_IN_SYSMACROS +#include <sys/sysmacros.h> +/* else it's in sys/types.h, included above */ +#endif + +#include "p2v.h" + +char **all_disks; +char **all_removable; + +/** + * Get parent device of a partition. + * + * Returns C<0> if no parent device could be found. + */ +static dev_t +partition_parent (dev_t part_dev) +{ + CLEANUP_FCLOSE FILE *fp = NULL; + CLEANUP_FREE char *path = NULL, *content = NULL; + size_t len = 0; + unsigned parent_major, parent_minor; + + if (asprintf (&path, "/sys/dev/block/%ju:%ju/../dev", + (uintmax_t) major (part_dev), + (uintmax_t) minor (part_dev)) == -1) + error (EXIT_FAILURE, errno, "asprintf"); + + fp = fopen (path, "r"); + if (fp == NULL) + return 0; + + if (getline (&content, &len, fp) == -1) + error (EXIT_FAILURE, errno, "getline"); + + if (sscanf (content, "%u:%u", &parent_major, &parent_minor) != 2) + return 0; + + return makedev (parent_major, parent_minor); +} + +/** + * Return true if the named device (eg. C<dev == "sda">) contains the + * root filesystem. C<root_device> is the major:minor of the root + * filesystem (eg. C<8:1> if the root filesystem was F</dev/sda1>). + * + * This doesn't work for LVs and so on. However we only really care + * if this test works on the P2V ISO where the root device is a + * regular partition. + */ +static int +device_contains (const char *dev, dev_t root_device) +{ + struct stat statbuf; + CLEANUP_FREE char *dev_name = NULL; + dev_t root_device_parent; + + if (asprintf (&dev_name, "/dev/%s", dev) == -1) + error (EXIT_FAILURE, errno, "asprintf"); + + if (stat (dev_name, &statbuf) == -1) + return 0; + + /* See if dev is the root_device. */ + if (statbuf.st_rdev == root_device) + return 1; + + /* See if dev is the parent device of the root_device. */ + root_device_parent = partition_parent (root_device); + if (root_device_parent == 0) + return 0; + if (statbuf.st_rdev == root_device_parent) + return 1; + + return 0; +} + +/** + * Enumerate all disks in F</sys/block> and add them to the global + * C<all_disks> and C<all_removable> arrays. + */ +void +find_all_disks (void) +{ + DIR *dir; + struct dirent *d; + size_t nr_disks = 0, nr_removable = 0; + dev_t root_device = 0; + struct stat statbuf; + + if (stat ("/", &statbuf) == 0) + root_device = statbuf.st_dev; + + /* The default list of disks is everything in /sys/block which + * matches the common patterns for disk names. + */ + dir = opendir ("/sys/block"); + if (!dir) + error (EXIT_FAILURE, errno, "opendir"); + + for (;;) { + errno = 0; + d = readdir (dir); + if (!d) break; + + if (STRPREFIX (d->d_name, "cciss!") || + STRPREFIX (d->d_name, "hd") || + STRPREFIX (d->d_name, "nvme") || + STRPREFIX (d->d_name, "sd") || + STRPREFIX (d->d_name, "ubd") || + STRPREFIX (d->d_name, "vd")) { + char *p; + + /* Skip the device containing the root filesystem. */ + if (device_contains (d->d_name, root_device)) + continue; + + nr_disks++; + all_disks = realloc (all_disks, sizeof (char *) * (nr_disks + 1)); + if (!all_disks) + error (EXIT_FAILURE, errno, "realloc"); + + all_disks[nr_disks-1] = strdup (d->d_name); + + /* cciss device /dev/cciss/c0d0 will be /sys/block/cciss!c0d0 */ + p = strchr (all_disks[nr_disks-1], '!'); + if (p) *p = '/'; + + all_disks[nr_disks] = NULL; + } + else if (STRPREFIX (d->d_name, "sr")) { + nr_removable++; + all_removable = realloc (all_removable, + sizeof (char *) * (nr_removable + 1)); + if (!all_removable) + error (EXIT_FAILURE, errno, "realloc"); + all_removable[nr_removable-1] = strdup (d->d_name); + all_removable[nr_removable] = NULL; + } + } + + /* Check readdir didn't fail */ + if (errno != 0) + error (EXIT_FAILURE, errno, "readdir: %s", "/sys/block"); + + /* Close the directory handle */ + if (closedir (dir) == -1) + error (EXIT_FAILURE, errno, "closedir: %s", "/sys/block"); + + if (all_disks) + qsort (all_disks, nr_disks, sizeof (char *), compare_strings); + if (all_removable) + qsort (all_removable, nr_removable, sizeof (char *), compare_strings); +} diff --git a/main.c b/main.c index ef191e9e34be..a83de71b7c73 100644 --- a/main.c +++ b/main.c @@ -20,7 +20,6 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> -#include <inttypes.h> #include <unistd.h> #include <getopt.h> #include <fcntl.h> @@ -30,14 +29,6 @@ #include <locale.h> #include <libintl.h> #include <sys/types.h> -#include <sys/stat.h> - -#if MAJOR_IN_MKDEV -#include <sys/mkdev.h> -#elif MAJOR_IN_SYSMACROS -#include <sys/sysmacros.h> -/* else it's in sys/types.h, included above */ -#endif /* errors in <gtk.h> */ #pragma GCC diagnostic push @@ -51,8 +42,6 @@ #include "ignore-value.h" #include "p2v.h" -char **all_disks; -char **all_removable; char **all_interfaces; int is_iso_environment = 0; int feature_colours_option = 0; @@ -61,7 +50,6 @@ static const char *test_disk = NULL; static void udevadm_settle (void); static void set_config_defaults (struct config *config); -static void find_all_disks (void); static void find_all_interfaces (void); enum { HELP_OPTION = CHAR_MAX + 1 }; @@ -372,151 +360,6 @@ set_config_defaults (struct config *config) config->output.storage = strdup ("/var/tmp"); } -/** - * Get parent device of a partition. - * - * Returns C<0> if no parent device could be found. - */ -static dev_t -partition_parent (dev_t part_dev) -{ - CLEANUP_FCLOSE FILE *fp = NULL; - CLEANUP_FREE char *path = NULL, *content = NULL; - size_t len = 0; - unsigned parent_major, parent_minor; - - if (asprintf (&path, "/sys/dev/block/%ju:%ju/../dev", - (uintmax_t) major (part_dev), - (uintmax_t) minor (part_dev)) == -1) - error (EXIT_FAILURE, errno, "asprintf"); - - fp = fopen (path, "r"); - if (fp == NULL) - return 0; - - if (getline (&content, &len, fp) == -1) - error (EXIT_FAILURE, errno, "getline"); - - if (sscanf (content, "%u:%u", &parent_major, &parent_minor) != 2) - return 0; - - return makedev (parent_major, parent_minor); -} - -/** - * Return true if the named device (eg. C<dev == "sda">) contains the - * root filesystem. C<root_device> is the major:minor of the root - * filesystem (eg. C<8:1> if the root filesystem was F</dev/sda1>). - * - * This doesn't work for LVs and so on. However we only really care - * if this test works on the P2V ISO where the root device is a - * regular partition. - */ -static int -device_contains (const char *dev, dev_t root_device) -{ - struct stat statbuf; - CLEANUP_FREE char *dev_name = NULL; - dev_t root_device_parent; - - if (asprintf (&dev_name, "/dev/%s", dev) == -1) - error (EXIT_FAILURE, errno, "asprintf"); - - if (stat (dev_name, &statbuf) == -1) - return 0; - - /* See if dev is the root_device. */ - if (statbuf.st_rdev == root_device) - return 1; - - /* See if dev is the parent device of the root_device. */ - root_device_parent = partition_parent (root_device); - if (root_device_parent == 0) - return 0; - if (statbuf.st_rdev == root_device_parent) - return 1; - - return 0; -} - -/** - * Enumerate all disks in F</sys/block> and add them to the global - * C<all_disks> and C<all_removable> arrays. - */ -static void -find_all_disks (void) -{ - DIR *dir; - struct dirent *d; - size_t nr_disks = 0, nr_removable = 0; - dev_t root_device = 0; - struct stat statbuf; - - if (stat ("/", &statbuf) == 0) - root_device = statbuf.st_dev; - - /* The default list of disks is everything in /sys/block which - * matches the common patterns for disk names. - */ - dir = opendir ("/sys/block"); - if (!dir) - error (EXIT_FAILURE, errno, "opendir"); - - for (;;) { - errno = 0; - d = readdir (dir); - if (!d) break; - - if (STRPREFIX (d->d_name, "cciss!") || - STRPREFIX (d->d_name, "hd") || - STRPREFIX (d->d_name, "nvme") || - STRPREFIX (d->d_name, "sd") || - STRPREFIX (d->d_name, "ubd") || - STRPREFIX (d->d_name, "vd")) { - char *p; - - /* Skip the device containing the root filesystem. */ - if (device_contains (d->d_name, root_device)) - continue; - - nr_disks++; - all_disks = realloc (all_disks, sizeof (char *) * (nr_disks + 1)); - if (!all_disks) - error (EXIT_FAILURE, errno, "realloc"); - - all_disks[nr_disks-1] = strdup (d->d_name); - - /* cciss device /dev/cciss/c0d0 will be /sys/block/cciss!c0d0 */ - p = strchr (all_disks[nr_disks-1], '!'); - if (p) *p = '/'; - - all_disks[nr_disks] = NULL; - } - else if (STRPREFIX (d->d_name, "sr")) { - nr_removable++; - all_removable = realloc (all_removable, - sizeof (char *) * (nr_removable + 1)); - if (!all_removable) - error (EXIT_FAILURE, errno, "realloc"); - all_removable[nr_removable-1] = strdup (d->d_name); - all_removable[nr_removable] = NULL; - } - } - - /* Check readdir didn't fail */ - if (errno != 0) - error (EXIT_FAILURE, errno, "readdir: %s", "/sys/block"); - - /* Close the directory handle */ - if (closedir (dir) == -1) - error (EXIT_FAILURE, errno, "closedir: %s", "/sys/block"); - - if (all_disks) - qsort (all_disks, nr_disks, sizeof (char *), compare_strings); - if (all_removable) - qsort (all_removable, nr_removable, sizeof (char *), compare_strings); -} - /** * Enumerate all network interfaces in F</sys/class/net> and add them * to the global C<all_interfaces> array.

In populate_disks() and populate_removable(), use local variables called "disks" and "removable", respectively, for most of the logic. Set each local at the top of the corresponding function, from the corresponding globals "all_disks" and "all_removable". No observable changes. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2124538 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- gui.c | 26 +++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/gui.c b/gui.c index 82c297b0be4b..bbd793c70d95 100644 --- a/gui.c +++ b/gui.c @@ -1099,6 +1099,7 @@ repopulate_output_combo (struct config *config) static void populate_disks (GtkTreeView *disks_list_p) { + const char * const *disks = (const char **)all_disks; GtkListStore *disks_store; GtkCellRenderer *disks_col_convert, *disks_col_device; GtkTreeIter iter; @@ -1107,20 +1108,20 @@ populate_disks (GtkTreeView *disks_list_p) disks_store = gtk_list_store_new (NUM_DISKS_COLS, G_TYPE_BOOLEAN, G_TYPE_STRING, G_TYPE_STRING); - if (all_disks != NULL) { - for (i = 0; all_disks[i] != NULL; ++i) { + if (disks != NULL) { + for (i = 0; disks[i] != NULL; ++i) { uint64_t size; CLEANUP_FREE char *size_gb = NULL; CLEANUP_FREE char *model = NULL; CLEANUP_FREE char *serial = NULL; CLEANUP_FREE char *device_descr = NULL; - if (all_disks[i][0] != '/') { /* not using --test-disk */ - size = get_blockdev_size (all_disks[i]); + if (disks[i][0] != '/') { /* not using --test-disk */ + size = get_blockdev_size (disks[i]); if (asprintf (&size_gb, "%" PRIu64 "G", size) == -1) error (EXIT_FAILURE, errno, "asprintf"); - model = get_blockdev_model (all_disks[i]); - serial = get_blockdev_serial (all_disks[i]); + model = get_blockdev_model (disks[i]); + serial = get_blockdev_serial (disks[i]); } if (asprintf (&device_descr, @@ -1129,7 +1130,7 @@ populate_disks (GtkTreeView *disks_list_p) "%s %s\n" "%s%s" "</small>", - all_disks[i], + disks[i], size_gb ? size_gb : "", model ? model : "", serial ? "s/n " : "", serial ? serial : "") == -1) error (EXIT_FAILURE, errno, "asprintf"); @@ -1137,7 +1138,7 @@ populate_disks (GtkTreeView *disks_list_p) gtk_list_store_append (disks_store, &iter); gtk_list_store_set (disks_store, &iter, DISKS_COL_CONVERT, TRUE, - DISKS_COL_HW_NAME, all_disks[i], + DISKS_COL_HW_NAME, disks[i], DISKS_COL_DEVICE, device_descr, -1); } @@ -1172,6 +1173,7 @@ populate_disks (GtkTreeView *disks_list_p) static void populate_removable (GtkTreeView *removable_list_p) { + const char * const *removable = (const char **)all_removable; GtkListStore *removable_store; GtkCellRenderer *removable_col_convert, *removable_col_device; GtkTreeIter iter; @@ -1180,17 +1182,17 @@ populate_removable (GtkTreeView *removable_list_p) removable_store = gtk_list_store_new (NUM_REMOVABLE_COLS, G_TYPE_BOOLEAN, G_TYPE_STRING, G_TYPE_STRING); - if (all_removable != NULL) { - for (i = 0; all_removable[i] != NULL; ++i) { + if (removable != NULL) { + for (i = 0; removable[i] != NULL; ++i) { CLEANUP_FREE char *device_descr = NULL; - if (asprintf (&device_descr, "<b>%s</b>\n", all_removable[i]) == -1) + if (asprintf (&device_descr, "<b>%s</b>\n", removable[i]) == -1) error (EXIT_FAILURE, errno, "asprintf"); gtk_list_store_append (removable_store, &iter); gtk_list_store_set (removable_store, &iter, REMOVABLE_COL_CONVERT, TRUE, - REMOVABLE_COL_HW_NAME, all_removable[i], + REMOVABLE_COL_HW_NAME, removable[i], REMOVABLE_COL_DEVICE, device_descr, -1); }

On Mon, Sep 19, 2022 at 03:35:04PM +0200, Laszlo Ersek wrote: This is not wrong, but I'll note why it was done this way originally. It's to keep the ordinary path first and the test path (only used by a make check-* rule) second. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top

At this point, in "gui.c", we only consume the "all_disks" and "all_removable" global variables for populating the conversion dialog, when said dialog is first created. Eliminate those global variable references, and pass them down instead as function parameters, via main() -> gui_conversion() -> create_conversion_dialog() -> { populate_disks(), populate_removable() }. No observable changes. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2124538 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- p2v.h | 4 ++- gui.c | 31 ++++++++++++-------- main.c | 3 +- 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/p2v.h b/p2v.h index e7f0b9e467fd..d48b4fcc6611 100644 --- a/p2v.h +++ b/p2v.h @@ -94,7 +94,9 @@ extern void update_config_from_kernel_cmdline (struct config *config, char **cmd extern void kernel_conversion (struct config *, char **cmdline, int cmdline_source); /* gui.c */ -extern void gui_conversion (struct config *); +extern void gui_conversion (struct config *config, + const char * const *disks, + const char * const *removable); /* conversion.c */ struct data_conn { /* Data per NBD connection / physical disk. */ diff --git a/gui.c b/gui.c index bbd793c70d95..2cf30943bb5e 100644 --- a/gui.c +++ b/gui.c @@ -101,7 +101,9 @@ #endif static void create_connection_dialog (struct config *); -static void create_conversion_dialog (struct config *); +static void create_conversion_dialog (struct config *config, + const char * const *disks, + const char * const *removable); static void create_running_dialog (void); static void show_connection_dialog (void); static void show_conversion_dialog (void); @@ -160,11 +162,13 @@ static const char gplv2plus[] = * Note that C<gtk_init> etc have already been called in C<main>. */ void -gui_conversion (struct config *config) +gui_conversion (struct config *config, + const char * const *disks, + const char * const *removable) { /* Create the dialogs. */ create_connection_dialog (config); - create_conversion_dialog (config); + create_conversion_dialog (config, disks, removable); create_running_dialog (); /* Start by displaying the connection dialog. */ @@ -680,8 +684,10 @@ connection_next_clicked (GtkWidget *w, gpointer data) /*----------------------------------------------------------------------*/ /* Conversion dialog. */ -static void populate_disks (GtkTreeView *disks_list_p); -static void populate_removable (GtkTreeView *removable_list_p); +static void populate_disks (GtkTreeView *disks_list_p, + const char * const *disks); +static void populate_removable (GtkTreeView *removable_list_p, + const char * const *removable); static void populate_interfaces (GtkTreeView *interfaces_list_p); static void toggled (GtkCellRendererToggle *cell, gchar *path_str, gpointer data); static void network_edited_callback (GtkCellRendererToggle *cell, gchar *path_str, gchar *new_text, gpointer data); @@ -728,7 +734,9 @@ enum { * C<show_conversion_dialog>. */ static void -create_conversion_dialog (struct config *config) +create_conversion_dialog (struct config *config, + const char * const *disks, + const char * const *removable) { GtkWidget *back; GtkWidget *hbox, *left_vbox, *right_vbox; @@ -925,7 +933,7 @@ create_conversion_dialog (struct config *config) gtk_scrolled_window_set_policy (GTK_SCROLLED_WINDOW (disks_sw), GTK_POLICY_NEVER, GTK_POLICY_AUTOMATIC); disks_list = gtk_tree_view_new (); - populate_disks (GTK_TREE_VIEW (disks_list)); + populate_disks (GTK_TREE_VIEW (disks_list), disks); scrolled_window_add_with_viewport (disks_sw, disks_list); gtk_container_add (GTK_CONTAINER (disks_frame), disks_sw); @@ -936,7 +944,7 @@ create_conversion_dialog (struct config *config) gtk_scrolled_window_set_policy (GTK_SCROLLED_WINDOW (removable_sw), GTK_POLICY_NEVER, GTK_POLICY_AUTOMATIC); removable_list = gtk_tree_view_new (); - populate_removable (GTK_TREE_VIEW (removable_list)); + populate_removable (GTK_TREE_VIEW (removable_list), removable); scrolled_window_add_with_viewport (removable_sw, removable_list); gtk_container_add (GTK_CONTAINER (removable_frame), removable_sw); @@ -1097,9 +1105,8 @@ repopulate_output_combo (struct config *config) * Populate the C<Fixed hard disks> treeview. */ static void -populate_disks (GtkTreeView *disks_list_p) +populate_disks (GtkTreeView *disks_list_p, const char * const *disks) { - const char * const *disks = (const char **)all_disks; GtkListStore *disks_store; GtkCellRenderer *disks_col_convert, *disks_col_device; GtkTreeIter iter; @@ -1171,9 +1178,9 @@ populate_disks (GtkTreeView *disks_list_p) * Populate the C<Removable media> treeview. */ static void -populate_removable (GtkTreeView *removable_list_p) +populate_removable (GtkTreeView *removable_list_p, + const char * const *removable) { - const char * const *removable = (const char **)all_removable; GtkListStore *removable_store; GtkCellRenderer *removable_col_convert, *removable_col_device; GtkTreeIter iter; diff --git a/main.c b/main.c index 6ebb71ce4314..86820dd27e08 100644 --- a/main.c +++ b/main.c @@ -256,7 +256,8 @@ main (int argc, char *argv[]) error (EXIT_FAILURE, 0, _("gtk_init_check returned false, indicating that\n" "a GUI is not possible on this host. Check X11, $DISPLAY etc.")); - gui_conversion (config); + gui_conversion (config, (const char **)all_disks, + (const char **)all_removable); } guestfs_int_free_string_list (cmdline);

Extract populate_disks_store() from populate_disks(). Extract populate_removable_store() from populate_removable(). This enables us to reuse the GtkListStore-filling loops later. While at it, move the "iter" local variables to the tightest scopes possible. No functional changes. This patch is easier to review with "git show -b". Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2124538 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- gui.c | 129 ++++++++++++-------- 1 file changed, 75 insertions(+), 54 deletions(-) diff --git a/gui.c b/gui.c index 2cf30943bb5e..88ea648cdca9 100644 --- a/gui.c +++ b/gui.c @@ -684,8 +684,12 @@ connection_next_clicked (GtkWidget *w, gpointer data) /*----------------------------------------------------------------------*/ /* Conversion dialog. */ +static void populate_disks_store (GtkListStore *disks_store, + const char * const *disks); static void populate_disks (GtkTreeView *disks_list_p, const char * const *disks); +static void populate_removable_store (GtkListStore *removable_store, + const char * const *removable); static void populate_removable (GtkTreeView *removable_list_p, const char * const *removable); static void populate_interfaces (GtkTreeView *interfaces_list_p); @@ -1104,52 +1108,60 @@ repopulate_output_combo (struct config *config) /** * Populate the C<Fixed hard disks> treeview. */ +static void +populate_disks_store (GtkListStore *disks_store, const char * const *disks) +{ + size_t i; + + if (disks == NULL) + return; + + for (i = 0; disks[i] != NULL; ++i) { + uint64_t size; + CLEANUP_FREE char *size_gb = NULL; + CLEANUP_FREE char *model = NULL; + CLEANUP_FREE char *serial = NULL; + CLEANUP_FREE char *device_descr = NULL; + GtkTreeIter iter; + + if (disks[i][0] != '/') { /* not using --test-disk */ + size = get_blockdev_size (disks[i]); + if (asprintf (&size_gb, "%" PRIu64 "G", size) == -1) + error (EXIT_FAILURE, errno, "asprintf"); + model = get_blockdev_model (disks[i]); + serial = get_blockdev_serial (disks[i]); + } + + if (asprintf (&device_descr, + "<b>%s</b>\n" + "<small>" + "%s %s\n" + "%s%s" + "</small>", + disks[i], + size_gb ? size_gb : "", model ? model : "", + serial ? "s/n " : "", serial ? serial : "") == -1) + error (EXIT_FAILURE, errno, "asprintf"); + + gtk_list_store_append (disks_store, &iter); + gtk_list_store_set (disks_store, &iter, + DISKS_COL_CONVERT, TRUE, + DISKS_COL_HW_NAME, disks[i], + DISKS_COL_DEVICE, device_descr, + -1); + } +} + static void populate_disks (GtkTreeView *disks_list_p, const char * const *disks) { GtkListStore *disks_store; GtkCellRenderer *disks_col_convert, *disks_col_device; - GtkTreeIter iter; - size_t i; disks_store = gtk_list_store_new (NUM_DISKS_COLS, G_TYPE_BOOLEAN, G_TYPE_STRING, G_TYPE_STRING); - if (disks != NULL) { - for (i = 0; disks[i] != NULL; ++i) { - uint64_t size; - CLEANUP_FREE char *size_gb = NULL; - CLEANUP_FREE char *model = NULL; - CLEANUP_FREE char *serial = NULL; - CLEANUP_FREE char *device_descr = NULL; - - if (disks[i][0] != '/') { /* not using --test-disk */ - size = get_blockdev_size (disks[i]); - if (asprintf (&size_gb, "%" PRIu64 "G", size) == -1) - error (EXIT_FAILURE, errno, "asprintf"); - model = get_blockdev_model (disks[i]); - serial = get_blockdev_serial (disks[i]); - } - - if (asprintf (&device_descr, - "<b>%s</b>\n" - "<small>" - "%s %s\n" - "%s%s" - "</small>", - disks[i], - size_gb ? size_gb : "", model ? model : "", - serial ? "s/n " : "", serial ? serial : "") == -1) - error (EXIT_FAILURE, errno, "asprintf"); - - gtk_list_store_append (disks_store, &iter); - gtk_list_store_set (disks_store, &iter, - DISKS_COL_CONVERT, TRUE, - DISKS_COL_HW_NAME, disks[i], - DISKS_COL_DEVICE, device_descr, - -1); - } - } + populate_disks_store (disks_store, disks); gtk_tree_view_set_model (disks_list_p, GTK_TREE_MODEL (disks_store)); gtk_tree_view_set_headers_visible (disks_list_p, TRUE); @@ -1177,33 +1189,42 @@ populate_disks (GtkTreeView *disks_list_p, const char * const *disks) /** * Populate the C<Removable media> treeview. */ +static void +populate_removable_store (GtkListStore *removable_store, + const char * const *removable) +{ + size_t i; + + if (removable == NULL) + return; + + for (i = 0; removable[i] != NULL; ++i) { + CLEANUP_FREE char *device_descr = NULL; + GtkTreeIter iter; + + if (asprintf (&device_descr, "<b>%s</b>\n", removable[i]) == -1) + error (EXIT_FAILURE, errno, "asprintf"); + + gtk_list_store_append (removable_store, &iter); + gtk_list_store_set (removable_store, &iter, + REMOVABLE_COL_CONVERT, TRUE, + REMOVABLE_COL_HW_NAME, removable[i], + REMOVABLE_COL_DEVICE, device_descr, + -1); + } +} + static void populate_removable (GtkTreeView *removable_list_p, const char * const *removable) { GtkListStore *removable_store; GtkCellRenderer *removable_col_convert, *removable_col_device; - GtkTreeIter iter; - size_t i; removable_store = gtk_list_store_new (NUM_REMOVABLE_COLS, G_TYPE_BOOLEAN, G_TYPE_STRING, G_TYPE_STRING); - if (removable != NULL) { - for (i = 0; removable[i] != NULL; ++i) { - CLEANUP_FREE char *device_descr = NULL; - - if (asprintf (&device_descr, "<b>%s</b>\n", removable[i]) == -1) - error (EXIT_FAILURE, errno, "asprintf"); - - gtk_list_store_append (removable_store, &iter); - gtk_list_store_set (removable_store, &iter, - REMOVABLE_COL_CONVERT, TRUE, - REMOVABLE_COL_HW_NAME, removable[i], - REMOVABLE_COL_DEVICE, device_descr, - -1); - } - } + populate_removable_store (removable_store, removable); gtk_tree_view_set_model (removable_list_p, GTK_TREE_MODEL (removable_store)); gtk_tree_view_set_headers_visible (removable_list_p, TRUE);

Using the previously extracted / reworked representation of disks and removables, introduce a new button in the conversion dialog that repopulates the "disks" and "removables" tree views with freshly collected entries. In case the user issues such commands in the XTerm window that make more disks visible to the kernel, for example iSCSI LUNs, the button allows virt-p2v to learn about those disks. Disable the new button for when "--test-disk" is passed on the command line; that option conflicts with the new feature. (This means the new button is insensitive in "make run-virt-p2v-directly". It is sensitive in the VM tests though.) Update the manual accordingly. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2124538 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- gui.c | 42 ++++++++++++++++++-- virt-p2v.pod | 21 +++++++--- 2 files changed, 55 insertions(+), 8 deletions(-) diff --git a/gui.c b/gui.c index f7b7ab25adaa..49301d9a985b 100644 --- a/gui.c +++ b/gui.c @@ -701,6 +701,7 @@ static void set_disks_from_ui (struct config *); static void set_removable_from_ui (struct config *); static void set_interfaces_from_ui (struct config *); static void conversion_back_clicked (GtkWidget *w, gpointer data); +static void refresh_disks_clicked (GtkWidget *w, gpointer data); 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); @@ -741,7 +742,7 @@ create_conversion_dialog (struct config *config, const char * const *disks, const char * const *removable) { - GtkWidget *back, *start_button; + GtkWidget *back, *refresh_disks, *start_button; GtkWidget *hbox, *left_vbox, *right_vbox; GtkWidget *target_frame, *target_vbox, *target_tbl; GtkWidget *guestname_label, *vcpus_label, *memory_label; @@ -984,16 +985,27 @@ create_conversion_dialog (struct config *config, /* Buttons. */ gtk_dialog_add_buttons (GTK_DIALOG (conv_dlg), _("_Back"), 1, - _("Start _conversion"), 2, + _("_Refresh disks (will reset selection)"), 2, + _("Start _conversion"), 3, NULL); back = gtk_dialog_get_widget_for_response (GTK_DIALOG (conv_dlg), 1); - start_button = gtk_dialog_get_widget_for_response (GTK_DIALOG (conv_dlg), 2); + refresh_disks = gtk_dialog_get_widget_for_response (GTK_DIALOG (conv_dlg), 2); + start_button = gtk_dialog_get_widget_for_response (GTK_DIALOG (conv_dlg), 3); + + /* Disable disk refreshing in case --test-disk was passed. */ + if (disks != NULL && + disks[0] != NULL && + disks[0][0] == '/' && + disks[1] == NULL) + gtk_widget_set_sensitive (refresh_disks, FALSE); /* Signals. */ g_signal_connect_swapped (G_OBJECT (conv_dlg), "destroy", G_CALLBACK (gtk_main_quit), NULL); g_signal_connect (G_OBJECT (back), "clicked", G_CALLBACK (conversion_back_clicked), NULL); + g_signal_connect (G_OBJECT (refresh_disks), "clicked", + G_CALLBACK (refresh_disks_clicked), NULL); g_signal_connect (G_OBJECT (start_button), "clicked", G_CALLBACK (start_conversion_clicked), config); g_signal_connect (G_OBJECT (vcpu_topo), "toggled", @@ -1600,6 +1612,30 @@ conversion_back_clicked (GtkWidget *w, gpointer data) gtk_widget_set_sensitive (next_button, FALSE); } +static void +refresh_disks_clicked (GtkWidget *w, gpointer data) +{ + GtkTreeModel *model; + GtkListStore *disks_store, *removable_store; + char **disks, **removable; + + model = gtk_tree_view_get_model (GTK_TREE_VIEW (disks_list)); + disks_store = GTK_LIST_STORE (model); + + model = gtk_tree_view_get_model (GTK_TREE_VIEW (removable_list)); + removable_store = GTK_LIST_STORE (model); + + gtk_list_store_clear (disks_store); + gtk_list_store_clear (removable_store); + + find_all_disks (&disks, &removable); + populate_disks_store (disks_store, (const char **)disks); + populate_removable_store (removable_store, (const char **)removable); + + guestfs_int_free_string_list (removable); + guestfs_int_free_string_list (disks); +} + static char *concat_warning (char *warning, const char *fs, ...) __attribute__((format (printf,2,3))); diff --git a/virt-p2v.pod b/virt-p2v.pod index aec5078f92db..6c9033ca250d 100644 --- a/virt-p2v.pod +++ b/virt-p2v.pod @@ -219,6 +219,15 @@ Removable media panel to create corresponding drives on the guest after conversion. Note that any data CDs/DVDs which are mounted in the drives are I<not> copied over. +At the bottom of the dialog, the C<Refresh disks> button instructs +virt-p2v to re-enumerate the fixed hard disks and the removable media +drives. (Note that the button will also reset the currently active +selections in both of those panels.) This button is useful in +combination with the C<XTerm> button on the L</SSH CONFIGURATION +DIALOG>: in the XTerm window, you can expose further block devices to +the kernel (such as LUNs from iSCSI targets), and the C<Refresh disks> +button allows virt-p2v to learn about all the block devices again. + │ Network interfaces │ │ @@ -239,10 +248,10 @@ the interface to be identified by the operator. When you are ready to begin the conversion, press the C<Start conversion> button: - │ - [ Back ] [ Start conversion ] │ - │ - ─ ─ ───────────────────────────────────────┘ + │ + [ Back ] [ Refresh disks ] [ Start conversion ] │ + │ + ─ ─ ──────────────────────────────────────────────────────────┘ =head2 CONVERSION RUNNING DIALOG @@ -509,7 +518,9 @@ features such as the Shutdown popup button. =item B<--test-disk=/PATH/TO/DISK.IMG> For testing or debugging purposes, replace F</dev/sda> with a local -file. You must use an absolute path. +file. You must use an absolute path. Note that the C<Refresh disks> +button will be disabled in the L</DISK AND NETWORK CONFIGURATION DIALOG> +of the GUI. =item B<-v>

On 09/23/22 11:42, Richard W.M. Jones wrote: Will do. I noticed that some of the diagrams were wider than the others, but I didn't notice that such diagrams belonged to different dialogs. Laszlo

On 09/23/22 12:14, Laszlo Ersek wrote: Commit range aa36551e515f..28d7ce8c9db9. I added the following note to patch#14: [lersek(a)redhat.com: refresh the disk/network configuration dialog's width in the manual; also replacing an incorrectly used pipe sign (U+007C, Vertical Line) with U+2502 (Box Drawings Light Vertical) ] And the cumulative difference between the posted version and the merged version of the series is: This update was a bit trickier than I had expected. First, I realized I could compress the extended button row to the left (sticking with the layout seen with the "Fixed hard disks" label; that is, the first button starts where the continuous horizontal line starts). Second, I had to double-check the GUI regarding the "Network interfaces" panel: the network name text entries extend to the very right, so I had to extend those in the manual too, not just move the border to the right. Third, with the extra space available, I had to spell out the full label "Connect to virtual network". Fourth, while looking at the diff closely, I noticed that the original line saying "Connect to ..." used a pipe character (|) for the right border (nowhere else seen in the manual), so I updated that to a U+2502 code point (used everywhere else) at once. ... I kind of wish the manual weren't this extensive; the more docs we have, the more need for updates is "generated" whenever we touch the code somewhere... In effect we have to hand-translate the GTK widget-filling logic to unicode diagrams in the manual. Laszlo