Linux from around 5.6 now enumerates individual disks in any order (whereas previously it enumerated only drivers in parallel). This means that /dev/sdX ordering is no longer stable - in particular we cannot be sure that /dev/sda inside the guest is the first disk that was attached to the appliance, /dev/sdb the second disk and so on. However we can still use SCSI PCI device numbering as found in /dev/disk/by-path. Use this to translate device names in and out of the appliance. Thanks: Vitaly Kuznetsov, Paolo Bonzini, Dan Berrangé. --- TODO | 13 +++ daemon/daemon.h | 2 + daemon/device-name-translation.c | 195 +++++++++++++++++++++++++++++-- daemon/devsparts.ml | 23 +++- daemon/guestfsd.c | 5 + daemon/stubs-macros.h | 10 +- lib/canonical-name.c | 5 + 7 files changed, 235 insertions(+), 18 deletions(-) diff --git a/TODO b/TODO index 066f633d4..064386ac2 100644 --- a/TODO +++ b/TODO @@ -189,6 +189,19 @@ might be replaced by direct use of the library (if this is easier). But it is very hard to be compatible between RHEL6 and RHEL5 when using the library directly. +Properly fix device name translation +------------------------------------ + +Currently for Device parameters we pass a string name like "/dev/sda" +or "/dev/sdb2" or "/dev/VG/LV" to the daemon. Since Linux broke +device enumeration (RHBZ#1804207) this works less well, requiring +non-trivial translation within the daemon. + +It would be far better if in the generator we mapped "/dev/XXX" to a +proper structure. Device index + partition | LV | MD | ... +This would be then used everywhere inside the daemon and mean we would +no longer need device name translation at all. + Visualization ------------- diff --git a/daemon/daemon.h b/daemon/daemon.h index 170fb2537..24cf8585d 100644 --- a/daemon/daemon.h +++ b/daemon/daemon.h @@ -215,6 +215,8 @@ extern void notify_progress_no_ratelimit (uint64_t position, uint64_t total, con /* device-name-translation.c */ extern char *device_name_translation (const char *device); +extern void device_name_translation_init (void); +extern void device_name_translation_free (void); extern char *reverse_device_name_translation (const char *device); /* stubs.c (auto-generated) */ diff --git a/daemon/device-name-translation.c b/daemon/device-name-translation.c index ed826bbae..30173f5c2 100644 --- a/daemon/device-name-translation.c +++ b/daemon/device-name-translation.c @@ -27,12 +27,125 @@ #include <dirent.h> #include <limits.h> #include <sys/stat.h> +#include <errno.h> +#include <error.h> + +#include "c-ctype.h" #include "daemon.h" +static char **cache; +static size_t cache_size; + +/** + * Cache daemon disk mapping. + * + * When the daemon starts up, populate a cache with the contents + * of /dev/disk/by-path. It's easiest to use C<ls -lv> here + * since the names are sorted awkwardly. + */ +void +device_name_translation_init (void) +{ + const char *by_path = "/dev/disk/by-path"; + CLEANUP_FREE char *out = NULL, *err = NULL; + CLEANUP_FREE_STRING_LIST char **lines = NULL; + size_t i, j; + int r; + + device_name_translation_free (); + + r = command (&out, &err, "ls", "-1v", by_path, NULL); + if (r == -1) + error (EXIT_FAILURE, 0, + "failed to initialize device name translation cache: %s", err); + + lines = split_lines (out); + if (lines == NULL) + error (EXIT_FAILURE, errno, "split_lines"); + + cache_size = guestfs_int_count_strings (lines); + cache = calloc (cache_size, sizeof (char *)); + if (cache == NULL) + error (EXIT_FAILURE, errno, "calloc"); + + /* Delete entries for partitions. (Mark them as NULL in the array + * and skip them below.) + */ + for (i = 0; i < cache_size; ++i) { + if (strstr (lines[i], "-part")) + lines[i] = NULL; + } + + /* Look up each device name. It should be a symlink to /dev/sdX. */ + for (i = j = 0; i < cache_size; ++i) { + if (lines[i] != NULL) { + CLEANUP_FREE char *full; + char *device; + + if (asprintf (&full, "%s/%s", by_path, lines[i]) == -1) + error (EXIT_FAILURE, errno, "asprintf"); + + device = realpath (full, NULL); + if (device == NULL) + error (EXIT_FAILURE, errno, "realpath: %s", full); + + /* Don't add the root device to the cache. */ + if (is_root_device (device)) + continue; + + cache[j++] = device; + } + } + + /* This is the final cache size because we deleted entries above. */ + cache_size = j; +} + +void +device_name_translation_free (void) +{ + size_t i; + + for (i = 0; i < cache_size; ++i) + free (cache[i]); + free (cache); + cache = NULL; + cache_size = 0; +} + /** * Perform device name translation. * + * Libguestfs defines a few standard formats for device names. + * (see also L<guestfs(3)/BLOCK DEVICE NAMING> and + * L<guestfs(3)/guestfs_canonical_device_name>). They are: + * + * =over 4 + * + * =item F</dev/sdX[N]> + * + * =item F</dev/hdX[N]> + * + * =item F</dev/vdX[N]> + * + * These mean the Nth partition on the Xth device. Because + * Linux no longer enumerates devices in the order they are + * passed to qemu, we must translate these by looking up + * the actual device using /dev/disk/by-path/ + * + * =item F</dev/mdX> + * + * =item F</dev/VG/LV> + * + * =item F</dev/mapper/...> + * + * =item F</dev/dm-N> + * + * These are not translated here. + * + * =back + * * It returns a newly allocated string which the caller must free. * * It returns C<NULL> on error. B<Note> it does I<not> call @@ -45,18 +158,59 @@ char * device_name_translation (const char *device) { int fd; - char *ret; + char *ret = NULL; + size_t len; - fd = open (device, O_RDONLY|O_CLOEXEC); + /* /dev/sdX[N] and aliases like /dev/vdX[N]. */ + if (STRPREFIX (device, "/dev/") && + strchr (device+5, '/') == NULL && /* not an LV name */ + device[5] != 'm' && /* not /dev/md - RHBZ#1414682 */ + ((len = strcspn (device+5, "d")) > 0 && len <= 2)) { + ssize_t i; + const char *start; + char dev[16]; + + /* Translate to a disk index in /dev/disk/by-path sorted numerically. */ + start = &device[5+len+1]; + len = strspn (start, "abcdefghijklmnopqrstuvwxyz"); + if (len >= sizeof dev - 1) { + fprintf (stderr, "unparseable device name: %s\n", device); + return NULL; + } + strcpy (dev, start); + dev[len] = '\0'; + + i = guestfs_int_drive_index (dev); + if (i >= 0 && i < (ssize_t) cache_size) { + /* Append the partition name if present. */ + if (asprintf (&ret, "%s%s", cache[i], start+len) == -1) + return NULL; + } + } + + /* If we didn't translate it above, continue with the same name. */ + if (ret == NULL) { + ret = strdup (device); + if (ret == NULL) + return NULL; + } + + /* Now check the device is openable. */ + fd = open (ret, O_RDONLY|O_CLOEXEC); if (fd >= 0) { close (fd); - return strdup (device); + return ret; } - if (errno != ENXIO && errno != ENOENT) + if (errno != ENXIO && errno != ENOENT) { + perror (ret); + free (ret); return NULL; + } - /* If the name begins with "/dev/sd" then try the alternatives. */ + free (ret); + + /* If the original name begins with "/dev/sd" then try the alternatives. */ if (!STRPREFIX (device, "/dev/sd")) return NULL; device += 7; /* device == "a1" etc. */ @@ -97,13 +251,34 @@ device_name_translation (const char *device) char * reverse_device_name_translation (const char *device) { - char *ret; + char *ret = NULL; + size_t i; + + /* Look it up in the cache, and if found return the canonical name. + * If not found return a copy of the original string. + */ + for (i = 0; i < cache_size; ++i) { + const size_t len = strlen (cache[i]); + + if (STREQ (device, cache[i]) || + (STRPREFIX (device, cache[i]) && c_isdigit (device[len]))) { + char drv[16]; + + guestfs_int_drive_name (i, drv); + if (asprintf (&ret, "/dev/sd%s%s", drv, &device[len]) == -1) { + reply_with_perror ("asprintf"); + return NULL; + } + break; + } + } - /* Currently a no-op. */ - ret = strdup (device); if (ret == NULL) { - reply_with_perror ("strdup"); - return NULL; + ret = strdup (device); + if (ret == NULL) { + reply_with_perror ("strdup"); + return NULL; + } } return ret; diff --git a/daemon/devsparts.ml b/daemon/devsparts.ml index 59e66e82e..b0703d3ab 100644 --- a/daemon/devsparts.ml +++ b/daemon/devsparts.ml @@ -23,9 +23,26 @@ open Std_utils open Utils -let map_block_devices ~return_md f = - let devs = Sys.readdir "/sys/block" in - let devs = Array.to_list devs in +let rec map_block_devices ~return_md f = + (* We need to get the devices in translated order. Use the + * same command that we use in device-name-translation.c. + *) + let path = "/dev/disk/by-path" in + let devs = command "ls" ["-1v"; path] in + let devs = String.trimr devs in + let devs = String.nsplit "\n" devs in + (* Delete entries for partitions. *) + let devs = List.filter (fun dev -> String.find dev "-part" = (-1)) devs in + let devs = + List.filter_map ( + fun file -> + let dev = Unix_utils.Realpath.realpath (sprintf "%s/%s" path file) in + (* Ignore non-/dev devices, and return without /dev/ prefix. *) + if String.is_prefix dev "/dev/" then + Some (String.sub dev 5 (String.length dev - 5)) + else + None + ) devs in let devs = List.filter ( fun dev -> String.is_prefix dev "sd" || diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c index 5400adf64..fd87f6520 100644 --- a/daemon/guestfsd.c +++ b/daemon/guestfsd.c @@ -233,6 +233,9 @@ main (int argc, char *argv[]) _umask (0); #endif + /* Initialize device name translations cache. */ + device_name_translation_init (); + /* Connect to virtio-serial channel. */ if (!channel) channel = VIRTIO_SERIAL_CHANNEL; @@ -322,6 +325,8 @@ main (int argc, char *argv[]) /* Enter the main loop, reading and performing actions. */ main_loop (sock); + device_name_translation_free (); + exit (EXIT_SUCCESS); } diff --git a/daemon/stubs-macros.h b/daemon/stubs-macros.h index e8b6c500b..235e79472 100644 --- a/daemon/stubs-macros.h +++ b/daemon/stubs-macros.h @@ -30,11 +30,6 @@ */ #define RESOLVE_DEVICE(path,path_out,is_filein) \ do { \ - if (!is_device_parameter ((path))) { \ - if (is_filein) cancel_receive (); \ - reply_with_error ("%s: %s: expecting a device name", __func__, (path)); \ - return; \ - } \ (path_out) = device_name_translation ((path)); \ if ((path_out) == NULL) { \ const int err = errno; \ @@ -43,6 +38,11 @@ reply_with_perror ("%s: %s", __func__, path); \ return; \ } \ + if (!is_device_parameter ((path_out))) { \ + if (is_filein) cancel_receive (); \ + reply_with_error ("%s: %s: expecting a device name", __func__, (path)); \ + return; \ + } \ } while (0) /* All functions that take a mountable argument must call this macro. diff --git a/lib/canonical-name.c b/lib/canonical-name.c index 42a0fd2a6..efe45c5f1 100644 --- a/lib/canonical-name.c +++ b/lib/canonical-name.c @@ -37,6 +37,11 @@ guestfs_impl_canonical_device_name (guestfs_h *g, const char *device) strchr (device+5, '/') == NULL && /* not an LV name */ device[5] != 'm' && /* not /dev/md - RHBZ#1414682 */ ((len = strcspn (device+5, "d")) > 0 && len <= 2)) { + /* NB! These do not need to be translated by + * device_name_translation. They will be translated if necessary + * when the caller uses them in APIs which go through to the + * daemon. + */ ret = safe_asprintf (g, "/dev/sd%s", &device[5+len+1]); } else if (STRPREFIX (device, "/dev/mapper/") || -- 2.25.0

Appliance device names are not reliable since the kernel no longer enumerates virtio-scsi devices serially. Instead get the UUID of the appliance and pass this as the parameter. Note this requires supermin >= 5.1.18 (from around July 2017). --- docs/guestfs-building.pod | 2 +- lib/appliance-kcmdline.c | 57 ++++++++++++++++++++++++++++++++++----- lib/guestfs-internal.h | 2 +- lib/launch-direct.c | 34 +---------------------- lib/launch-libvirt.c | 19 +++++++------ m4/guestfs-appliance.m4 | 3 ++- 6 files changed, 67 insertions(+), 50 deletions(-) diff --git a/docs/guestfs-building.pod b/docs/guestfs-building.pod index 005626515..8e3da1d32 100644 --- a/docs/guestfs-building.pod +++ b/docs/guestfs-building.pod @@ -83,7 +83,7 @@ I<Required>. I<Required>. The following features must be enabled: C<virtio-pci>, C<virtio-serial>, C<virtio-block>, C<virtio-net>. -=item supermin E<ge> 5.1.0 +=item supermin E<ge> 5.1.18 I<Required>. For alternatives, see L</USING A PREBUILT BINARY APPLIANCE> below. diff --git a/lib/appliance-kcmdline.c b/lib/appliance-kcmdline.c index 8eb4999af..211cc4687 100644 --- a/lib/appliance-kcmdline.c +++ b/lib/appliance-kcmdline.c @@ -26,8 +26,10 @@ #include <stdlib.h> #include <stdbool.h> #include <string.h> +#include <unistd.h> #include "c-ctype.h" +#include "ignore-value.h" #include "guestfs.h" #include "guestfs-internal.h" @@ -54,15 +56,53 @@ #define EARLYPRINTK "earlyprintk=pl011,0x9000000" #endif +COMPILE_REGEXP (re_uuid, "UUID=([-0-9a-f]+)", 0) + +static void +read_uuid (guestfs_h *g, void *retv, const char *line, size_t len) +{ + char **ret = retv; + + *ret = match1 (g, line, re_uuid); +} + +/** + * Given a disk image containing an extX filesystem, return the UUID. + * The L<file(1)> command does the hard work. + */ +static char * +get_root_uuid (guestfs_h *g, const char *appliance) +{ + CLEANUP_CMD_CLOSE struct command *cmd = guestfs_int_new_command (g); + char *ret = NULL; + int r; + + guestfs_int_cmd_add_arg (cmd, "file"); + guestfs_int_cmd_add_arg (cmd, "--"); + guestfs_int_cmd_add_arg (cmd, appliance); + guestfs_int_cmd_set_stdout_callback (cmd, read_uuid, &ret, 0); + r = guestfs_int_cmd_run (cmd); + if (r == -1) { + if (ret) free (ret); + return NULL; + } + if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) { + guestfs_int_external_command_failed (g, r, "file", NULL); + if (ret) free (ret); + return NULL; + } + + return ret; +} + /** * Construct the Linux command line passed to the appliance. This is * used by the C<direct> and C<libvirt> backends, and is simply * located in this file because it's a convenient place for this * common code. * - * The C<appliance_dev> parameter must be the full device name of the - * appliance disk and must have already been adjusted to take into - * account virtio-blk or virtio-scsi; eg C</dev/sdb>. + * The C<appliance> parameter is the filename of the appliance + * (could be NULL) from which we obtain the root UUID. * * The C<flags> parameter can contain the following flags logically * or'd together (or 0): @@ -80,7 +120,8 @@ * be freed by the caller. */ char * -guestfs_int_appliance_command_line (guestfs_h *g, const char *appliance_dev, +guestfs_int_appliance_command_line (guestfs_h *g, + const char *appliance, int flags) { CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (argv); @@ -164,8 +205,12 @@ guestfs_int_appliance_command_line (guestfs_h *g, const char *appliance_dev, guestfs_int_add_string (g, &argv, "8250.nr_uarts=1"); /* Tell supermin about the appliance device. */ - if (appliance_dev) - guestfs_int_add_sprintf (g, &argv, "root=%s", appliance_dev); + if (appliance) { + CLEANUP_FREE char *uuid = get_root_uuid (g, appliance); + if (!uuid) + return NULL; + guestfs_int_add_sprintf (g, &argv, "root=UUID=%s", uuid); + } /* SELinux - deprecated setting, never worked and should not be enabled. */ if (g->selinux) diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h index 8c6affa54..fe761a784 100644 --- a/lib/guestfs-internal.h +++ b/lib/guestfs-internal.h @@ -688,7 +688,7 @@ extern int guestfs_int_build_appliance (guestfs_h *g, char **kernel, char **init const char *guestfs_int_get_cpu_model (int kvm); /* appliance-kcmdline.c */ -extern char *guestfs_int_appliance_command_line (guestfs_h *g, const char *appliance_dev, int flags); +extern char *guestfs_int_appliance_command_line (guestfs_h *g, const char *appliance, int flags); #define APPLIANCE_COMMAND_LINE_IS_TCG 1 /* appliance-uefi.c */ diff --git a/lib/launch-direct.c b/lib/launch-direct.c index 1718041a5..2de57ea32 100644 --- a/lib/launch-direct.c +++ b/lib/launch-direct.c @@ -61,8 +61,6 @@ struct backend_direct_data { char guestfsd_sock[UNIX_PATH_MAX]; /* Path to daemon socket. */ }; -static char *make_appliance_dev (guestfs_h *g); - static char * create_cow_overlay_direct (guestfs_h *g, void *datav, struct drive *drv) { @@ -382,7 +380,6 @@ launch_direct (guestfs_h *g, void *datav, const char *arg) int uefi_flags; CLEANUP_FREE char *kernel = NULL, *initrd = NULL, *appliance = NULL; int has_appliance_drive; - CLEANUP_FREE char *appliance_dev = NULL; uint32_t size; CLEANUP_FREE void *buf = NULL; struct hv_param *hp; @@ -636,8 +633,6 @@ launch_direct (guestfs_h *g, void *datav, const char *arg) append_list ("scsi-hd"); append_list ("drive=appliance"); } end_list (); - - appliance_dev = make_appliance_dev (g); } /* Create the virtio serial bus. */ @@ -697,7 +692,7 @@ launch_direct (guestfs_h *g, void *datav, const char *arg) flags = 0; if (!has_kvm || force_tcg) flags |= APPLIANCE_COMMAND_LINE_IS_TCG; - append = guestfs_int_appliance_command_line (g, appliance_dev, flags); + append = guestfs_int_appliance_command_line (g, appliance, flags); arg ("-append", append); /* Note: custom command line parameters must come last so that @@ -961,33 +956,6 @@ launch_direct (guestfs_h *g, void *datav, const char *arg) return -1; } -/* Calculate the appliance device name. - * - * The easy thing would be to use g->nr_drives (indeed, that's what we - * used to do). However this breaks if some of the drives being added - * use the deprecated 'iface' parameter. To further add confusion, - * the format of the 'iface' parameter has never been defined, but - * given existing usage we can assume it has one of only three values: - * NULL, "ide" or "virtio" (which means virtio-blk). See RHBZ#975797. - */ -static char * -make_appliance_dev (guestfs_h *g) -{ - size_t i, index = 0; - struct drive *drv; - char dev[64] = "/dev/sd"; - - /* Calculate the index of the drive. */ - ITER_DRIVES (g, i, drv) { - if (drv->iface == NULL || STREQ (drv->iface, "ide")) - index++; - } - - guestfs_int_drive_name (index, &dev[7]); - - return safe_strdup (g, dev); /* Caller frees. */ -} - static int shutdown_direct (guestfs_h *g, void *datav, int check_for_errors) { diff --git a/lib/launch-libvirt.c b/lib/launch-libvirt.c index 49786fdd9..cf83657a5 100644 --- a/lib/launch-libvirt.c +++ b/lib/launch-libvirt.c @@ -153,8 +153,9 @@ struct backend_libvirt_data { */ struct libvirt_xml_params { struct backend_libvirt_data *data; - char *kernel; /* paths to kernel and initrd */ + char *kernel; /* paths to kernel, initrd and appliance */ char *initrd; + char *appliance; char *appliance_overlay; /* path to qcow2 overlay backed by appliance */ char appliance_dev[64]; /* appliance device name */ size_t appliance_index; /* index of appliance */ @@ -323,10 +324,10 @@ launch_libvirt (guestfs_h *g, void *datav, const char *libvirt_uri) .data = data, .kernel = NULL, .initrd = NULL, + .appliance = NULL, .appliance_overlay = NULL, }; CLEANUP_FREE xmlChar *xml = NULL; - CLEANUP_FREE char *appliance = NULL; struct sockaddr_un addr; struct drive *drv; size_t i; @@ -486,18 +487,18 @@ launch_libvirt (guestfs_h *g, void *datav, const char *libvirt_uri) debug (g, "build appliance"); if (guestfs_int_build_appliance (g, &params.kernel, &params.initrd, - &appliance) == -1) + &params.appliance) == -1) goto cleanup; guestfs_int_launch_send_progress (g, 3); TRACE0 (launch_build_libvirt_appliance_end); /* Note that appliance can be NULL if using the old-style appliance. */ - if (appliance) { + if (params.appliance) { #ifndef APPLIANCE_FORMAT_AUTO - params.appliance_overlay = make_qcow2_overlay (g, appliance, "raw"); + params.appliance_overlay = make_qcow2_overlay (g, params.appliance, "raw"); #else - params.appliance_overlay = make_qcow2_overlay (g, appliance, NULL); + params.appliance_overlay = make_qcow2_overlay (g, params.appliance, NULL); #endif if (!params.appliance_overlay) goto cleanup; @@ -714,7 +715,7 @@ launch_libvirt (guestfs_h *g, void *datav, const char *libvirt_uri) goto cleanup; } - if (appliance) + if (params.appliance) guestfs_int_add_dummy_appliance_drive (g); TRACE0 (launch_libvirt_end); @@ -726,6 +727,7 @@ launch_libvirt (guestfs_h *g, void *datav, const char *libvirt_uri) free (params.kernel); free (params.initrd); + free (params.appliance); free (params.appliance_overlay); return 0; @@ -751,6 +753,7 @@ launch_libvirt (guestfs_h *g, void *datav, const char *libvirt_uri) free (params.kernel); free (params.initrd); + free (params.appliance); free (params.appliance_overlay); g->state = CONFIG; @@ -1215,7 +1218,7 @@ construct_libvirt_xml_boot (guestfs_h *g, flags = 0; if (!params->data->is_kvm) flags |= APPLIANCE_COMMAND_LINE_IS_TCG; - cmdline = guestfs_int_appliance_command_line (g, params->appliance_dev, flags); + cmdline = guestfs_int_appliance_command_line (g, params->appliance, flags); start_element ("os") { if (params->data->firmware) diff --git a/m4/guestfs-appliance.m4 b/m4/guestfs-appliance.m4 index 827a83357..daed959af 100644 --- a/m4/guestfs-appliance.m4 +++ b/m4/guestfs-appliance.m4 @@ -34,7 +34,8 @@ then you have to --disable-appliance as well, since the appliance contains the daemon inside it.]) fi -dnl Check for supermin >= 5.1.0. +dnl Check for supermin >= 5.1.18. +dnl We don't check it, but it must support the root=UUID=... notation. AC_PATH_PROG([SUPERMIN],[supermin],[no]) dnl Pass supermin --packager-config option. -- 2.25.0