[libvirt] [PATCH 0/2] virsh: document and improve cdrom-change semantics
http://bugzilla.redhat.com/601143 complained that there was no documentation of the fact that 'attach-disk' on a floppy or cdrom is really way to change the contents of the media in that disk, rather than a request to add a second virtual device. Back in March, we changed things to add a new command to make better sense at the API level, but didn't expose it well in virsh. Eric Blake (2): virsh: document attach-disk better virsh: introduce change-disk command tools/virsh.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++++++- tools/virsh.pod | 24 ++++++++++- 2 files changed, 141 insertions(+), 5 deletions(-)
http://bugzilla.redhat.com/601143, part 1 - document existing behavior. Ever since Mar 2010 (commit ced154cb), the use of 'attach-disk' or 'attach-device' to change cdrom/floppy media has been documented but deprecated, but the replacement to use 'update-device' was not documented. * tools/virsh.c (cmdAttachInterface, cmdAttachDisk): Fix bad error message. * tools/virsh.pod (attach-device, attach-disk): Refer to update-device for cdrom and floppy behavior. (update-device): Add documentation. --- tools/virsh.c | 9 ++++++--- tools/virsh.pod | 13 ++++++++++++- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index d60c27b..fab4b74 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -7262,7 +7262,8 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) } else if (STREQ(type, "bridge")) { typ = 2; } else { - vshError(ctl, _("No support %s in command 'attach-interface'"), type); + vshError(ctl, _("No support for %s in command 'attach-interface'"), + type); goto cleanup; } @@ -7524,14 +7525,16 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (STREQ(driver, "file") || STREQ(driver, "tap")) { isFile = 1; } else if (STRNEQ(driver, "phy")) { - vshError(ctl, _("No support %s in command 'attach-disk'"), driver); + vshError(ctl, _("No support for %s in command 'attach-disk'"), + driver); goto cleanup; } } if (mode) { if (STRNEQ(mode, "readonly") && STRNEQ(mode, "shareable")) { - vshError(ctl, _("No support %s in command 'attach-disk'"), mode); + vshError(ctl, _("No support for %s in command 'attach-disk'"), + mode); goto cleanup; } } diff --git a/tools/virsh.pod b/tools/virsh.pod index b54003c..b195b67 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -525,13 +525,17 @@ format of the device sections to get the most accurate set of accepted values. Attach a device to the domain, using a device definition in an XML file. See the documentation to learn about libvirt XML format for a device. +For cdrom and floppy devices, this command only replaces the media within +the single existing device; consider using B<update-device> for this usage. =item B<attach-disk> I<domain-id> I<source> I<target> optional I<--driver driver> I<--subdriver subdriver> I<--type type> I<--mode mode> Attach a new disk device to the domain. I<source> and I<target> are paths for the files and devices. I<driver> can be I<file>, I<tap> or I<phy> depending on the kind of access. -I<type> can indicate I<cdrom> or I<floppy> as alternative to the disk default. +I<type> can indicate I<cdrom> or I<floppy> as alternative to the disk default, +although this use only replaces the media within the existing virtual cdrom or +floppy device; consider using B<update-device> for this usage instead. I<mode> can specify the two specific mode I<readonly> or I<shareable>. =item B<attach-interface> I<domain-id> I<type> I<source> optional I<--target target> I<--mac mac> I<--script script> @@ -561,6 +565,13 @@ I<type> can be either I<network> to indicate a physical network device or I<brid It is recommended to use the I<mac> option to distinguish between the interfaces if more than one are present on the domain. +=item B<update-device> I<domain-id> I<file> optional I<--persistent> + +Update the characteristics of a device associated with I<domain-id>, +based on the device definition in an XML I<file>. If the I<--persistent> +option is used, the changes will affect the next boot of the domain. +See the documentation to learn about libvirt XML format for a device. + =back =head1 VIRTUAL NETWORK COMMANDS -- 1.7.0.1
2010/6/9 Eric Blake <eblake@redhat.com>:
http://bugzilla.redhat.com/601143, part 1 - document existing behavior. Ever since Mar 2010 (commit ced154cb), the use of 'attach-disk' or 'attach-device' to change cdrom/floppy media has been documented but deprecated, but the replacement to use 'update-device' was not documented.
* tools/virsh.c (cmdAttachInterface, cmdAttachDisk): Fix bad error message. * tools/virsh.pod (attach-device, attach-disk): Refer to update-device for cdrom and floppy behavior. (update-device): Add documentation. --- tools/virsh.c | 9 ++++++--- tools/virsh.pod | 13 ++++++++++++- 2 files changed, 18 insertions(+), 4 deletions(-)
ACK. Matthias
On 06/22/2010 03:30 PM, Matthias Bolte wrote:
2010/6/9 Eric Blake <eblake@redhat.com>:
http://bugzilla.redhat.com/601143, part 1 - document existing behavior. Ever since Mar 2010 (commit ced154cb), the use of 'attach-disk' or 'attach-device' to change cdrom/floppy media has been documented but deprecated, but the replacement to use 'update-device' was not documented.
* tools/virsh.c (cmdAttachInterface, cmdAttachDisk): Fix bad error message. * tools/virsh.pod (attach-device, attach-disk): Refer to update-device for cdrom and floppy behavior. (update-device): Add documentation. --- tools/virsh.c | 9 ++++++--- tools/virsh.pod | 13 ++++++++++++- 2 files changed, 18 insertions(+), 4 deletions(-)
ACK.
Thanks. This part of the patch was non-controversial, so I've pushed it (to make it easier to justify back-porting along with other documentation cleanups). The second half, adding 'virsh change-disk', will just have to wait for a later day when we have more consensus on how best to represent it. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Implement an idea originally requested 22 Mar 2010. * tools/virsh.c (cmdChangeDisk): New command, providing convenience wrapper around update-device. (commands): List it. * tools/virsh.pod (change-disk): Document it. (attach-disk): Update cross-reference. --- tools/virsh.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 13 +++++- 2 files changed, 124 insertions(+), 2 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index fab4b74..ba130bf 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -7646,6 +7646,118 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) } /* + * "change-disk" command + */ +static const vshCmdInfo info_change_disk[] = { + {"help", N_("change disk device")}, + {"desc", N_("Update attributes of existing disk device.")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_change_disk[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"source", VSH_OT_DATA, VSH_OFLAG_REQ, N_("source of disk device")}, + {"target", VSH_OT_DATA, VSH_OFLAG_REQ, N_("target of disk device")}, + {"driver", VSH_OT_STRING, 0, N_("driver of disk device")}, + {"subdriver", VSH_OT_STRING, 0, N_("subdriver of disk device")}, + {"type", VSH_OT_STRING, 0, N_("target device type")}, + {"mode", VSH_OT_STRING, 0, N_("mode of device reading and writing")}, + {"persistent", VSH_OT_BOOL, 0, N_("persist disk attachment")}, + {NULL, 0, 0, NULL} +}; + +static int +cmdChangeDisk(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + char *source, *target, *driver, *subdriver, *type, *mode; + int isFile = 0, ret = FALSE; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *xml; + unsigned int flags; + + if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) + goto cleanup; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + goto cleanup; + + if (!(source = vshCommandOptString(cmd, "source", NULL))) + goto cleanup; + + if (!(target = vshCommandOptString(cmd, "target", NULL))) + goto cleanup; + + driver = vshCommandOptString(cmd, "driver", NULL); + subdriver = vshCommandOptString(cmd, "subdriver", NULL); + type = vshCommandOptString(cmd, "type", NULL); + mode = vshCommandOptString(cmd, "mode", NULL); + + if (driver) { + if (STREQ(driver, "file") || STREQ(driver, "tap")) { + isFile = 1; + } else if (STRNEQ(driver, "phy")) { + vshError(ctl, _("No support for %s in command 'change-disk'"), + driver); + goto cleanup; + } + } + + if (mode) { + if (STRNEQ(mode, "readonly") && STRNEQ(mode, "shareable")) { + vshError(ctl, _("No support for %s in command 'attach-disk'"), + mode); + goto cleanup; + } + } + + /* Make XML of disk */ + virBufferVSprintf(&buf, " <disk type='%s'", isFile ? "file" : "block"); + if (type) + virBufferVSprintf(&buf, " device='%s'", type); + virBufferVSprintf(&buf, ">\n" + " <driver name='%s'", driver ? driver : "phy"); + if (subdriver) + virBufferVSprintf(&buf, " type='%s'", subdriver); + virBufferVSprintf(&buf, "/>\n" + " <source %s='%s'/>\n", isFile ? "file" : "dev", + source); + virBufferVSprintf(&buf, "<target dev='%s'/>\n", target); + if (mode != NULL) + virBufferVSprintf(&buf, " <%s/>\n", mode); + virBufferAddLit(&buf, " </disk>\n"); + + if ((xml = virBufferContentAndReset(&buf)) == NULL) { + vshPrint(ctl, "%s", _("Failed to allocate XML buffer")); + return FALSE; + } + + if (vshCommandOptBool(cmd, "persistent")) { + flags = VIR_DOMAIN_DEVICE_MODIFY_CONFIG; + if (virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; + } else { + flags = VIR_DOMAIN_DEVICE_MODIFY_LIVE; + } + ret = virDomainUpdateDeviceFlags(dom, xml, flags); + VIR_FREE(xml); + + if (ret != 0) { + vshError(ctl, "%s", _("Failed to change disk")); + ret = FALSE; + } else { + vshPrint(ctl, "%s", _("Disk changed successfully\n")); + ret = TRUE; + } + + cleanup: + if (dom) + virDomainFree(dom); + virBufferFreeAndReset(&buf); + return ret; +} + +/* * "detach-disk" command */ static const vshCmdInfo info_detach_disk[] = { @@ -8758,6 +8870,7 @@ static const vshCmdDef commands[] = { #ifndef WIN32 {"cd", cmdCd, opts_cd, info_cd}, #endif + {"change-disk", cmdChangeDisk, opts_change_disk, info_change_disk}, {"connect", cmdConnect, opts_connect, info_connect}, #ifndef WIN32 {"console", cmdConsole, opts_console, info_console}, diff --git a/tools/virsh.pod b/tools/virsh.pod index b195b67..ca6b1c6 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -535,8 +535,8 @@ I<source> and I<target> are paths for the files and devices. I<driver> can be I<file>, I<tap> or I<phy> depending on the kind of access. I<type> can indicate I<cdrom> or I<floppy> as alternative to the disk default, although this use only replaces the media within the existing virtual cdrom or -floppy device; consider using B<update-device> for this usage instead. -I<mode> can specify the two specific mode I<readonly> or I<shareable>. +floppy device; consider using B<change-disk> for this usage instead. +I<mode> can specify the two specific modes I<readonly> or I<shareable>. =item B<attach-interface> I<domain-id> I<type> I<source> optional I<--target target> I<--mac mac> I<--script script> @@ -548,6 +548,15 @@ I<mac> allows to specify the MAC address of the network interface. I<script> allows to specify a path to a script handling a bridge instead of the default one. +=item B<change-disk> I<domain-id> I<source> I<target> optional I<--driver driver> I<--subdriver subdriver> I<--type type> I<--mode mode> + +Change an existing disk device of the domain, such as hot-plugging a disk +or changing the media in a cdrom. +I<source> and I<target> are paths for the files and devices. +I<driver> can be I<file>, I<tap> or I<phy> depending on the kind of access. +I<type> can indicate I<cdrom> or I<floppy> as alternative to the disk default. +I<mode> can specify the two specific modes I<readonly> or I<shareable>. + =item B<detach-device> I<domain-id> I<FILE> Detach a device from the domain, takes the same kind of XML descriptions -- 1.7.0.1
2010/6/9 Eric Blake <eblake@redhat.com>:
Implement an idea originally requested 22 Mar 2010.
* tools/virsh.c (cmdChangeDisk): New command, providing convenience wrapper around update-device. (commands): List it. * tools/virsh.pod (change-disk): Document it. (attach-disk): Update cross-reference. --- tools/virsh.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 13 +++++- 2 files changed, 124 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index fab4b74..ba130bf 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -7646,6 +7646,118 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) }
/* + * "change-disk" command + */ +static const vshCmdInfo info_change_disk[] = { + {"help", N_("change disk device")}, + {"desc", N_("Update attributes of existing disk device.")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_change_disk[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"source", VSH_OT_DATA, VSH_OFLAG_REQ, N_("source of disk device")}, + {"target", VSH_OT_DATA, VSH_OFLAG_REQ, N_("target of disk device")}, + {"driver", VSH_OT_STRING, 0, N_("driver of disk device")}, + {"subdriver", VSH_OT_STRING, 0, N_("subdriver of disk device")}, + {"type", VSH_OT_STRING, 0, N_("target device type")}, + {"mode", VSH_OT_STRING, 0, N_("mode of device reading and writing")}, + {"persistent", VSH_OT_BOOL, 0, N_("persist disk attachment")}, + {NULL, 0, 0, NULL} +};
The difference between command option names and corresponding XML parts should be eliminated, because it's confusing.
+ +static int +cmdChangeDisk(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + char *source, *target, *driver, *subdriver, *type, *mode; + int isFile = 0, ret = FALSE; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *xml; + unsigned int flags; + + if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) + goto cleanup; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + goto cleanup; + + if (!(source = vshCommandOptString(cmd, "source", NULL))) + goto cleanup; + + if (!(target = vshCommandOptString(cmd, "target", NULL))) + goto cleanup; + + driver = vshCommandOptString(cmd, "driver", NULL); + subdriver = vshCommandOptString(cmd, "subdriver", NULL); + type = vshCommandOptString(cmd, "type", NULL); + mode = vshCommandOptString(cmd, "mode", NULL); + + if (driver) { + if (STREQ(driver, "file") || STREQ(driver, "tap")) { + isFile = 1; + } else if (STRNEQ(driver, "phy")) { + vshError(ctl, _("No support for %s in command 'change-disk'"), + driver); + goto cleanup; + } + }
I think this is too strictly modeled after what Xen uses. This forces the user to specify the optional driver option in order to attach a file-based disk. Without the driver option the device gets attached as block device.
+ if (mode) { + if (STRNEQ(mode, "readonly") && STRNEQ(mode, "shareable")) { + vshError(ctl, _("No support for %s in command 'attach-disk'"), + mode); + goto cleanup; + } + } + + /* Make XML of disk */ + virBufferVSprintf(&buf, " <disk type='%s'", isFile ? "file" : "block"); + if (type) + virBufferVSprintf(&buf, " device='%s'", type);
I would rename type to device and use the now free option name type to let the user explicitly specify file or block, instead of inferring this central option from the driver name. Also we can let the diver option just be a free form string once it's not used any longer to infer the disk type.
+ virBufferVSprintf(&buf, ">\n" + " <driver name='%s'", driver ? driver : "phy"); + if (subdriver) + virBufferVSprintf(&buf, " type='%s'", subdriver); + virBufferVSprintf(&buf, "/>\n" + " <source %s='%s'/>\n", isFile ? "file" : "dev", + source); + virBufferVSprintf(&buf, "<target dev='%s'/>\n", target); + if (mode != NULL) + virBufferVSprintf(&buf, " <%s/>\n", mode); + virBufferAddLit(&buf, " </disk>\n");
I suggest the following changes to the set of options an their mapping: domain: domain name, id or uuid (no change) type: type of the source, 'file' or 'block' (new and required, free after renaming the original type option to device) source: source of disk device (no change) target: target of disk device (no change) device: type of the target, 'floppy', 'disk' or 'cdrom' (renamed from type, optional) driver: driver of disk device (no change) subdriver: subdriver of disk device (no change) mode: mode of device reading and writing (no change) persistent: persistent disk attachment (no change) Mapping in some pseudo code: verify type to be "file" or "block" if given verify mode to be "readonly" or "shareable" xml += "<disk type='$(type)'" if device given: xml += " device='$(device)'" xml += ">" if driver given: xml += "<driver name='$(driver)'" if subdriver given: xml += " type='$(subdriver)'" xml += "/>" attr = type == "file" ? "file" : "dev" xml += "<source $(attr)='$(source)'/>" xml += "<target dev='$(target)'/>" if mode given: xml += "<$(mode)/>" xml += "</disk>" Matthias
On 06/22/2010 04:31 PM, Matthias Bolte wrote:
2010/6/9 Eric Blake <eblake@redhat.com>:
Implement an idea originally requested 22 Mar 2010.
+static const vshCmdOptDef opts_change_disk[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"source", VSH_OT_DATA, VSH_OFLAG_REQ, N_("source of disk device")}, + {"target", VSH_OT_DATA, VSH_OFLAG_REQ, N_("target of disk device")}, + {"driver", VSH_OT_STRING, 0, N_("driver of disk device")}, + {"subdriver", VSH_OT_STRING, 0, N_("subdriver of disk device")}, + {"type", VSH_OT_STRING, 0, N_("target device type")}, + {"mode", VSH_OT_STRING, 0, N_("mode of device reading and writing")}, + {"persistent", VSH_OT_BOOL, 0, N_("persist disk attachment")}, + {NULL, 0, 0, NULL} +};
The difference between command option names and corresponding XML parts should be eliminated, because it's confusing.
However, it maps to exactly the same options specified in 'virsh attach-disk', and ultimately this command is replacing the deprecated hack where you would use 'virsh attach-disk' to change a cdrom media in the VM.
+ +static int +cmdChangeDisk(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + char *source, *target, *driver, *subdriver, *type, *mode; + int isFile = 0, ret = FALSE; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *xml; + unsigned int flags; + + if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) + goto cleanup; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + goto cleanup; + + if (!(source = vshCommandOptString(cmd, "source", NULL))) + goto cleanup; + + if (!(target = vshCommandOptString(cmd, "target", NULL))) + goto cleanup; + + driver = vshCommandOptString(cmd, "driver", NULL); + subdriver = vshCommandOptString(cmd, "subdriver", NULL); + type = vshCommandOptString(cmd, "type", NULL); + mode = vshCommandOptString(cmd, "mode", NULL); + + if (driver) { + if (STREQ(driver, "file") || STREQ(driver, "tap")) { + isFile = 1; + } else if (STRNEQ(driver, "phy")) { + vshError(ctl, _("No support for %s in command 'change-disk'"), + driver); + goto cleanup; + } + }
I think this is too strictly modeled after what Xen uses.
This forces the user to specify the optional driver option in order to attach a file-based disk. Without the driver option the device gets attached as block device.
I'm open to improvements - although my first attempt copied attach-disk, I can easily see change-disk being a simpler interface by default (if we're changing the media, the device must already exist; so maybe all we need is enough to get a handle to the existing device, dump its xml, then modify that in place to swap to the new source, rather than creating the new xml from scratch). But if we break the symmetry, we need to make sure we like the layout of whatever command style we use for change-disk, and that it still has the full flexibility (via optional arguments) to expose the full power of virDomainUpdateDeviceFlags without requiring the user to start from xml. Any other ideas on what we want it to look like?
+ /* Make XML of disk */ + virBufferVSprintf(&buf, " <disk type='%s'", isFile ? "file" : "block"); + if (type) + virBufferVSprintf(&buf, " device='%s'", type);
I would rename type to device and use the now free option name type to let the user explicitly specify file or block, instead of inferring this central option from the driver name. Also we can let the diver option just be a free form string once it's not used any longer to infer the disk type.
Does that suggestion also work backwards-compatibly for the attach-disk interface?
I suggest the following changes to the set of options an their mapping:
domain: domain name, id or uuid (no change) type: type of the source, 'file' or 'block' (new and required, free after renaming the original type option to device) source: source of disk device (no change) target: target of disk device (no change) device: type of the target, 'floppy', 'disk' or 'cdrom' (renamed from type, optional) driver: driver of disk device (no change) subdriver: subdriver of disk device (no change) mode: mode of device reading and writing (no change) persistent: persistent disk attachment (no change)
Mapping in some pseudo code:
Looks reasonable as well, but I'll wait for any other comments before I start coding up whatever approach gets the most consensus.
verify type to be "file" or "block" if given verify mode to be "readonly" or "shareable"
xml += "<disk type='$(type)'"
if device given: xml += " device='$(device)'"
xml += ">"
if driver given: xml += "<driver name='$(driver)'"
if subdriver given: xml += " type='$(subdriver)'"
xml += "/>"
attr = type == "file" ? "file" : "dev"
xml += "<source $(attr)='$(source)'/>" xml += "<target dev='$(target)'/>"
if mode given: xml += "<$(mode)/>"
xml += "</disk>"
Matthias
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 06/09/2010 01:19 PM, Eric Blake wrote:
http://bugzilla.redhat.com/601143 complained that there was no documentation of the fact that 'attach-disk' on a floppy or cdrom is really way to change the contents of the media in that disk, rather than a request to add a second virtual device. Back in March, we changed things to add a new command to make better sense at the API level, but didn't expose it well in virsh.
Eric Blake (2): virsh: document attach-disk better virsh: introduce change-disk command
Ping. Awaiting a reviewer for these two: https://www.redhat.com/archives/libvir-list/2010-June/msg00217.html -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Matthias Bolte