Libguestfs - Oct 2012 - [PATCH for discussion only] launch: Add add_drive 'serial' option.

These two patches are for discussion of the proposed API only (in
fact, it doesn't compile).

virtio-scsi allows you to name drives (using the qemu serial=...
option or libvirt <serial/>).  Let's allow users to specify a serial
when adding a drive, and then map that to a well-known name in the
libguestfs API (/dev/disk/guests/SERIAL[PARTNUM]).

Note that I chose not to change list-devices / list-partitions.  These
still return raw device names (which still exist, even if the serial
is used).  Instead there is a new API called list-serial-names which
returns a mapping of the serial names to raw device names.

This patch is in preparation for hotplugging.  The plan would be to
remove the restriction that add_drive{_opts} has to be called before
launch.  If it's called after launch (ie. hotplugging) then we
strongly recommend that users specify the serial option so the disk
has a predictable name.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://et.redhat.com/~rjones/virt-df/
-------------- next part --------------
>From 6614d4f1d66921bf002067539b42c126d4bbfdb7 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones at redhat.com>
Date: Wed, 3 Oct 2012 11:46:07 +0100
Subject: [PATCH 1/2] build: Use 'tmp-d' as name of temporary directory
 instead of 'tmp'.

When building supermin.d/daemon.img, use 'tmp-d' instead of
'tmp'
as the name of the temporary directory.

This is just code motion.
---
 appliance/Makefile.am | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/appliance/Makefile.am b/appliance/Makefile.am
index fb1f676..6d8b74a 100644
--- a/appliance/Makefile.am
+++ b/appliance/Makefile.am
@@ -76,12 +76,12 @@ stamp-supermin: make.sh packagelist excludelist
 supermin.d/daemon.img: ../daemon/guestfsd guestfsd.suppressions
 	mkdir -p supermin.d
 	rm -f $@ $@-t
-	rm -rf tmp
-	mkdir -p tmp$(DAEMON_SUPERMIN_DIR) tmp/etc
-	ln ../daemon/guestfsd tmp$(DAEMON_SUPERMIN_DIR)/guestfsd
-	ln $(srcdir)/guestfsd.suppressions tmp/etc/guestfsd.suppressions
-	( cd tmp && find | cpio --quiet -o -H newc ) > $@-t
-	rm -rf tmp
+	rm -rf tmp-d
+	mkdir -p tmp-d$(DAEMON_SUPERMIN_DIR) tmp-d/etc
+	ln ../daemon/guestfsd tmp-d$(DAEMON_SUPERMIN_DIR)/guestfsd
+	ln $(srcdir)/guestfsd.suppressions tmp-d/etc/guestfsd.suppressions
+	( cd tmp-d && find | cpio --quiet -o -H newc ) > $@-t
+	rm -rf tmp-d
 	mv $@-t $@
 
 supermin.d/init.img: init
-- 
1.7.11.4

-------------- next part --------------
>From 98d956d57c3d9df12a39c6abe5e639ee3b54f8a0 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones at redhat.com>
Date: Wed, 3 Oct 2012 10:50:51 +0100
Subject: [PATCH 2/2] launch: Add add_drive 'serial' option. New API:
 list-serial-names.

Allow the user to pass an optional serial name when adding a drive.
This is passed through to qemu / libvirt, and from there to the
appliance which exposes it through udev, creating a special alias of
the device /dev/disk/guestfs/<serial>.  Partitions are named
/dev/disk/guestfs/<serial><partnum>.

virtio-blk and virtio-scsi limit the serial name to 20 bytes.  We
further limit the name to maximum 20 alphabetic characters.

list-devices and list-partitions are not changed: these calls still
return raw block device names.  However a new call, list-serial-names,
returns a hash table allowing callers to map between serial names, and
block device and partition names.
---
 appliance/99-guestfs-serial.rules |  8 ++++++++
 appliance/Makefile.am             | 22 ++++++++++++++++++++-
 daemon/devsparts.c                |  6 ++++++
 generator/actions.ml              | 28 ++++++++++++++++++++++++++-
 src/MAX_PROC_NR                   |  2 +-
 src/guestfs-internal.h            |  1 +
 src/guestfs.pod                   | 18 ++++++++++++++++++
 src/launch-appliance.c            |  6 +++++-
 src/launch-libvirt.c              |  6 ++++++
 src/launch.c                      | 40 ++++++++++++++++++++++++++++++++++-----
 10 files changed, 128 insertions(+), 9 deletions(-)
 create mode 100644 appliance/99-guestfs-serial.rules

diff --git a/appliance/99-guestfs-serial.rules
b/appliance/99-guestfs-serial.rules
new file mode 100644
index 0000000..87784f3
--- /dev/null
+++ b/appliance/99-guestfs-serial.rules
@@ -0,0 +1,8 @@
+# For libguestfs, create /dev/disk/guestfs/<serial>
+#
+# As written, it's likely that this only works with virtio-scsi
+# because ID_SCSI_SERIAL is specific to the output of the 'scsi_id'
+# program.
+
+KERNEL=="sd*|vd*", ENV{DEVTYPE}=="disk",
ENV{ID_SCSI_SERIAL}=="?*", \
+  SYMLINK+="disk/guestfs/$env{ID_SCSI_SERIAL}"
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/daemon/devsparts.c b/daemon/devsparts.c
index 8f6c507..74684ae 100644
--- a/daemon/devsparts.c
+++ b/daemon/devsparts.c
@@ -282,3 +282,9 @@ do_nr_devices (void)
 
   return (int) i;
 }
