From b893294dcc948d4b41318a400fc4235a190d306b Mon Sep 17 00:00:00 2001 From: "Richard W.M. Jones" Date: Wed, 26 Oct 2011 19:27:01 +0100 Subject: [PATCH] add-domain: Add readonlydisk optional argument (RHBZ#747290). This optional argument controls how 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 | 54 +++++++++++++++++++- regressions/test-add-domain.sh | 23 +++++++-- regressions/test-guestfish-d.sh | 13 +++-- src/virt.c | 108 +++++++++++++++++++++++++++++++++------- 4 files changed, 173 insertions(+), 25 deletions(-) diff --git a/generator/generator_actions.ml b/generator/generator_actions.ml index f10a040..9844b94 100644 --- a/generator/generator_actions.ml +++ b/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 be passed instead of the domain name. The C string is treated as a UUID first and looked up, and if that lookup fails then we treat C as a name as usual. +The optional C parameter controls what we do for +disks which are marked Ereadonly/E in the libvirt XML. +Possible values are: + +=over 4 + +=item readonlydisk = \"error\" + +If C is false: + +The whole call is aborted with an error if any disk with +the Ereadonly/E flag is found. + +If C is true: + +Disks with the Ereadonly/E flag are added read-only. + +=item readonlydisk = \"read\" + +If C is false: + +Disks with the Ereadonly/E flag are added read-only. +Other disks are added read/write. + +If C is true: + +Disks with the Ereadonly/E flag are added read-only. + +=item readonlydisk = \"write\" (default) + +If C is false: + +Disks with the Ereadonly/E flag are added read/write. + +If C is true: + +Disks with the Ereadonly/E flag are added read-only. + +=item readonlydisk = \"ignore\" + +If C is true or false: + +Disks with the Ereadonly/E flag are skipped. + +=back + The other optional parameters are passed directly through to C."); (* 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 for more information. +The optional C parameter controls what we do for +disks which are marked Ereadonly/E in the libvirt XML. +See C for possible values. + The other optional parameters are passed directly through to C."); *) diff --git a/regressions/test-add-domain.sh b/regressions/test-add-domain.sh index ea6d388..7616c38 100755 --- a/regressions/test-add-domain.sh +++ b/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 < test.xml < + + + + + + @@ -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 < test.xml < test.xml < + + + + + + @@ -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 diff --git a/src/virt.c b/src/virt.c index e4bafa2..a95b029 100644 --- a/src/virt.c +++ b/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) { /* 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 in XML */ + readonly = data->readonly; + + if (skip) + return 0; + + if (error) { + error (g, _("%s: disk is marked 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 -- 1.8.3.1