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

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. 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 | 145 +++++++++++++++++++++++++++++++++++++---- 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, 342 insertions(+), 22 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3e98800..963f9b2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -167,6 +167,7 @@ virDomainDeviceAddressTypeToString; virDomainDefClearDynamicValues; virDomainControllerTypeToString; virDomainControllerDefFree; +virDomainDeviceAddressTypeToString; # domain_event.h diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0a91c2a..527b5d7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4964,18 +4964,18 @@ static int qemudDomainAttachPciDiskDevice(virConnectPtr conn, static int qemudDomainAttachPciControllerDevice(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev) + virDomainControllerDefPtr dev) { int i, ret; - const char* type = virDomainControllerTypeToString(dev->data.controller->type); + const char* type = virDomainControllerTypeToString(dev->type); qemuDomainObjPrivatePtr priv = vm->privateData; for (i = 0 ; i < vm->def->ncontrollers ; i++) { - if ((vm->def->controllers[i]->type == dev->data.controller->type) && - (vm->def->controllers[i]->idx == dev->data.controller->idx)) { + if ((vm->def->controllers[i]->type == dev->type) && + (vm->def->controllers[i]->idx == dev->idx)) { qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, _("target %s:%d already exists"), - type, dev->data.controller->idx); + type, dev->idx); return -1; } } @@ -4988,17 +4988,130 @@ static int qemudDomainAttachPciControllerDevice(virConnectPtr conn, qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorAttachPCIDiskController(priv->mon, type, - &dev->data.controller->addr.data.pci); + &dev->addr.data.pci); qemuDomainObjExitMonitorWithDriver(driver, vm); if (ret == 0) { - dev->data.controller->addr.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - virDomainControllerInsertPreAlloced(vm->def, dev->data.controller); + dev->addr.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + virDomainControllerInsertPreAlloced(vm->def, dev); } return ret; } +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; +} + +static int qemudDomainAttachSCSIDisk(virConnectPtr conn, + struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainDiskDefPtr dev, + int qemuCmdFlags) +{ + int i; + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainDeviceDriveAddress driveAddr; + virDomainControllerDefPtr cont; + char *drivestr = NULL; + int ret = -1; + + for (i = 0 ; i < vm->def->ndisks ; i++) { + if (STREQ(vm->def->disks[i]->dst, dev->dst)) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("target %s already exists"), dev->dst); + goto cleanup; + } + } + + /* This func allocates the bus/unit IDs so must be before + * we search for controller + */ + if (qemuBuildDriveStr(conn, dev, 0, qemuCmdFlags, &drivestr) < 0) + goto cleanup; + + + /* We should have an adddress now, so make sure */ + if (dev->addr.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("unexpected disk address type %s"), + virDomainDeviceAddressTypeToString(dev->addr.type)); + goto cleanup; + } + + for (i = 0 ; i < dev->addr.data.drive.controller ; i++) { + cont = qemuDomainFindOrCreateSCSIDiskController(conn, driver, vm, i); + if (!cont) + goto cleanup; + } + + if (cont->addr.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("SCSI controller %d was missing its PCI address"), cont->idx); + goto cleanup; + } + + if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) { + virReportOOMError(conn); + goto cleanup; + } + + qemuDomainObjEnterMonitorWithDriver(driver, vm); + ret = qemuMonitorAttachDrive(priv->mon, + drivestr, + &cont->addr.data.pci, + &driveAddr); + + qemuDomainObjExitMonitorWithDriver(driver, vm); + + if (ret == 0) { + if (dev->addr.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE || + dev->addr.mode == VIR_DOMAIN_DEVICE_ADDRESS_MODE_DYNAMIC) { + dev->addr.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; + dev->addr.mode = VIR_DOMAIN_DEVICE_ADDRESS_MODE_DYNAMIC; + memcpy(&dev->addr.data.drive, &driveAddr, sizeof(driveAddr)); + } + virDomainDiskInsertPreAlloced(vm->def, dev); + } + +cleanup: + VIR_FREE(drivestr); + return ret; +} + + static int qemudDomainAttachUsbMassstorageDevice(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, @@ -5362,9 +5475,10 @@ static int qemudDomainAttachDevice(virDomainPtr dom, if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) { ret = qemudDomainAttachUsbMassstorageDevice(dom->conn, driver, vm, dev); - } else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI || - dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { + } else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { ret = qemudDomainAttachPciDiskDevice(dom->conn, driver, vm, dev); + } else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) { + ret = qemudDomainAttachSCSIDisk(dom->conn, driver, vm, dev->data.disk, qemuCmdFlags); } else { qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, _("disk bus '%s' cannot be hotplugged."), @@ -5385,7 +5499,8 @@ static int qemudDomainAttachDevice(virDomainPtr dom, } } else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { if (dev->data.controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { - ret = qemudDomainAttachPciControllerDevice(dom->conn, driver, vm, dev); + ret = qemudDomainAttachPciControllerDevice(dom->conn, driver, vm, + dev->data.controller); } else { qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, _("disk controller bus '%s' cannot be hotplugged."), @@ -5786,8 +5901,7 @@ static int qemudDomainDetachDevice(virDomainPtr dom, 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)) { + dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { ret = qemudDomainDetachPciDiskDevice(dom->conn, driver, vm, dev); if (driver->securityDriver) driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn, vm, dev->data.disk); @@ -5806,9 +5920,10 @@ static int qemudDomainDetachDevice(virDomainPtr dom, } } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { ret = qemudDomainDetachHostDevice(dom->conn, driver, vm, dev); - } else + } else { qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, - "%s", _("only SCSI or virtio disk device can be detached dynamically")); + "%s", _("This type of device cannot be hot unplugged")); + } if (!ret && virDomainSaveStatus(dom->conn, driver->caps, driver->stateDir, vm) < 0) ret = -1; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 4ecbcba..30f2cce 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1256,3 +1256,23 @@ int qemuMonitorAttachPCIDiskController(qemuMonitorPtr mon, return ret; } + + +int qemuMonitorAttachDrive(qemuMonitorPtr mon, + const char *drivestr, + virDomainDevicePCIAddress *controllerAddr, + virDomainDeviceDriveAddress *driveAddr) +{ + DEBUG("mon=%p, fd=%d drivestr=%s domain=%d bus=%d slot=%d function=%d", + mon, mon->fd, drivestr, + controllerAddr->domain, controllerAddr->bus, + controllerAddr->slot, controllerAddr->function); + int ret; + + if (mon->json) + ret = qemuMonitorJSONAttachDrive(mon, drivestr, controllerAddr, driveAddr); + else + ret = qemuMonitorTextAttachDrive(mon, drivestr, controllerAddr, driveAddr); + + return ret; +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 70827c7..fcfdcd5 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -264,4 +264,9 @@ int qemuMonitorAttachPCIDiskController(qemuMonitorPtr mon, const char *bus, virDomainDevicePCIAddress *guestAddr); +int qemuMonitorAttachDrive(qemuMonitorPtr mon, + const char *drivestr, + virDomainDevicePCIAddress *controllerAddr, + virDomainDeviceDriveAddress *driveAddr); + #endif /* QEMU_MONITOR_H */ diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 2e605ac..61df373 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1202,10 +1202,9 @@ int qemuMonitorJSONAddUSBDeviceMatch(qemuMonitorPtr mon, } -/* XXX qemu also returns a 'function' number now */ static int -qemuMonitorJSONGetGuestAddress(virJSONValuePtr reply, - virDomainDevicePCIAddress *guestAddr) +qemuMonitorJSONGetGuestPCIAddress(virJSONValuePtr reply, + virDomainDevicePCIAddress *guestAddr) { virJSONValuePtr addr; @@ -1277,7 +1276,7 @@ int qemuMonitorJSONAddPCIHostDevice(qemuMonitorPtr mon, ret = qemuMonitorJSONCheckError(cmd, reply); if (ret == 0 && - qemuMonitorJSONGetGuestAddress(reply, guestAddr) < 0) + qemuMonitorJSONGetGuestPCIAddress(reply, guestAddr) < 0) ret = -1; virJSONValueFree(cmd); @@ -1318,7 +1317,7 @@ int qemuMonitorJSONAddPCIDisk(qemuMonitorPtr mon, ret = qemuMonitorJSONCheckError(cmd, reply); if (ret == 0 && - qemuMonitorJSONGetGuestAddress(reply, guestAddr) < 0) + qemuMonitorJSONGetGuestPCIAddress(reply, guestAddr) < 0) ret = -1; virJSONValueFree(cmd); @@ -1350,7 +1349,7 @@ int qemuMonitorJSONAddPCINetwork(qemuMonitorPtr mon, ret = qemuMonitorJSONCheckError(cmd, reply); if (ret == 0 && - qemuMonitorJSONGetGuestAddress(reply, guestAddr) < 0) + qemuMonitorJSONGetGuestPCIAddress(reply, guestAddr) < 0) ret = -1; virJSONValueFree(cmd); @@ -1514,7 +1513,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; +} + + +static int +qemuMonitorJSONGetGuestDriveAddress(virJSONValuePtr reply, + virDomainDeviceDriveAddress *driveAddr) +{ + virJSONValuePtr addr; + + addr = virJSONValueObjectGet(reply, "return"); + if (!addr || addr->type != VIR_JSON_TYPE_OBJECT) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("drive_add reply was missing device address")); + return -1; + } + + if (virJSONValueObjectGetNumberUint(addr, "bus", &driveAddr->bus) < 0) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("drive_add reply was missing device bus number")); + return -1; + } + + if (virJSONValueObjectGetNumberUint(addr, "unit", &driveAddr->unit) < 0) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("drive_add reply was missing device unit number")); + return -1; + } + + return 0; +} + + +int qemuMonitorJSONAttachDrive(qemuMonitorPtr mon, + const char *drivestr, + virDomainDevicePCIAddress* controllerAddr, + virDomainDeviceDriveAddress* driveAddr) +{ + int ret; + virJSONValuePtr cmd = NULL; + virJSONValuePtr reply = NULL; + char *dev; + + if (virAsprintf(&dev, "%.2x:%.2x.%.1x", + controllerAddr->bus, controllerAddr->slot, controllerAddr->function) < 0) { + virReportOOMError(NULL); + return -1; + } + + cmd = qemuMonitorJSONMakeCommand("drive_add", + "s:pci_addr", dev, + "s:opts", drivestr, + NULL); + VIR_FREE(dev); + if (!cmd) + return -1; + + ret = qemuMonitorJSONCommand(mon, cmd, &reply); + + if (ret == 0) + ret = qemuMonitorJSONCheckError(cmd, reply); + + if (ret == 0 && + qemuMonitorJSONGetGuestDriveAddress(reply, driveAddr) < 0) ret = -1; virJSONValueFree(cmd); diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 221dfa7..1997dbb 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -144,4 +144,9 @@ int qemuMonitorJSONAttachPCIDiskController(qemuMonitorPtr mon, const char *bus, virDomainDevicePCIAddress *guestAddr); +int qemuMonitorJSONAttachDrive(qemuMonitorPtr mon, + const char *drivestr, + virDomainDevicePCIAddress *controllerAddr, + virDomainDeviceDriveAddress *driveAddr); + #endif /* QEMU_MONITOR_JSON_H */ diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index a9d8a4c..16bc81f 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -1620,3 +1620,105 @@ cleanup: VIR_FREE(reply); return ret; } + + +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; + + 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; + } + + 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; +} + + +int qemuMonitorTextAttachDrive(qemuMonitorPtr mon, + const char *drivestr, + virDomainDevicePCIAddress *controllerAddr, + virDomainDeviceDriveAddress *driveAddr) +{ + char *cmd = NULL; + char *reply = NULL; + int ret = -1; + char *safe_str; + int tryOldSyntax = 0; + + safe_str = qemuMonitorEscapeArg(drivestr); + if (!safe_str) { + virReportOOMError(NULL); + return -1; + } + +try_command: + ret = virAsprintf(&cmd, "drive_add %s%.2x:%.2x:%.2x %s", + (tryOldSyntax ? "" : "pci_addr="), + controllerAddr->domain, controllerAddr->bus, + controllerAddr->slot, safe_str); + if (ret == -1) { + virReportOOMError(NULL); + goto cleanup; + } + + if (qemuMonitorCommand(mon, cmd, &reply) < 0) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to close fd in qemu with '%s'"), cmd); + goto cleanup; + } + + if (qemudParseDriveAddReply(reply, driveAddr) < 0) { + if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { + VIR_FREE(reply); + tryOldSyntax = 1; + goto try_command; + } + qemudReportError (NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("adding %s disk failed: %s"), drivestr, reply); + VIR_FREE(reply); + return -1; + } + + ret = 0; + +cleanup: + VIR_FREE(cmd); + VIR_FREE(reply); + VIR_FREE(safe_str); + return ret; +} + + diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index 0e131ea..94fcc02 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -144,5 +144,10 @@ int qemuMonitorTextAttachPCIDiskController(qemuMonitorPtr mon, const char *bus, virDomainDevicePCIAddress *guestAddr); +int qemuMonitorTextAttachDrive(qemuMonitorPtr mon, + const char *drivestr, + virDomainDevicePCIAddress *controllerAddr, + virDomainDeviceDriveAddress *driveAddr); + #endif /* QEMU_MONITOR_TEXT_H */ -- 1.6.5.2