[libvirt] [PATCH/RFC] qemu/kvm: allow to hot remove scsi/virtio disks

[Daniel Veillard]

On Fri, Oct 17, 2008 at 04:24:52PM +0200, Guido Günther wrote:
> On Fri, Oct 17, 2008 at 02:37:17PM +0200, Daniel Veillard wrote:
> [..snip..] 
> >   Looks fine, i just removed a couple of extra spaces at end of line
> > before commiting :-)
> Thanks a lot for applying this so quickly! Attached is a patch that
> allows for unplugging of disks. To do that I added a token to
> virDomainDiskDef that holds the PCI bus/slot number [1] so we can
> indentify the device on unplug. It's a union since other
> busses/hypervisors might have other requirements. The token is meant as
> an internal handle and will never show up in an XML and should probably
> move up into virDomainDeviceDef. Comments are welcome.

  In principle this looks just fine to me except a couple of stylistic
issues:

[...]
> +++ b/src/domain_conf.h
> @@ -84,6 +84,12 @@ struct _virDomainDiskDef {
>      int type;
>      int device;
>      int bus;
> +    union {
> +        struct {
> +            int bus;
> +            int slot;
> +        } pci;
> +    } token;

 I'm not sure what the point of the token union is. Other adressing
mechanism may be used still I think having the structure not unioned
at this point makes things a bit clearer.

> +static int qemudDomainDetchPciDiskDevice(virDomainPtr dom, virDomainDeviceDefPtr dev)

  Well I don't really see an improvement in using "Detch" instead of
  "Detach", it's not like the identifier is much smaller, makes it less
  readable IMHO, ditto for qemudDomainDetchDevice()

[...]
> +    if (detach->token.pci.slot < 1 || detach->token.pci.bus != 0) {
> +        qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
> +                         _("disk %s cannot be detached"), detach->dst);
> +        return -1;
> +    }

  shouldn't the error be "non PCI disk %s cannot be detached" instead ?

> +    ret = asprintf(&cmd, "pci_del 0 %d", detach->token.pci.slot);
> +    if (ret == -1) {
> +        qemudReportError(dom->conn, NULL, NULL, VIR_ERR_NO_MEMORY, NULL);
> +        return ret;
> +    }
> +
> +    if (qemudMonitorCommand(driver, vm, cmd, &reply) < 0) {
> +        qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
> +                         _("cannot detach disk %s"), detach->dst);

  Live attach/detach operations are among the ones the harder to debug from
an user perspective I would suggest to try to be as explicit as possible
   "failed to execute detach disk %s command"

> +        VIR_FREE(cmd);
> +        return -1;
> +    }
> +
> +    DEBUG ("pci_del reply: %s", reply);
> +    /* If the command fails due to a wrong slot qemu prints: invalid slot, 
> +     * nothing is printed on success */
> +    if (strstr(reply, "invalid slot")) {
> +        qemudReportError (dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
> +                          _("cannot detach disk %s"), detach->dst);


    same thing if we have a hint about why this failed ...
        "cannot detach disk %s : invalid slot"

[...]
> +static int qemudDomainDetchDevice(virDomainPtr dom,
> +                                   const char *xml) {

  Detach :-)


> +    if (dev->type == VIR_DOMAIN_DEVICE_DISK &&
> +        dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK &&
> +        (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI ||
> +         dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO))
> +                    ret = qemudDomainDetchPciDiskDevice(dom, dev);
> +    else {
> +        qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
> +                         "%s", _("this device type cannot be attached"));

 or even better turn this positively as
       "only SCSI or virtio disk device can be attached dynamically"


  Those are just stylistic issues, I can apply the patch with those
  changed if you wish if you don't have time for a new patch,

    thanks,

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/