add-domain: Add readonlydisk optional argument (RHBZ#747290).

This optional argument controls how <readonly/> disks in
the libvirt XML are handled.  It can be set to one of:

  "write" (default) - add them R/W
  "read" - add them R/O
  "error" - throw an error if asked to add them R/W
  "ignore" - ignore these disks

I have added limited regression tests for this feature.  However
libvirt's test:/// XML does not allow you to specify that a domain
starts off shut down, so we cannot fully test this.  Instead I tested
it by hand.

generator/generator_actions.ml

@@ -1097,7 +1097,7 @@ not part of the formal API and can be removed or changed at any time.");
 This returns the internal list of drives.  'debug' commands are
 not part of the formal API and can be removed or changed at any time.");
 
-  ("add_domain", (RInt "nrdisks", [String "dom"], [String "libvirturi"; Bool "readonly"; String "iface"; Bool "live"; Bool "allowuuid"]), -1, [FishAlias "domain"],
+  ("add_domain", (RInt "nrdisks", [String "dom"], [String "libvirturi"; Bool "readonly"; String "iface"; Bool "live"; Bool "allowuuid"; String "readonlydisk"]), -1, [FishAlias "domain"],
    [],
    "add the disk(s) from a named libvirt domain",
    "\
@@ -1136,12 +1136,58 @@ I<may> be passed instead of the domain name.  The C<dom> string is
 treated as a UUID first and looked up, and if that lookup fails
 then we treat C<dom> as a name as usual.
 
+The optional C<readonlydisk> parameter controls what we do for
+disks which are marked E<lt>readonly/E<gt> in the libvirt XML.
+Possible values are:
+
+=over 4
+
+=item readonlydisk = \"error\"
+
+If C<readonly> is false:
+
+The whole call is aborted with an error if any disk with
+the E<lt>readonly/E<gt> flag is found.
+
+If C<readonly> is true:
+
+Disks with the E<lt>readonly/E<gt> flag are added read-only.
+
+=item readonlydisk = \"read\"
+
+If C<readonly> is false:
+
+Disks with the E<lt>readonly/E<gt> flag are added read-only.
+Other disks are added read/write.
+
+If C<readonly> is true:
+
+Disks with the E<lt>readonly/E<gt> flag are added read-only.
+
+=item readonlydisk = \"write\" (default)
+
+If C<readonly> is false:
+
+Disks with the E<lt>readonly/E<gt> flag are added read/write.
+
+If C<readonly> is true:
+
+Disks with the E<lt>readonly/E<gt> flag are added read-only.
+
+=item readonlydisk = \"ignore\"
+
+If C<readonly> is true or false:
+
+Disks with the E<lt>readonly/E<gt> flag are skipped.
+
+=back
+
 The other optional parameters are passed directly through to
 C<guestfs_add_drive_opts>.");
 
 (*
 This interface is not quite baked yet. -- RWMJ 2010-11-11
-  ("add_libvirt_dom", (RInt "nrdisks", [Pointer ("virDomainPtr", "dom")], [Bool "readonly"; String "iface"; Bool "live"]), -1, [NotInFish],
+  ("add_libvirt_dom", (RInt "nrdisks", [Pointer ("virDomainPtr", "dom")], [Bool "readonly"; String "iface"; Bool "live"; String "readonlydisk"]), -1, [NotInFish],
    [],
    "add the disk(s) from a libvirt domain",
    "\
@@ -1171,6 +1217,10 @@ XML definition.  The default (if the flag is omitted) is never
 to try.  See L<guestfs(3)/ATTACHING TO RUNNING DAEMONS> for more
 information.
 
+The optional C<readonlydisk> parameter controls what we do for
+disks which are marked E<lt>readonly/E<gt> in the libvirt XML.
+See C<guestfs_add_domain> for possible values.
+
 The other optional parameters are passed directly through to
 C<guestfs_add_drive_opts>.");
 *)

regressions/test-add-domain.sh

@@ -20,11 +20,11 @@
 
 set -e
 
-rm -f test1.img test2.img test3.img test.xml test.out
+rm -f test1.img test2.img test3.img test4.img test.xml test.out
 
 cwd="$(pwd)"
 
-truncate -s 1M test1.img test2.img test3.img
+truncate -s 1M test1.img test2.img test3.img test4.img
 
 # Libvirt test XML, see libvirt.git/examples/xml/test/testnode.xml
 cat > test.xml <<EOF
@@ -51,6 +51,12 @@ cat > test.xml <<EOF
         <source file="$cwd/test3.img"/>
         <target dev="hdc"/>
       </disk>
+      <disk type="file">
+        <driver name="qemu" type="raw"/>
+        <source file="$cwd/test4.img"/>
+        <target dev="hdd"/>
+        <readonly/>
+      </disk>
     </devices>
   </domain>
 </node>
@@ -65,6 +71,16 @@ grep -sq "test1.img.*snapshot=on" test.out
 grep -sq "test2.img.*snapshot=on.*format=raw" test.out
 grep -sq "test3.img.*snapshot=on.*format=qcow2" test.out
 
+# Test readonlydisk = "ignore".
+../fish/guestfish >test.out <<EOF
+  -domain guest libvirturi:test://$cwd/test.xml readonly:true readonlydisk:ignore
+  debug-drives
+EOF
+grep -sq "test1.img" test.out
+grep -sq "test2.img" test.out
+grep -sq "test3.img" test.out
+! grep -sq "test4.img" test.out
+
 # Test atomicity.
 rm test3.img
 
@@ -75,5 +91,6 @@ EOF
 ! grep -sq "test1.img" test.out
 ! grep -sq "test2.img" test.out
 ! grep -sq "test3.img" test.out
+! grep -sq "test4.img" test.out
 
-rm -f test1.img test2.img test3.img test.xml test.out
+rm -f test1.img test2.img test3.img test4.img test.xml test.out

regressions/test-guestfish-d.sh

@@ -20,11 +20,11 @@
 
 set -e
 
-rm -f test1.img test2.img test3.img test.xml test.out
+rm -f test1.img test2.img test3.img test4.img test.xml test.out
 
 cwd="$(pwd)"
 
-truncate -s 1M test1.img test2.img test3.img
+truncate -s 1M test1.img test2.img test3.img test4.img
 
 # Libvirt test XML, see libvirt.git/examples/xml/test/testnode.xml
 cat > test.xml <<EOF
@@ -51,6 +51,12 @@ cat > test.xml <<EOF
         <source file="$cwd/test3.img"/>
         <target dev="hdc"/>
       </disk>
+      <disk type="file">
+        <driver name="qemu" type="raw"/>
+        <source file="$cwd/test4.img"/>
+        <target dev="hdd"/>
+        <readonly/>
+      </disk>
     </devices>
   </domain>
 </node>
@@ -62,5 +68,6 @@ grep -sq "test1.img.*snapshot=on" test.out
 ! grep -sq "test1.img.*format" test.out
 grep -sq "test2.img.*snapshot=on.*format=raw" test.out
 grep -sq "test3.img.*snapshot=on.*format=qcow2" test.out
+grep -sq "test4.img.*snapshot=on.*format=raw" test.out
 
-rm -f test1.img test2.img test3.img test.xml test.out
+rm -f test1.img test2.img test3.img test4.img test.xml test.out

src/virt.c

@@ -67,6 +67,8 @@ struct guestfs___add_libvirt_dom_argv {
   const char *iface;
 #define GUESTFS___ADD_LIBVIRT_DOM_LIVE_BITMASK (UINT64_C(1)<<2)
   int live;
+#define GUESTFS___ADD_LIBVIRT_DOM_READONLYDISK_BITMASK (UINT64_C(1)<<3)
+  const char *readonlydisk;
 };
 
 static int guestfs___add_libvirt_dom (guestfs_h *g, virDomainPtr dom, const struct guestfs___add_libvirt_dom_argv *optargs);
@@ -83,6 +85,7 @@ guestfs__add_domain (guestfs_h *g, const char *domain_name,
   int readonly;
   int live;
   int allowuuid;
+  const char *readonlydisk;
   const char *iface;
   struct guestfs___add_libvirt_dom_argv optargs2 = { .bitmask = 0 };
 
@@ -96,6 +99,8 @@ guestfs__add_domain (guestfs_h *g, const char *domain_name,
          ? optargs->live : 0;
   allowuuid = optargs->bitmask & GUESTFS_ADD_DOMAIN_ALLOWUUID_BITMASK
             ? optargs->allowuuid : 0;
+  readonlydisk = optargs->bitmask & GUESTFS_ADD_DOMAIN_READONLYDISK_BITMASK
+               ? optargs->readonlydisk : NULL;
 
   if (live && readonly) {
     error (g, _("you cannot set both live and readonly flags"));
@@ -144,6 +149,10 @@ guestfs__add_domain (guestfs_h *g, const char *domain_name,
     optargs2.bitmask |= GUESTFS___ADD_LIBVIRT_DOM_LIVE_BITMASK;
     optargs2.live = live;
   }
+  if (readonlydisk) {
+    optargs2.bitmask |= GUESTFS___ADD_LIBVIRT_DOM_READONLYDISK_BITMASK;
+    optargs2.readonlydisk = readonlydisk;
+  }
 
   r = guestfs___add_libvirt_dom (g, dom, &optargs2);
 
@@ -333,9 +342,23 @@ guestfs___for_each_disk (guestfs_h *g,
 
 /* This was proposed as an external API, but it's not quite baked yet. */
 
-static int add_disk (guestfs_h *g, const char *filename, const char *format, int readonly, void *optargs_vp);
+static int add_disk (guestfs_h *g, const char *filename, const char *format, int readonly, void *data);
 static int connect_live (guestfs_h *g, virDomainPtr dom);
 
+enum readonlydisk {
+  readonlydisk_error,
+  readonlydisk_read,
+  readonlydisk_write,
+  readonlydisk_ignore,
+};
+
+struct add_disk_data {
+  int readonly;
+  enum readonlydisk readonlydisk;
+  /* Other args to pass through to add_drive_opts. */
+  struct guestfs_add_drive_opts_argv optargs;
+};
+
 static int
 guestfs___add_libvirt_dom (guestfs_h *g, virDomainPtr dom,
                            const struct guestfs___add_libvirt_dom_argv *optargs)
@@ -345,6 +368,8 @@ guestfs___add_libvirt_dom (guestfs_h *g, virDomainPtr dom,
   int readonly;
   const char *iface;
   int live;
+  /* Default for back-compat reasons: */
+  enum readonlydisk readonlydisk = readonlydisk_write;
 
   readonly =
     optargs->bitmask & GUESTFS___ADD_LIBVIRT_DOM_READONLY_BITMASK
@@ -356,6 +381,21 @@ guestfs___add_libvirt_dom (guestfs_h *g, virDomainPtr dom,
     optargs->bitmask & GUESTFS___ADD_LIBVIRT_DOM_LIVE_BITMASK
     ? optargs->live : 0;
 
+  if ((optargs->bitmask & GUESTFS___ADD_LIBVIRT_DOM_READONLYDISK_BITMASK)) {
+    if (STREQ (optargs->readonlydisk, "error"))
+      readonlydisk = readonlydisk_error;
+    else if (STREQ (optargs->readonlydisk, "read"))
+      readonlydisk = readonlydisk_read;
+    else if (STREQ (optargs->readonlydisk, "write"))
+      readonlydisk = readonlydisk_write;
+    else if (STREQ (optargs->readonlydisk, "ignore"))
+      readonlydisk = readonlydisk_ignore;
+    else {
+      error (g, _("unknown readonlydisk parameter"));
+      return -1;
+    }
+  }
+
   if (live && readonly) {
     error (g, _("you cannot set both live and readonly flags"));
     return -1;
@@ -392,21 +432,20 @@ guestfs___add_libvirt_dom (guestfs_h *g, virDomainPtr dom,
   }
 
   /* Add the disks. */
-  struct guestfs_add_drive_opts_argv optargs2 = { .bitmask = 0 };
-  if (readonly) {
-    optargs2.bitmask |= GUESTFS_ADD_DRIVE_OPTS_READONLY_BITMASK;
-    optargs2.readonly = readonly;
-  }
+  struct add_disk_data data;
+  data.optargs.bitmask = 0;
+  data.readonly = readonly;
+  data.readonlydisk = readonlydisk;
   if (iface) {
-    optargs2.bitmask |= GUESTFS_ADD_DRIVE_OPTS_IFACE_BITMASK;
-    optargs2.iface = iface;
+    data.optargs.bitmask |= GUESTFS_ADD_DRIVE_OPTS_IFACE_BITMASK;
+    data.optargs.iface = iface;
   }
 
   /* Checkpoint the command line around the operation so that either
    * all disks are added or none are added.
    */
   struct drive **cp = guestfs___checkpoint_drives (g);
-  r = guestfs___for_each_disk (g, dom, add_disk, &optargs2);
+  r = guestfs___for_each_disk (g, dom, add_disk, &data);
   if (r == -1)
     guestfs___rollback_drives (g, cp);
 
@@ -415,18 +454,53 @@ guestfs___add_libvirt_dom (guestfs_h *g, virDomainPtr dom,
 
 static int
 add_disk (guestfs_h *g,
-          const char *filename, const char *format, int readonly,
-          void *optargs_vp)
+          const char *filename, const char *format, int readonly_in_xml,
+          void *datavp)
 {
-  struct guestfs_add_drive_opts_argv *optargs = optargs_vp;
+  struct add_disk_data *data = datavp;
+  /* Copy whole struct so we can make local changes: */
+  struct guestfs_add_drive_opts_argv optargs = data->optargs;
+  int readonly, error = 0, skip = 0;
+
+  if (readonly_in_xml) {        /* <readonly/> appears in the XML */
+    if (data->readonly) {       /* asked to add disk read-only */
+      switch (data->readonlydisk) {
+      case readonlydisk_error: readonly = 1; break;
+      case readonlydisk_read: readonly = 1; break;
+      case readonlydisk_write: readonly = 1; break;
+      case readonlydisk_ignore: skip = 1; break;
+      default: abort ();
+      }
+    } else {                    /* asked to add disk for read/write */
+      switch (data->readonlydisk) {
+      case readonlydisk_error: error = 1; break;
+      case readonlydisk_read: readonly = 1; break;
+      case readonlydisk_write: readonly = 0; break;
+      case readonlydisk_ignore: skip = 1; break;
+      default: abort ();
+      }
+    }
+  } else                        /* no <readonly/> in XML */
+    readonly = data->readonly;
+
+  if (skip)
+    return 0;
+
+  if (error) {
+    error (g, _("%s: disk is marked <readonly/> in libvirt XML, and readonlydisk was set to \"error\""),
+           filename);
+    return -1;
+  }
+
+  optargs.bitmask |= GUESTFS_ADD_DRIVE_OPTS_READONLY_BITMASK;
+  optargs.readonly = readonly;
 
   if (format) {
-    optargs->bitmask |= GUESTFS_ADD_DRIVE_OPTS_FORMAT_BITMASK;
-    optargs->format = format;
-  } else
-    optargs->bitmask &= ~GUESTFS_ADD_DRIVE_OPTS_FORMAT_BITMASK;
+    optargs.bitmask |= GUESTFS_ADD_DRIVE_OPTS_FORMAT_BITMASK;
+    optargs.format = format;
+  }
 
-  return guestfs__add_drive_opts (g, filename, optargs);
+  return guestfs__add_drive_opts (g, filename, &optargs);
 }
 
 static int