[libvirt] [PATCH 11/34] Properly support SCSI drive hotplug

[Daniel Veillard]

On Fri, Jan 08, 2010 at 05:23:07PM +0000, Daniel P. Berrange wrote:
> The current SCSI hotplug support attaches a brand new SCSI controller
> for every disk. This is broken because the semantics differ from those
> used when starting the VM initially. In the latter case, each SCSI
> controller is filled before a new one is added.
> 
> If the user specifies an high drive index (sdazz) then at initial
> startup, many intermediate SCSI controllers may be added with no
> drives.

  Argh, okay :-)

> This patch changes SCSI hotplug so that it exactly matches the
> behaviour of initial startup. First the SCSI controller number is
> determined for the drive to be hotplugged. If any controller upto
> and including that controller number is not yet present, it is
> attached. Then finally the drive is attached to the last controller.
> 
> NB, this breaks SCSI hotunplug, because there is no 'drive_del'
> command in current QEMU. Previous SCSI hotunplug was broken in
> any case because it was unplugging the entire controller, not
> just the drive in question.
> 
> A future QEMU will allow proper SCSI hotunplug of a drive.
> 
> This patch is derived from work done by Wolfgang Mauerer on disk
> controllers.
> 
> * src/qemu/qemu_driver.c: Fix SCSI hotplug to add a drive to
>  the correct controller, instead of just attaching a new
>   controller.
> * src/qemu/qemu_monitor.c, src/qemu/qemu_monitor.h,
>   src/qemu/qemu_monitor_json.c, src/qemu/qemu_monitor_json.h,
>   src/qemu/qemu_monitor_text.c, src/qemu/qemu_monitor_text.h: Add
>   support for 'drive_add' command
> ---
>  src/libvirt_private.syms     |    1 +
>  src/qemu/qemu_driver.c       |  123 +++++++++++++++++++++++++++++++++++++++--
>  src/qemu/qemu_monitor.c      |   20 +++++++
>  src/qemu/qemu_monitor.h      |    5 ++
>  src/qemu/qemu_monitor_json.c |   81 +++++++++++++++++++++++++---
>  src/qemu/qemu_monitor_json.h |    5 ++
>  src/qemu/qemu_monitor_text.c |  102 ++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_monitor_text.h |    5 ++
>  8 files changed, 329 insertions(+), 13 deletions(-)
[...]
> +static virDomainControllerDefPtr
> +qemuDomainFindOrCreateSCSIDiskController(virConnectPtr conn,
> +                                         struct qemud_driver *driver,
> +                                         virDomainObjPtr vm,
> +                                         int controller)
> +{
> +    int i;
> +    virDomainControllerDefPtr cont;
> +    for (i = 0 ; i < vm->def->ncontrollers ; i++) {
> +        cont = vm->def->controllers[i];
> +
> +        if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
> +            continue;
> +
> +        if (cont->idx == controller)
> +            return cont;
> +    }
> +
> +    /* No SCSI controller present, for back compatability we
> +     * now hotplug a controller */
> +    if (VIR_ALLOC(cont) < 0) {
> +        virReportOOMError(conn);
> +        return NULL;
> +    }
> +    cont->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI;
> +    cont->idx = 0;
> +
> +    VIR_INFO0("No SCSI controller present, hotplugging one");
> +    if (qemudDomainAttachPciControllerDevice(conn, driver,
> +                                             vm, cont) < 0) {
> +        VIR_FREE(cont);
> +        return NULL;
> +    }
> +    return cont;
> +}

  cosmetic change, formatting the comment and blank line after
argument declaration, if you can sneak it in

> @@ -1515,7 +1514,75 @@ int qemuMonitorJSONAttachPCIDiskController(qemuMonitorPtr mon,
>          ret = qemuMonitorJSONCheckError(cmd, reply);
>  
>      if (ret == 0 &&
> -        qemuMonitorJSONGetGuestAddress(reply, guestAddr) < 0)
> +        qemuMonitorJSONGetGuestPCIAddress(reply, guestAddr) < 0)
> +        ret = -1;
> +
> +    virJSONValueFree(cmd);
> +    virJSONValueFree(reply);
> +    return ret;
> +}

  Hum, looks like a leak plug too here, or I got confused by the patch


> +    if (ret == 0 &&
> +        qemuMonitorJSONGetGuestDriveAddress(reply, driveAddr) < 0)
>          ret = -1;
>  
>      virJSONValueFree(cmd);

  okay probably a patch side effect


> +static int
> +qemudParseDriveAddReply(const char *reply,
> +                        virDomainDeviceDriveAddressPtr addr)
> +{
> +    char *s, *e;
> +
> +    /* If the command succeeds qemu prints:
> +     * OK bus X, unit Y
> +     */
> +
> +    if (!(s = strstr(reply, "OK ")))
> +        return -1;
> +
> +    s += 3;

  I would rather search for "bus " in the string here

> +    if (STRPREFIX(s, "bus ")) {
> +        s += strlen("bus ");
> +
> +        if (virStrToLong_ui(s, &e, 10, &addr->bus) == -1) {
> +            VIR_WARN(_("Unable to parse bus '%s'\n"), s);
> +            return -1;
> +        }
> +
> +        if (!STRPREFIX(e, ", ")) {
> +            VIR_WARN(_("Expected ', ' parsing drive_add reply '%s'\n"), s);
> +            return -1;
> +        }
> +        s = e + 2;
> +    }

  and then search for "unit " for inceased flexibility

> +    if (!STRPREFIX(s, "unit ")) {
> +        VIR_WARN(_("Expected 'unit ' parsing drive_add reply '%s'\n"), s);
> +        return -1;
> +    }
> +    s += strlen("bus ");
> +
> +    if (virStrToLong_ui(s, &e, 10, &addr->unit) == -1) {
> +        VIR_WARN(_("Unable to parse unit number '%s'\n"), s);
> +        return -1;
> +    }
> +
> +    return 0;
> +}

  ACK,

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/