Laszlo Ersek
2022-Sep-19 13:34 UTC
[Libguestfs] [p2v PATCH 00/15] recognize block device nodes (such as iSCSI /dev/sdX) added via XTerm
- Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2124538 - Based on: [p2v PATCH 0/4] fix crash in "v2v_version" string lifecycle management https://listman.redhat.com/archives/libguestfs/2022-September/029869.html Message-Id: <20220911142501.18230-1-lersek at redhat.com> The first 13 patches are (mainly) refactorings and (occasionally) fixes for small warts. Patch#14 introduces a "Refresh disks" button, and extends the manual accordingly. Patch#15 adds a new section to the manual ("ACCESSING ISCSI DEVICES") which explains how to set up iSCSI LUNs from the XTerm / shell window, and how to expose the new /dev/sdX block device nodes to the running virt-p2v process with the "Refresh disks" button. Actual conversion of a physical Windows 2022 installation, from an iSCSI OS disk, tested by Vera Wu in <https://bugzilla.redhat.com/show_bug.cgi?id=2124538#c19>. Laszlo Laszlo Ersek (15): set_from_ui_generic(): eliminate small code duplication set_interfaces_from_ui(): open-code the set_from_ui_generic() logic gui.c: consume "all_disks" and "all_removables" only for dialog setup compare(): move to "utils.c", rename to compare_strings() find_all_disks(): extract (plus friends) to new file "disks.c" find_all_disks(): minimize global variable references populate_disks(), populate_removable(): minimize references to globals set_config_defaults(): clean up some warts set_config_defaults(): hoist find_all_disks() call to main() gui.c: remove "all_disks" and "all_removable" global variable references "all_disks", "all_removable": eliminate gui.c: extract GtkListStore-filling loops for "disks" and "removable" create_conversion_dialog(): make "start_button" local create_conversion_dialog(): add button to refresh disks & removables virt-p2v.pod: explain how to bring iSCSI LUNs to the disk selection dialog Makefile.am | 1 + disks.c | 186 +++++++++++ gui.c | 330 ++++++++++++++------ main.c | 226 ++------------ p2v.h | 14 +- utils.c | 8 + virt-p2v.pod | 119 ++++++- 7 files changed, 579 insertions(+), 305 deletions(-) create mode 100644 disks.c
Laszlo Ersek
2022-Sep-19 13:34 UTC
[Libguestfs] [p2v PATCH 01/15] set_from_ui_generic(): eliminate small code duplication
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 at redhat.com> --- 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");
Laszlo Ersek
2022-Sep-19 13:34 UTC
[Libguestfs] [p2v PATCH 02/15] set_interfaces_from_ui(): open-code the set_from_ui_generic() logic
In a subsequent patch, we will modify set_from_ui_generic() in such a way that it will fit the fixed disks and the removable media drives, but no longer the interfaces. Detach set_interfaces_from_ui() from the set_from_ui_generic() function. Copy the set_from_ui_generic() function body, open-code the parameters, also directly use INTERFACES_COL_CONVERT in place of "0 /* CONVERT */". No functional changes. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2124538 Signed-off-by: Laszlo Ersek <lersek at redhat.com> --- gui.c | 34 ++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/gui.c b/gui.c index 561ba64d2a71..c40ed31a8bbc 100644 --- a/gui.c +++ b/gui.c @@ -1448,8 +1448,38 @@ set_removable_from_ui (struct config *config) static void set_interfaces_from_ui (struct config *config) { - set_from_ui_generic (all_interfaces, &config->interfaces, - GTK_TREE_VIEW (interfaces_list)); + GtkTreeModel *model; + GtkTreeIter iter; + gboolean b, v; + size_t i, j; + + guestfs_int_free_string_list (config->interfaces); + if (all_interfaces == NULL) { + config->interfaces = NULL; + return; + } + + model = gtk_tree_view_get_model (GTK_TREE_VIEW (interfaces_list)); + + config->interfaces = malloc ((1 + + guestfs_int_count_strings (all_interfaces)) * + sizeof (char *)); + if (config->interfaces == 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, INTERFACES_COL_CONVERT, &v, -1); + if (v) { + assert (all_interfaces[i] != NULL); + config->interfaces[j++] = strdup (all_interfaces[i]); + } + b = gtk_tree_model_iter_next (model, &iter); + ++i; + } + + config->interfaces[j] = NULL; } static void
Laszlo Ersek
2022-Sep-19 13:34 UTC
[Libguestfs] [p2v PATCH 03/15] gui.c: consume "all_disks" and "all_removables" only for dialog setup
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 at redhat.com> --- 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
Laszlo Ersek
2022-Sep-19 13:35 UTC
[Libguestfs] [p2v PATCH 04/15] compare(): move to "utils.c", rename to compare_strings()
We're going to use compare() from multiple source files, going forward, so move it to "utils.c". While at it, rename it to compare_strings(). No functional changes. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2124538 Signed-off-by: Laszlo Ersek <lersek at redhat.com> --- p2v.h | 1 + main.c | 14 +++----------- utils.c | 8 ++++++++ 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/p2v.h b/p2v.h index 32dc55d94de5..e91c47c36428 100644 --- a/p2v.h +++ b/p2v.h @@ -128,6 +128,7 @@ extern char *get_blockdev_serial (const char *dev); extern char *get_if_addr (const char *if_name); extern char *get_if_vendor (const char *if_name, int truncate); extern void wait_network_online (const struct config *); +extern int compare_strings (const void *vp1, const void *vp2); /* virt-v2v version and features (read from remote). */ extern char *v2v_version; diff --git a/main.c b/main.c index 8a93f9eb402b..ef191e9e34be 100644 --- a/main.c +++ b/main.c @@ -372,14 +372,6 @@ set_config_defaults (struct config *config) config->output.storage = strdup ("/var/tmp"); } -static int -compare (const void *vp1, const void *vp2) -{ - char * const *p1 = (char * const *) vp1; - char * const *p2 = (char * const *) vp2; - return strcmp (*p1, *p2); -} - /** * Get parent device of a partition. * @@ -520,9 +512,9 @@ find_all_disks (void) error (EXIT_FAILURE, errno, "closedir: %s", "/sys/block"); if (all_disks) - qsort (all_disks, nr_disks, sizeof (char *), compare); + qsort (all_disks, nr_disks, sizeof (char *), compare_strings); if (all_removable) - qsort (all_removable, nr_removable, sizeof (char *), compare); + qsort (all_removable, nr_removable, sizeof (char *), compare_strings); } /** @@ -576,5 +568,5 @@ find_all_interfaces (void) error (EXIT_FAILURE, errno, "closedir: %s", "/sys/class/net"); if (all_interfaces) - qsort (all_interfaces, nr_interfaces, sizeof (char *), compare); + qsort (all_interfaces, nr_interfaces, sizeof (char *), compare_strings); } diff --git a/utils.c b/utils.c index 932c1c1dd50f..8915871f591e 100644 --- a/utils.c +++ b/utils.c @@ -253,3 +253,11 @@ wait_network_online (const struct config *config) ignore_value (system (NETWORK_ONLINE_COMMAND)); } + +int +compare_strings (const void *vp1, const void *vp2) +{ + char * const *p1 = (char * const *) vp1; + char * const *p2 = (char * const *) vp2; + return strcmp (*p1, *p2); +}
Laszlo Ersek
2022-Sep-19 13:35 UTC
[Libguestfs] [p2v PATCH 05/15] find_all_disks(): extract (plus friends) to new file "disks.c"
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 at redhat.com> --- 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.
Laszlo Ersek
2022-Sep-19 13:35 UTC
[Libguestfs] [p2v PATCH 06/15] find_all_disks(): minimize global variable references
In find_all_disks(), use two local variables called "ret_disks" and "ret_removable", for most of the logic. Only set "all_disks" and "all_removable" at the end of the function. No observable changes. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2124538 Signed-off-by: Laszlo Ersek <lersek at redhat.com> --- disks.c | 30 +++++++++++--------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/disks.c b/disks.c index 4f94719787ac..e9c92b8c69e8 100644 --- a/disks.c +++ b/disks.c @@ -116,6 +116,7 @@ find_all_disks (void) DIR *dir; struct dirent *d; size_t nr_disks = 0, nr_removable = 0; + char **ret_disks = NULL, **ret_removable = NULL; dev_t root_device = 0; struct stat statbuf; @@ -147,26 +148,26 @@ find_all_disks (void) continue; nr_disks++; - all_disks = realloc (all_disks, sizeof (char *) * (nr_disks + 1)); - if (!all_disks) + ret_disks = realloc (ret_disks, sizeof (char *) * (nr_disks + 1)); + if (!ret_disks) error (EXIT_FAILURE, errno, "realloc"); - all_disks[nr_disks-1] = strdup (d->d_name); + ret_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], '!'); + p = strchr (ret_disks[nr_disks-1], '!'); if (p) *p = '/'; - all_disks[nr_disks] = NULL; + ret_disks[nr_disks] = NULL; } else if (STRPREFIX (d->d_name, "sr")) { nr_removable++; - all_removable = realloc (all_removable, + ret_removable = realloc (ret_removable, sizeof (char *) * (nr_removable + 1)); - if (!all_removable) + if (!ret_removable) error (EXIT_FAILURE, errno, "realloc"); - all_removable[nr_removable-1] = strdup (d->d_name); - all_removable[nr_removable] = NULL; + ret_removable[nr_removable-1] = strdup (d->d_name); + ret_removable[nr_removable] = NULL; } } @@ -178,8 +179,11 @@ find_all_disks (void) 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); + if (ret_disks) + qsort (ret_disks, nr_disks, sizeof (char *), compare_strings); + if (ret_removable) + qsort (ret_removable, nr_removable, sizeof (char *), compare_strings); + + all_disks = ret_disks; + all_removable = ret_removable; }
Laszlo Ersek
2022-Sep-19 13:35 UTC
[Libguestfs] [p2v PATCH 07/15] populate_disks(), populate_removable(): minimize references to globals
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 at redhat.com> --- 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); }
Laszlo Ersek
2022-Sep-19 13:35 UTC
[Libguestfs] [p2v PATCH 08/15] set_config_defaults(): clean up some warts
The statement if (!test_disk) { /* block A */ } else { /* block B */ } is needlessly complex; drop the logical negation in exchange for reordering the branches: if (test_disk) { /* block B */ } else { /* block A */ } While at it, fix a typo in an error message in the same context. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2124538 Signed-off-by: Laszlo Ersek <lersek at redhat.com> --- main.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/main.c b/main.c index a83de71b7c73..48603e5c54b1 100644 --- a/main.c +++ b/main.c @@ -327,20 +327,20 @@ set_config_defaults (struct config *config) get_rtc_config (&config->rtc); /* Find all block devices in the system. */ - if (!test_disk) - find_all_disks (); - else { + if (test_disk) { /* For testing and debugging purposes, you can use * --test-disk=/path/to/disk.img */ all_disks = malloc (2 * sizeof (char *)); if (all_disks == NULL) - error (EXIT_FAILURE, errno, "realloc"); + error (EXIT_FAILURE, errno, "malloc"); all_disks[0] = strdup (test_disk); if (all_disks[0] == NULL) error (EXIT_FAILURE, errno, "strdup"); all_disks[1] = NULL; - } + } else + find_all_disks (); + if (all_disks) config->disks = guestfs_int_copy_string_list (all_disks);
Laszlo Ersek
2022-Sep-19 13:35 UTC
[Libguestfs] [p2v PATCH 09/15] set_config_defaults(): hoist find_all_disks() call to main()
Remove the "all_disks" and "all_removable" global variable references from set_config_defaults(), by hoisting the find_all_disks() call, and the associated test disk setup, to main(), just before main() calls set_config_defaults(). Pass the fixed disk and removable media drive arrays as parameters to set_config_defaults(). While at it, the "test_disk" variable no longer needs to be global; both set and consume it locally to main(), now. No observable changes. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2124538 Signed-off-by: Laszlo Ersek <lersek at redhat.com> --- main.c | 53 +++++++++++--------- 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/main.c b/main.c index 48603e5c54b1..6ebb71ce4314 100644 --- a/main.c +++ b/main.c @@ -46,10 +46,11 @@ char **all_interfaces; int is_iso_environment = 0; int feature_colours_option = 0; int force_colour = 0; -static const char *test_disk = NULL; static void udevadm_settle (void); -static void set_config_defaults (struct config *config); +static void set_config_defaults (struct config *config, + const char * const *disks, + const char * const *removable); static void find_all_interfaces (void); enum { HELP_OPTION = CHAR_MAX + 1 }; @@ -129,6 +130,7 @@ main (int argc, char *argv[]) char **cmdline = NULL; int cmdline_source = 0; struct config *config = new_config (); + const char *test_disk = NULL; setlocale (LC_ALL, ""); bindtextdomain (PACKAGE, LOCALEBASEDIR); @@ -212,7 +214,23 @@ main (int argc, char *argv[]) test_nbd_server (); - set_config_defaults (config); + /* Find all block devices in the system. */ + if (test_disk) { + /* For testing and debugging purposes, you can use + * --test-disk=/path/to/disk.img + */ + all_disks = malloc (2 * sizeof (char *)); + if (all_disks == NULL) + error (EXIT_FAILURE, errno, "malloc"); + all_disks[0] = strdup (test_disk); + if (all_disks[0] == NULL) + error (EXIT_FAILURE, errno, "strdup"); + all_disks[1] = NULL; + } else + find_all_disks (); + + set_config_defaults (config, (const char **)all_disks, + (const char **)all_removable); /* Parse /proc/cmdline (if it exists) or use the --cmdline parameter * to initialize the configuration. This allows defaults to be pass @@ -254,7 +272,9 @@ udevadm_settle (void) } static void -set_config_defaults (struct config *config) +set_config_defaults (struct config *config, + const char * const *disks, + const char * const *removable) { long i; char hostname[257]; @@ -326,27 +346,10 @@ set_config_defaults (struct config *config) get_cpu_config (&config->cpu); get_rtc_config (&config->rtc); - /* Find all block devices in the system. */ - if (test_disk) { - /* For testing and debugging purposes, you can use - * --test-disk=/path/to/disk.img - */ - all_disks = malloc (2 * sizeof (char *)); - if (all_disks == NULL) - error (EXIT_FAILURE, errno, "malloc"); - all_disks[0] = strdup (test_disk); - if (all_disks[0] == NULL) - error (EXIT_FAILURE, errno, "strdup"); - all_disks[1] = NULL; - } else - find_all_disks (); - - if (all_disks) - config->disks = guestfs_int_copy_string_list (all_disks); - - /* Find all removable devices in the system. */ - if (all_removable) - config->removable = guestfs_int_copy_string_list (all_removable); + if (disks) + config->disks = guestfs_int_copy_string_list ((char **)disks); + if (removable) + config->removable = guestfs_int_copy_string_list ((char **)removable); /* Find all network interfaces in the system. */ find_all_interfaces ();
Laszlo Ersek
2022-Sep-19 13:35 UTC
[Libguestfs] [p2v PATCH 10/15] gui.c: remove "all_disks" and "all_removable" global variable references
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 at redhat.com> --- 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);
Laszlo Ersek
2022-Sep-19 13:35 UTC
[Libguestfs] [p2v PATCH 11/15] "all_disks", "all_removable": eliminate
Assignments to "all_disks" and "all_removable" are now centralized in main() -- with a side nod to find_all_disks() --, and read accesses of "all_disks" and "all_removable" are also centralized to main(). Replace these global variables with variables that are local to main(). No observable changes. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2124538 Signed-off-by: Laszlo Ersek <lersek at redhat.com> --- p2v.h | 11 ++-------- disks.c | 13 +++++------ main.c | 23 +++++++++++--------- 3 files changed, 20 insertions(+), 27 deletions(-) diff --git a/p2v.h b/p2v.h index d48b4fcc6611..3a972a474ebf 100644 --- a/p2v.h +++ b/p2v.h @@ -66,15 +66,8 @@ 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); +/* disks.c */ +extern void find_all_disks (char ***disks, char ***removable); /* rtc.c */ extern void get_rtc_config (struct rtc_config *); diff --git a/disks.c b/disks.c index e9c92b8c69e8..4eb006246d84 100644 --- a/disks.c +++ b/disks.c @@ -36,9 +36,6 @@ #include "p2v.h" -char **all_disks; -char **all_removable; - /** * Get parent device of a partition. * @@ -107,11 +104,11 @@ device_contains (const char *dev, dev_t root_device) } /** - * Enumerate all disks in F</sys/block> and add them to the global - * C<all_disks> and C<all_removable> arrays. + * Enumerate all disks in F</sys/block> and return them in the C<disks> and + * C<removable> arrays. */ void -find_all_disks (void) +find_all_disks (char ***disks, char ***removable) { DIR *dir; struct dirent *d; @@ -184,6 +181,6 @@ find_all_disks (void) if (ret_removable) qsort (ret_removable, nr_removable, sizeof (char *), compare_strings); - all_disks = ret_disks; - all_removable = ret_removable; + *disks = ret_disks; + *removable = ret_removable; } diff --git a/main.c b/main.c index 86820dd27e08..b0e86c591093 100644 --- a/main.c +++ b/main.c @@ -131,6 +131,7 @@ main (int argc, char *argv[]) int cmdline_source = 0; struct config *config = new_config (); const char *test_disk = NULL; + char **disks, **removable; setlocale (LC_ALL, ""); bindtextdomain (PACKAGE, LOCALEBASEDIR); @@ -219,18 +220,19 @@ main (int argc, char *argv[]) /* For testing and debugging purposes, you can use * --test-disk=/path/to/disk.img */ - all_disks = malloc (2 * sizeof (char *)); - if (all_disks == NULL) + disks = malloc (2 * sizeof (char *)); + if (disks == NULL) error (EXIT_FAILURE, errno, "malloc"); - all_disks[0] = strdup (test_disk); - if (all_disks[0] == NULL) + disks[0] = strdup (test_disk); + if (disks[0] == NULL) error (EXIT_FAILURE, errno, "strdup"); - all_disks[1] = NULL; + disks[1] = NULL; + + removable = NULL; } else - find_all_disks (); + find_all_disks (&disks, &removable); - set_config_defaults (config, (const char **)all_disks, - (const char **)all_removable); + set_config_defaults (config, (const char **)disks, (const char **)removable); /* Parse /proc/cmdline (if it exists) or use the --cmdline parameter * to initialize the configuration. This allows defaults to be pass @@ -256,11 +258,12 @@ 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, (const char **)all_disks, - (const char **)all_removable); + gui_conversion (config, (const char **)disks, (const char **)removable); } guestfs_int_free_string_list (cmdline); + guestfs_int_free_string_list (removable); + guestfs_int_free_string_list (disks); free_config (config); exit (EXIT_SUCCESS);
Laszlo Ersek
2022-Sep-19 13:35 UTC
[Libguestfs] [p2v PATCH 12/15] gui.c: extract GtkListStore-filling loops for "disks" and "removable"
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 at redhat.com> --- 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);
Laszlo Ersek
2022-Sep-19 13:35 UTC
[Libguestfs] [p2v PATCH 13/15] create_conversion_dialog(): make "start_button" local
The "start_button" variable for the corresponding widget in the Conversion dialog need not be global; it is only used temporarily, while we connect the dialog's "Start" response to start_conversion_clicked(). Make it local. No functional changes. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2124538 Signed-off-by: Laszlo Ersek <lersek at redhat.com> --- gui.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/gui.c b/gui.c index 88ea648cdca9..f7b7ab25adaa 100644 --- a/gui.c +++ b/gui.c @@ -127,8 +127,7 @@ static GtkWidget *conv_dlg, *vcpus_warning, *memory_warning, *target_warning_label, *o_combo, *oc_entry, *os_entry, *of_entry, *oa_combo, *info_label, - *disks_list, *removable_list, *interfaces_list, - *start_button; + *disks_list, *removable_list, *interfaces_list; static int vcpus_entry_when_last_sensitive; /* The running dialog which is displayed when virt-v2v is running. */ @@ -742,7 +741,7 @@ create_conversion_dialog (struct config *config, const char * const *disks, const char * const *removable) { - GtkWidget *back; + GtkWidget *back, *start_button; GtkWidget *hbox, *left_vbox, *right_vbox; GtkWidget *target_frame, *target_vbox, *target_tbl; GtkWidget *guestname_label, *vcpus_label, *memory_label;
Laszlo Ersek
2022-Sep-19 13:35 UTC
[Libguestfs] [p2v PATCH 14/15] create_conversion_dialog(): add button to refresh disks & removables
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 at redhat.com> --- 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>
Laszlo Ersek
2022-Sep-19 13:35 UTC
[Libguestfs] [p2v PATCH 15/15] virt-p2v.pod: explain how to bring iSCSI LUNs to the disk selection dialog
Setting up an iSCSI initiator is not without traps, so explain the basics in a few paragraphs in the manual. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2124538 Signed-off-by: Laszlo Ersek <lersek at redhat.com> --- virt-p2v.pod | 102 +++++++++++++++++++- 1 file changed, 100 insertions(+), 2 deletions(-) diff --git a/virt-p2v.pod b/virt-p2v.pod index 6c9033ca250d..2260c6b44d84 100644 --- a/virt-p2v.pod +++ b/virt-p2v.pod @@ -225,8 +225,9 @@ 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. +the kernel (such as L<LUNs from iSCSI targets|/ACCESSING ISCSI +DEVICES>), and the C<Refresh disks> button allows virt-p2v to learn +about all the block devices again. ? Network interfaces ? @@ -448,6 +449,103 @@ L<virt-p2v-make-disk(1)/ADDING AN SSH IDENTITY> L<virt-p2v-make-kickstart(1)/ADDING AN SSH IDENTITY> +=head1 ACCESSING ISCSI DEVICES + +In case the disk that contains the operating system, or other disks that +you want to convert, are LUNs of remote iSCSI targets, follow the steps +below so that virt-p2v can learn about said disks. Note that this +procedure depends on the use of the GUI. + +The guide below is roughly based on the L<RHEL9 product +documentation|https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/9/html/managing_storage_devices/configuring-an-iscsi-initiator_managing-storage-devices>. + +=over 4 + +=item 1. + +Open a shell in an XTerm window, using the C<XTerm> button of the L</SSH +CONFIGURATION DIALOG>. + +(Note that the XTerm window(s) persist while you advance to further +dialogs in virt-p2v, therefore it's unnecessary to jump back and forth +between virt-p2v dialogs just for entering additional shell commands in +the XTerm window(s).) + +=item 2. + +Using C<vi> or another text editor, set the iSCSI initiator name in +F</etc/iscsi/initiatorname.iscsi>, for example: + + InitiatorName=iqn.1994-05.com.redhat:846e82c634 + +If the file does not exist, create it. (Remember that this file is part +of the virt-p2v Live environment, therefore saving it does not modify +any hard disks.) + +=item 3. + +Configure any further iSCSI initiator details I<completely> that are +required by the iSCSI target that you intend to log in to; that is, +before you issue the first C<iscsiadm> command below. This includes the +CHAP user name and password if the target authenticates the initiator +with CHAP, and the reverse direction CHAP user name and password too, if +you want to ascertain the identity of the target on the initiator as +well (this is called "mutual authentication"). + +Completing the configuration at this stage is important because the +first C<iscsiadm> command will start up the C<iscsid> service, and +configuration changes with that service already running will not (or may +not) take effect until/unless you restart the service using +C<systemctl>. + +=item 4. + +Discover the iSCSI targets offered by the desired host: + + iscsiadm -m discovery -t st -p IP_ADDRESS + +The command should respond with a two-column list of targets. The +symbolic target names are in the right hand side column, for example: + + 10.64.24.179:3260,1 iqn.2006-04.example:444 + +=item 5. + +Picking an appropriate target from the right hand side column of the +previous step's output, log in to the target: + + iscsiadm -m node -T TARGET -l + +This command will inform you whether the login attempt was successful. + +=item 6. + +In case the login succeeds, a scan for LUNs on the iSCSI target will +commence at once. There are two pitfalls here. One, dependent on +network characteristics, the scan may take several (tens of) seconds. +Two, even if the login succeeds, ACLs on the target may I<silently> +prevent the initiator from seeing particular LUNs -- meaning that no new +F</dev/sdX> nodes will appear. This is why it is important to get the +initiator name (and, potentially, CHAP authentication) correct at the +very beginning of this procedure. + +Verify the results of the target scan with the C<dmesg> command, and/or +with + + ls -l /dev/disk/by-path/ip-*-iscsi-*-lun-* + +If these symlinks exist, containing the C<IP_ADDRESS> from step 4 and +the C<TARGET> name from step 5 in their filenames, then the target scan +has successfully found the corresponding LUNs. + +=item 7. + +Once the remote LUNs have been successfully enumerated, click the +C<Refresh disks> button in the L</DISK AND NETWORK CONFIGURATION +DIALOG>. + +=back + =head1 COMMON PROBLEMS =head2 Timeouts