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

[Eric Blake]

On 06/22/2010 04:31 PM, Matthias Bolte wrote:
> 2010/6/9 Eric Blake <eblake at 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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100622/84127952/attachment-0001.sig>