[libvirt] [PATCH] Support virDomainAttachDevice and virDomainDetachDevice for disks in UML

[Daniel P. Berrange]

On Thu, Aug 19, 2010 at 02:49:34PM +0200, soren at linux2go.dk wrote:
> From: Soren Hansen <soren at linux2go.dk>
> 
> UML supports hot plugging and unplugging of various devices. This patch
> exposes this functionality for disks.

Nice, I had no idea they supported that.

> Signed-off-by: Soren Hansen <soren at linux2go.dk>
> ---
>  src/uml/uml_driver.c |  223 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 221 insertions(+), 2 deletions(-)
> 
> diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
> index 04493ba..a09bc60 100644
> --- a/src/uml/uml_driver.c
> +++ b/src/uml/uml_driver.c
> @@ -1686,6 +1686,225 @@ cleanup:
>  }
>  
>  
> +static inline void umlShrinkDisks(virDomainDefPtr def, size_t i)
> +{
> +    if (def->ndisks > 1) {
> +        memmove(def->disks + i,
> +                def->disks + i + 1,
> +                sizeof(*def->disks) *
> +                (def->ndisks - (i + 1)));
> +        def->ndisks--;
> +        if (VIR_REALLOC_N(def->disks, def->ndisks) < 0) {
> +            /* ignore, harmless */
> +        }
> +    } else {
> +        VIR_FREE(def->disks);
> +        def->ndisks = 0;
> +    }
> +}

Since this code is already used in the QEMU driver, I think we
should just put it straight into src/conf/domain_conf.c so we
can share it. 'virDomainDiskRemove' is probably a more accurate
name than ShrinkDisks.


> +
> +
> +static int umlDomainAttachUmlDisk(struct uml_driver *driver,
> +                                  virDomainObjPtr vm,
> +                                  virDomainDiskDefPtr disk)
> +{
> +    int i, ret;
> +    char *cmd = NULL;
> +    char *reply;
> +
> +    for (i = 0 ; i < vm->def->ndisks ; i++) {
> +        if (STREQ(vm->def->disks[i]->dst, disk->dst)) {
> +            umlReportError(VIR_ERR_OPERATION_FAILED,
> +                           _("target %s already exists"), disk->dst);
> +            return -1;
> +        }
> +    }
> +
> +    if (!disk->src) {
> +        umlReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("disk source path is missing"));
> +        goto error;
> +    }
> +
> +    if (virAsprintf(&cmd, "config %s=%s", disk->dst, disk->src) < 0) {
> +        virReportOOMError();
> +        return -1;
> +    }
> +
> +    if (umlMonitorCommand(driver, vm, cmd, &reply) < 0)
> +        return -1;

Needs to be a 'goto error' to avoid leaking 'cmd'

> +
> +    if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) {
> +        virReportOOMError();
> +        goto error;
> +    }
> +
> +    if (ret < 0)
> +        goto error;
> +
> +    virDomainDiskInsertPreAlloced(vm->def, disk);
> +
> +    VIR_FREE(cmd);
> +
> +    return 0;
> +
> +error:
> +
> +    VIR_FREE(cmd);
> +
> +    return -1;
> +}

> +    dev = virDomainDeviceDefParse(driver->caps, vm->def, xml,
> +                                  VIR_DOMAIN_XML_INACTIVE);
> +    if (dev == NULL)
> +        goto cleanup;
> +
> +    if (dev->type == VIR_DOMAIN_DEVICE_DISK &&
> +        dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> +        if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_UML)
> +            ret = umlDomainDetachUmlDisk(driver, vm, dev);
> +        else {
> +            umlReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("This type of disk cannot be hot unplugged"));
> +        }
> +    } else {
> +        umlReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       "%s", _("This type of device cannot be hot unplugged"));
> +    }
> +
> +cleanup:
> +    virDomainDeviceDefFree(dev);
> +    if (vm)
> +        virDomainObjUnlock(vm);
> +    umlDriverUnlock(driver);
> +    return ret;
> +}
> +
>  
>  static int umlDomainGetAutostart(virDomainPtr dom,
>                              int *autostart) {
> @@ -1900,9 +2119,9 @@ static virDriver umlDriver = {
>      umlDomainStartWithFlags, /* domainCreateWithFlags */
>      umlDomainDefine, /* domainDefineXML */
>      umlDomainUndefine, /* domainUndefine */
> -    NULL, /* domainAttachDevice */
> +    umlDomainAttachDevice, /* domainAttachDevice */
>      NULL, /* domainAttachDeviceFlags */
> -    NULL, /* domainDetachDevice */
> +    umlDomainDetachDevice, /* domainDetachDevice */
>      NULL, /* domainDetachDeviceFlags */

You should also implement the DeviceFlags variants at
the same time. You can just do a dumb wrapper like QEMU
driver does, for simplicity.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|