[libvirt] [PATCH 2/2] virsh: introduce change-disk command

[Matthias Bolte]

2010/6/9 Eric Blake <eblake at 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