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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list