[Libguestfs] [PATCH v4 3/5] launch: Add add_drive 'label' option. New API: list-disk-labels

From: "Richard W.M. Jones" <rjones(a)redhat.com&gt; Allow the user to pass an optional disk label when adding a drive. This is passed through to qemu / libvirt using the disk serial field, and from there to the appliance which exposes it through udev, creating a special alias of the device /dev/disk/guestfs/<label>. Partitions are named /dev/disk/guestfs/<label><partnum>. virtio-blk and virtio-scsi limit the serial field to 20 bytes. We further limit the name to maximum 20 ASCII characters in [a-zA-Z]. list-devices and list-partitions are not changed: these calls still return raw block device names. However a new call, list-disk-labels, returns a hash table allowing callers to map between disk labels, and block device and partition names. This commit also includes a test. --- Makefile.am | 1 + appliance/99-guestfs-serial.rules | 17 ++++++++ appliance/Makefile.am | 22 +++++++++- configure.ac | 1 + daemon/devsparts.c | 76 +++++++++++++++++++++++++++++++++ generator/actions.ml | 28 +++++++++++- src/MAX_PROC_NR | 2 +- src/guestfs-internal.h | 1 + src/guestfs.pod | 20 +++++++++ src/launch-appliance.c | 6 ++- src/launch-libvirt.c | 6 +++ src/launch.c | 42 +++++++++++++++--- tests/disk-labels/Makefile.am | 26 +++++++++++ tests/disk-labels/test-disk-labels.pl | 72 +++++++++++++++++++++++++++++++ 14 files changed, 311 insertions(+), 9 deletions(-) create mode 100644 appliance/99-guestfs-serial.rules create mode 100644 tests/disk-labels/Makefile.am create mode 100755 tests/disk-labels/test-disk-labels.pl diff --git a/Makefile.am b/Makefile.am index 7a0a091..4e476ea 100644 --- a/Makefile.am +++ b/Makefile.am @@ -51,6 +51,7 @@ SUBDIRS += tests/mount-local SUBDIRS += tests/9p SUBDIRS += tests/rsync SUBDIRS += tests/bigdirs +SUBDIRS += tests/disk-labels SUBDIRS += tests/regressions endif diff --git a/appliance/99-guestfs-serial.rules b/appliance/99-guestfs-serial.rules new file mode 100644 index 0000000..2438958 --- /dev/null +++ b/appliance/99-guestfs-serial.rules @@ -0,0 +1,17 @@ +# For libguestfs, create /dev/disk/guestfs/<serial> +# and /dev/disk/guestfs/<serial><partnum> + +KERNEL=="sd*[!0-9]", ENV{DEVTYPE}=="disk", ENV{ID_SCSI_SERIAL}=="?*", \ + SYMLINK+="disk/guestfs/$env{ID_SCSI_SERIAL}" +KERNEL=="sd*", ENV{DEVTYPE}=="partition", ENV{ID_SCSI_SERIAL}=="?*", \ + SYMLINK+="disk/guestfs/$env{ID_SCSI_SERIAL}%n" + +# As written, it's likely the above only works with virtio-scsi +# because ID_SCSI_SERIAL is specific to the output of the 'scsi_id' +# program. The following will not work because ID_SERIAL contains +# some unwanted text. + +#KERNEL=="vd*[!0-9]", ATTRS{serial}=="?*", ENV{ID_SERIAL}="$attr{serial}", \ +# SYMLINK+="disk/guestfs/$env{ID_SERIAL}" +#KERNEL=="vd*[0-9]", ATTRS{serial}=="?*", ENV{ID_SERIAL}="$attr{serial}", \ +# SYMLINK+="disk/guestfs/$env{ID_SERIAL}%n" diff --git a/appliance/Makefile.am b/appliance/Makefile.am index 6d8b74a..8481534 100644 --- a/appliance/Makefile.am +++ b/appliance/Makefile.am @@ -37,7 +37,8 @@ superminfs_DATA = \ supermin.d/base.img \ supermin.d/daemon.img \ supermin.d/init.img \ - supermin.d/hostfiles + supermin.d/hostfiles \ + supermin.d/udev-rules.img # This used to be a configure-generated file (as is update.sh still). # However config.status always touches the destination file, which @@ -91,6 +92,25 @@ supermin.d/init.img: init echo "init" | cpio --quiet -o -H newc > $@-t mv $@-t $@ +# We should put this file in /lib/udev/rules.d, but put it in /etc so +# we don't have to deal with all the UsrMove crap in Fedora. +supermin.d/udev-rules.img: 99-guestfs-serial.rules + mkdir -p supermin.d + rm -f $@ $@-t + rm -rf tmp-u + mkdir -p tmp-u/etc/udev/rules.d + for f in $^; do ln $$f tmp-u/etc/udev/rules.d/$$f; done + ( cd tmp-u && find | cpio --quiet -o -H newc ) > $@-t + rm -rf tmp-u + mv $@-t $@ + +# If installing the daemon, install the udev rules too. + +if INSTALL_DAEMON +udevrulesdir = /lib/udev/rules.d +udevrules_DATA = 99-guestfs-serial.rules +endif + # libguestfs-make-fixed-appliance script and man page. sbin_SCRIPTS = libguestfs-make-fixed-appliance diff --git a/configure.ac b/configure.ac index 7e580b3..0422bcb 100644 --- a/configure.ac +++ b/configure.ac @@ -1376,6 +1376,7 @@ AC_CONFIG_FILES([Makefile tests/charsets/Makefile tests/data/Makefile tests/disks/Makefile + tests/disk-labels/Makefile tests/extra/Makefile tests/guests/Makefile tests/luks/Makefile diff --git a/daemon/devsparts.c b/daemon/devsparts.c index 8f6c507..7e319cb 100644 --- a/daemon/devsparts.c +++ b/daemon/devsparts.c @@ -24,6 +24,7 @@ #include <unistd.h> #include <fcntl.h> #include <dirent.h> +#include <limits.h> #include <sys/stat.h> #include "c-ctype.h" @@ -282,3 +283,78 @@ do_nr_devices (void) return (int) i; } + +#define GUESTFSDIR "/dev/disk/guestfs" + +char ** +do_list_disk_labels (void) +{ + DIR *dir = NULL; + struct dirent *d; + char *path = NULL, *rawdev = NULL; + DECLARE_STRINGSBUF (ret); + + dir = opendir (GUESTFSDIR); + if (!dir) { + reply_with_perror ("opendir: %s", GUESTFSDIR); + return NULL; + } + + errno = 0; + while ((d = readdir (dir)) != NULL) { + if (d->d_name[0] == '.') + continue; + + if (asprintf (&path, "%s/%s", GUESTFSDIR, d->d_name) == -1) { + reply_with_perror ("asprintf"); + free_stringslen (ret.argv, ret.size); + goto error; + } + + rawdev = realpath (path, NULL); + if (rawdev == NULL) { + reply_with_perror ("realpath: %s", path); + free_stringslen (ret.argv, ret.size); + goto error; + } + + free (path); + path = NULL; + + if (add_string (&ret, d->d_name) == -1) + goto error; + + if (add_string_nodup (&ret, rawdev) == -1) + goto error; + rawdev = NULL; /* buffer now owned by the stringsbuf */ + } + + /* Check readdir didn't fail */ + if (errno != 0) { + reply_with_perror ("readdir: %s", GUESTFSDIR); + free_stringslen (ret.argv, ret.size); + goto error; + } + + /* Close the directory handle */ + if (closedir (dir) == -1) { + reply_with_perror ("closedir: %s", GUESTFSDIR); + free_stringslen (ret.argv, ret.size); + dir = NULL; + goto error; + } + + dir = NULL; + + if (end_stringsbuf (&ret) == -1) + goto error; + + return ret.argv; /* caller frees */ + + error: + if (dir) + closedir (dir); + free (path); + free (rawdev); + return NULL; +} diff --git a/generator/actions.ml b/generator/actions.ml index 713c716..25d2809 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -1184,7 +1184,7 @@ not all belong to a single logical operating system { defaults with name = "add_drive"; - style = RErr, [String "filename"], [OBool "readonly"; OString "format"; OString "iface"; OString "name"]; + style = RErr, [String "filename"], [OBool "readonly"; OString "format"; OString "iface"; OString "name"; OString "label"]; once_had_no_optargs = true; fish_alias = ["add"]; config_only = true; shortdesc = "add an image to examine or modify"; @@ -1238,6 +1238,15 @@ The name the drive had in the original guest, e.g. C</dev/sdb>. This is used as a hint to the guest inspection process if it is available. +=item C<label> + +Give the disk a label. The label should be a unique, short +string using I<only> ASCII characters C<[a-zA-Z]>. +As well as its usual name in the API (such as C</dev/sda>), +the drive will also be named C</dev/disk/guestfs/I<label>>. + +See L<guestfs(3)/DISK LABELS>. + =back" }; { defaults with @@ -9925,6 +9934,23 @@ on C<device>. The optional C<blockscount> is the size of the filesystem in blocks. If omitted it defaults to the size of C<device>." (* XXX document optional args properly *) }; + { defaults with + name = "list_disk_labels"; + style = RHashtable "labels", [], []; + proc_nr = Some 369; + tests = []; + shortdesc = "mapping of disk labels to devices"; + longdesc = "\ +If you add drives using the optional C<label> parameter +of C<guestfs_add_drive_opts>, you can use this call to +map between disk labels, and raw block device and partition +names (like C</dev/sda> and C</dev/sda1>). + +This returns a hashtable, where keys are the disk labels +(I<without> the C</dev/disk/guestfs> prefix), and the values +are the full raw block device and partition names +(eg. C</dev/sda> and C</dev/sda1>)." }; + ] (* Non-API meta-commands available only in guestfish. diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR index cb35cf9..446dfcc 100644 --- a/src/MAX_PROC_NR +++ b/src/MAX_PROC_NR @@ -1 +1 @@ -368 +369 diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h index 652cf31..ed9ada4 100644 --- a/src/guestfs-internal.h +++ b/src/guestfs-internal.h @@ -143,6 +143,7 @@ struct drive { char *format; char *iface; char *name; + char *disk_label; bool use_cache_none; void *priv; /* Data used by attach method. */ diff --git a/src/guestfs.pod b/src/guestfs.pod index 2b33bf3..48d810b 100644 --- a/src/guestfs.pod +++ b/src/guestfs.pod @@ -1191,6 +1191,26 @@ L</guestfs_list_devices>, L</guestfs_list_partitions> and similar calls return the true names of the devices and partitions as known to the appliance, but see L</guestfs_canonical_device_name>. +=head3 DISK LABELS + +In libguestfs E<ge> 1.20, you can give a label to a disk when you add +it, using the optional C<label> parameter to L</guestfs_add_drive_opts>. +(Note that disk labels are different from and not related to +filesystem labels). + +Not all versions of libguestfs support setting a disk label, and when +it is supported, it is limited to 20 ASCII characters C<[a-zA-Z]>. + +When you add a disk with a label, it can either be addressed +using C</dev/sd*>, or using C</dev/disk/guestfs/I<label>>. +Partitions on the disk can be addressed using +C</dev/disk/guestfs/I<label>I<partnum>>. + +Listing devices (L</guestfs_list_devices>) and partitions +(L</guestfs_list_partitions>) returns the raw block device name. +However you can use L</guestfs_list_disk_labels> to map disk labels +to raw block device and partition names. + =head3 ALGORITHM FOR BLOCK DEVICE NAME TRANSLATION Usually this translation is transparent. However in some (very rare) diff --git a/src/launch-appliance.c b/src/launch-appliance.c index 531faef..378f121 100644 --- a/src/launch-appliance.c +++ b/src/launch-appliance.c @@ -908,6 +908,8 @@ qemu_drive_param (guestfs_h *g, const struct drive *drv, size_t index) len += strlen (drv->iface); if (drv->format) len += strlen (drv->format); + if (drv->disk_label) + len += strlen (drv->disk_label); r = safe_malloc (g, len); @@ -930,11 +932,13 @@ qemu_drive_param (guestfs_h *g, const struct drive *drv, size_t index) else iface = "virtio"; - snprintf (&r[i], len-i, "%s%s%s%s,id=hd%zu,if=%s", + snprintf (&r[i], len-i, "%s%s%s%s%s%s,id=hd%zu,if=%s", drv->readonly ? ",snapshot=on" : "", drv->use_cache_none ? ",cache=none" : "", drv->format ? ",format=" : "", drv->format ? drv->format : "", + drv->disk_label ? ",serial=" : "", + drv->disk_label ? drv->disk_label : "", index, iface); diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c index d33693e..aa1c39f 100644 --- a/src/launch-libvirt.c +++ b/src/launch-libvirt.c @@ -913,6 +913,12 @@ construct_libvirt_xml_disk (guestfs_h *g, xmlTextWriterPtr xo, } XMLERROR (-1, xmlTextWriterEndElement (xo)); + if (drv->disk_label) { + XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "serial")); + XMLERROR (-1, xmlTextWriterWriteString (xo, BAD_CAST drv->disk_label)); + XMLERROR (-1, xmlTextWriterEndElement (xo)); + } + XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "address")); XMLERROR (-1, xmlTextWriterWriteAttribute (xo, BAD_CAST "type", diff --git a/src/launch.c b/src/launch.c index 6e11111..1b6cf4b 100644 --- a/src/launch.c +++ b/src/launch.c @@ -71,6 +71,7 @@ static struct drive * create_drive_struct (guestfs_h *g, const char *path, int readonly, const char *format, const char *iface, const char *name, + const char *disk_label, int use_cache_none) { struct drive *drv = safe_malloc (g, sizeof (struct drive)); @@ -80,6 +81,7 @@ create_drive_struct (guestfs_h *g, const char *path, drv->format = format ? safe_strdup (g, format) : NULL; drv->iface = iface ? safe_strdup (g, iface) : NULL; drv->name = name ? safe_strdup (g, name) : NULL; + drv->disk_label = disk_label ? safe_strdup (g, disk_label) : NULL; drv->use_cache_none = use_cache_none; drv->priv = drv->free_priv = NULL; @@ -92,7 +94,7 @@ guestfs___add_dummy_appliance_drive (guestfs_h *g) { struct drive *drv; - drv = create_drive_struct (g, "", 0, NULL, NULL, NULL, 0); + drv = create_drive_struct (g, "", 0, NULL, NULL, NULL, NULL, 0); add_drive_to_handle (g, drv); } @@ -119,6 +121,7 @@ free_drive_struct (struct drive *drv) free (drv->format); free (drv->iface); free (drv->name); + free (drv->disk_label); if (drv->priv && drv->free_priv) drv->free_priv (drv->priv); free (drv); @@ -183,6 +186,27 @@ valid_format_iface (const char *str) return 1; } +/* Check the disk label is reasonable. It can't contain certain + * characters, eg. '/', ','. However be stricter here and ensure it's + * just alphabetic and <= 20 characters in length. + */ +static int +valid_disk_label (const char *str) +{ + size_t len = strlen (str); + + if (len == 0 || len > 20) + return 0; + + while (len > 0) { + char c = *str++; + len--; + if (!c_isalpha (c)) + return 0; + } + return 1; +} + /* Traditionally you have been able to use /dev/null as a filename, as * many times as you like. Ancient KVM (RHEL 5) cannot handle adding * /dev/null readonly. qemu 1.2 + virtio-scsi segfaults when you use @@ -193,7 +217,7 @@ valid_format_iface (const char *str) */ static int add_null_drive (guestfs_h *g, int readonly, const char *format, - const char *iface, const char *name) + const char *iface, const char *name, const char *disk_label) { char *tmpfile = NULL; int fd = -1; @@ -227,7 +251,7 @@ add_null_drive (guestfs_h *g, int readonly, const char *format, goto err; } - drv = create_drive_struct (g, tmpfile, readonly, format, iface, name, 0); + drv = create_drive_struct (g, tmpfile, readonly, format, iface, name, disk_label, 0); add_drive_to_handle (g, drv); free (tmpfile); @@ -248,6 +272,7 @@ guestfs__add_drive_opts (guestfs_h *g, const char *filename, const char *format; const char *iface; const char *name; + const char *disk_label; int use_cache_none; struct drive *drv; @@ -265,6 +290,8 @@ guestfs__add_drive_opts (guestfs_h *g, const char *filename, ? optargs->iface : NULL; name = optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_NAME_BITMASK ? optargs->name : NULL; + disk_label = optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_LABEL_BITMASK + ? optargs->label : NULL; if (format && !valid_format_iface (format)) { error (g, _("%s parameter is empty or contains disallowed characters"), @@ -276,9 +303,13 @@ guestfs__add_drive_opts (guestfs_h *g, const char *filename, "iface"); return -1; } + if (disk_label && !valid_disk_label (disk_label)) { + error (g, _("label parameter is empty, too long, or contains disallowed characters")); + return -1; + } if (STREQ (filename, "/dev/null")) - return add_null_drive (g, readonly, format, iface, name); + return add_null_drive (g, readonly, format, iface, name, disk_label); /* For writable files, see if we can use cache=none. This also * checks for the existence of the file. For readonly we have @@ -295,7 +326,8 @@ guestfs__add_drive_opts (guestfs_h *g, const char *filename, } } - drv = create_drive_struct (g, filename, readonly, format, iface, name, use_cache_none); + drv = create_drive_struct (g, filename, readonly, format, iface, name, disk_label, + use_cache_none); add_drive_to_handle (g, drv); return 0; } diff --git a/tests/disk-labels/Makefile.am b/tests/disk-labels/Makefile.am new file mode 100644 index 0000000..cd8f0e7 --- /dev/null +++ b/tests/disk-labels/Makefile.am @@ -0,0 +1,26 @@ +# libguestfs +# Copyright (C) 2012 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, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +include $(top_srcdir)/subdir-rules.mk + +TESTS = \ + test-disk-labels.pl + +TESTS_ENVIRONMENT = $(top_builddir)/run --test + +EXTRA_DIST = \ + $(TESTS) diff --git a/tests/disk-labels/test-disk-labels.pl b/tests/disk-labels/test-disk-labels.pl new file mode 100755 index 0000000..137adac --- /dev/null +++ b/tests/disk-labels/test-disk-labels.pl @@ -0,0 +1,72 @@ +#!/usr/bin/perl +# Copyright (C) 2012 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, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +# Test using the 'label' option of add_drive, and the +# list_disk_labels call. + +use strict; +use warnings; + +use Sys::Guestfs; + +my $g = Sys::Guestfs->new (); + +# Add two drives. +foreach (["test1.img", "a"], ["test2.img", "b"]) { + my ($output, $label) = @$_; + open FILE, ">$output" or die "$output: $!"; + truncate FILE, 512 * 1024 * 1024 or die "$output: truncate: $!"; + close FILE or die "$output: $!"; + $g->add_drive ($output, readonly => 0, format => "raw", label => $label); +} + +$g->launch (); + +# Partition the drives. +$g->part_disk ("/dev/disk/guestfs/a", "mbr"); +$g->part_init ("/dev/disk/guestfs/b", "mbr"); +$g->part_add ("/dev/disk/guestfs/b", "p", 64, 100 * 1024 * 2 - 1); +$g->part_add ("/dev/disk/guestfs/b", "p", 100 * 1024 * 2, -64); + +# Check the partitions exist using both the disk label and raw name. +die unless + $g->blockdev_getsize64 ("/dev/disk/guestfs/a1") == + $g->blockdev_getsize64 ("/dev/sda1"); +die unless + $g->blockdev_getsize64 ("/dev/disk/guestfs/b1") == + $g->blockdev_getsize64 ("/dev/sdb1"); +die unless + $g->blockdev_getsize64 ("/dev/disk/guestfs/b2") == + $g->blockdev_getsize64 ("/dev/sdb2"); + +# Check list_disk_labels +my %labels = $g->list_disk_labels (); +die unless exists $labels{"a"}; +die unless $labels{"a"} eq "/dev/sda"; +die unless exists $labels{"b"}; +die unless $labels{"b"} eq "/dev/sdb"; +die unless exists $labels{"a1"}; +die unless $labels{"a1"} eq "/dev/sda1"; +die unless exists $labels{"b1"}; +die unless $labels{"b1"} eq "/dev/sdb1"; +die unless exists $labels{"b2"}; +die unless $labels{"b2"} eq "/dev/sdb2"; + +unlink "test1.img"; +unlink "test2.img"; + +exit 0 -- 1.7.10.4