+
+char **
+do_list_serial_names (void)
+{
+  XXX partition names? XXX
+}
diff --git a/generator/actions.ml b/generator/actions.ml
index 4e13855..6595993 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 "serial"];
     once_had_no_optargs = true;
     fish_alias = ["add"]; config_only = true;
     shortdesc = "add an image to examine or modify";
@@ -1237,6 +1237,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<serial>
+
+Give the disk a serial name.  This name should be a unique, short
+string using only alphabetic ASCII characters.
+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<serial>>.
+
+See L<guestfs(3)/SERIAL NAMES>.
+
 =back
 
 C<filename> can have the special value C</dev/null>, which means
@@ -9928,6 +9937,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_serial_names";
+    style = RHashtable "names", [], [];
+    proc_nr = Some 369;
+    tests = [];
+    shortdesc = "mapping of serial names to devices";
+    longdesc = "\
+If you add drives using the optional C<serial> parameter
+of C</guestfs_add_drive_opts>, you can use this call to
+map between serial names, and raw block device and partition
+names (like C</dev/sda> and C</dev/sda1>).
+
+This returns a hashtable, where keys are the serial names
+(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 42bf05d..4b70a33 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -145,6 +145,7 @@ struct drive {
   char *format;
   char *iface;
   char *name;
+  char *serial;
   bool use_cache_none;
 };
 
diff --git a/src/guestfs.pod b/src/guestfs.pod
index 2b33bf3..8b3b915 100644
--- a/src/guestfs.pod
+++ b/src/guestfs.pod
@@ -1191,6 +1191,24 @@ 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 SERIAL NAMES
+
+In libguestfs E<ge> 1.20, you can give a name to a disk when you add
+it, using the optional C<serial> parameter to
L</guestfs_add_drive_opts>.
+
+Not all versions of libguestfs support setting a serial name, and when
+it is supported, it is limited to 20 alphabetic ASCII characters.
+
+When you add a disk with a serial name, it can either be addressed
+using C</dev/sd*>, or using C</dev/disk/guestfs/I<serial>>.
+Partitions on the disk can be addressed using
+C</dev/disk/guestfs/I<serial>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_serial_names> to map serial names
+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 e353e05..3fb6223 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->serial)
+    len += strlen (drv->serial);
 
   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->serial ? ",serial=" : "",
+            drv->serial ? drv->serial : "",
             index,
             iface);
 
diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
index d678266..9b15d3e 100644
--- a/src/launch-libvirt.c
+++ b/src/launch-libvirt.c
@@ -890,6 +890,12 @@ construct_libvirt_xml_disk (guestfs_h *g, xmlTextWriterPtr
xo,
   }
   XMLERROR (-1, xmlTextWriterEndElement (xo));
 
+  if (drv->serial) {
+    XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "serial"));
+    XMLERROR (-1, xmlTextWriterWriteString (xo, BAD_CAST drv->serial));
+    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 a0d6c12..06a79e1 100644
--- a/src/launch.c
+++ b/src/launch.c
@@ -110,10 +110,31 @@ valid_format_iface (const char *str)
   return 1;
 }
 
+/* Check the serial parameter 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_serial (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;
+}
+
 static void
 add_drive (guestfs_h *g, const char *path,
            int readonly, const char *format,
-           const char *iface, const char *name,
+           const char *iface, const char *name, const char *serial,
            int use_cache_none)
 {
   struct drive **drv = &(g->drives);
@@ -128,6 +149,7 @@ add_drive (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)->serial = serial ? safe_strdup (g, serial) : NULL;
   (*drv)->use_cache_none = use_cache_none;
 }
 
@@ -141,7 +163,7 @@ add_drive (guestfs_h *g, const char *path,
  */
 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 *serial)
 {
   char *tmpfile = NULL;
   int fd = -1;
@@ -169,7 +191,7 @@ add_null_drive (guestfs_h *g, int readonly, const char
*format,
     goto err;
   }
 
-  add_drive (g, tmpfile, readonly, format, iface, name, 0);
+  add_drive (g, tmpfile, readonly, format, iface, name, serial, 0);
   free (tmpfile);
 
   return 0;
@@ -189,6 +211,7 @@ guestfs__add_drive_opts (guestfs_h *g, const char *filename,
   const char *format;
   const char *iface;
   const char *name;
+  const char *serial;
   int use_cache_none;
 
   if (strchr (filename, ':') != NULL) {
@@ -205,6 +228,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;
+  serial = optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_SERIAL_BITMASK
+         ? optargs->serial : NULL;
 
   if (format && !valid_format_iface (format)) {
     error (g, _("%s parameter is empty or contains disallowed
characters"),
@@ -216,9 +241,13 @@ guestfs__add_drive_opts (guestfs_h *g, const char
*filename,
            "iface");
     return -1;
   }
+  if (serial && !valid_serial (serial)) {
+    error (g, _("serial 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, serial);
 
   /* For writable files, see if we can use cache=none.  This also
    * checks for the existence of the file.  For readonly we have
@@ -235,7 +264,8 @@ guestfs__add_drive_opts (guestfs_h *g, const char *filename,
     }
   }
 
-  add_drive (g, filename, readonly, format, iface, name, use_cache_none);
+  add_drive (g, filename, readonly, format, iface, name, serial,
+             use_cache_none);
   return 0;
 }
 
-- 
1.7.11.4