Re: [libvirt] [PATCH 9/9] Implement disk- and controller hotremove

On Fri, Sep 18, 2009 at 05:26:16PM +0200, Wolfgang Mauerer wrote: > Since both disks and disk controllers can be dynamically > added to the system, it makes sense to be also able to remove > them. This is the main patch which requires additional support in QEMu if I'm understanding your patcheset to qemu-devel correctly.

> diff --git a/src/domain_conf.h b/src/domain_conf.h > index 17f8b14..c7d49cf 100644 > --- a/src/domain_conf.h > +++ b/src/domain_conf.h > @@ -149,6 +149,14 @@ virDiskHasValidPciAddr(virDomainDiskDefPtr def) > return def->pci_addr.domain || def->pci_addr.domain || def->pci_addr.slot; > } > > +static inline int > +virDiskHasValidController(virDomainDiskDefPtr def) > +{ > + return def->controller_id != NULL || > + def->controller_pci_addr.domain || def->controller_pci_addr.domain > + || def->controller_pci_addr.slot; > +} > + > > /* Two types of disk backends */ > enum virDomainFSType { > diff --git a/src/qemu_driver.c b/src/qemu_driver.c > index ddc46f6..5ac89a1 100644 > --- a/src/qemu_driver.c > +++ b/src/qemu_driver.c > @@ -6204,32 +6204,31 @@ cleanup: > return ret; > } > > -static int qemudDomainDetachPciDiskDevice(virConnectPtr conn, > - virDomainObjPtr vm, virDomainDeviceDefPtr dev) > +static int qemudDomainDetachDiskController(virConnectPtr conn, > + virDomainObjPtr vm, virDomainDeviceDefPtr dev) > { > int i, ret = -1; > char *cmd = NULL; > char *reply = NULL; > - virDomainDiskDefPtr detach = NULL; > + virDomainControllerDefPtr detach = NULL; > int tryOldSyntax = 0; > > - for (i = 0 ; i < vm->def->ndisks ; i++) { > - if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) { > - detach = vm->def->disks[i]; > +// virDomainControllerDefPtr is dev->data.controller > + for (i = 0 ; i < vm->def->ncontrollers ; i++) { > + if (vm->def->controllers[i]->pci_addr.domain == > + dev->data.controller->pci_addr.domain && > + vm->def->controllers[i]->pci_addr.bus == > + dev->data.controller->pci_addr.bus && > + vm->def->controllers[i]->pci_addr.slot == > + dev->data.controller->pci_addr.slot) { > + detach = vm->def->controllers[i]; > break; > } > } > > if (!detach) { > qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, > - _("disk %s not found"), dev->data.disk->dst); > - goto cleanup; > - } > - > - if (!virDiskHasValidPciAddr(detach)) { > - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, > - _("disk %s cannot be detached - no PCI address for device"), > - detach->dst); > + _("Controller %s not found"), dev->data.disk->dst); > goto cleanup; > } > > @@ -6251,7 +6250,9 @@ try_command: > > if (qemudMonitorCommand(vm, cmd, &reply) < 0) { > qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, > - _("failed to execute detach disk %s command"), detach->dst); > + _("failed to execute detach controller %d:%d:%d " \ > + "command"), detach->pci_addr.domain, > + detach->pci_addr.bus, detach->pci_addr.slot); > goto cleanup; > } > > @@ -6267,6 +6268,124 @@ try_command: > if (strstr(reply, "invalid slot") || > strstr(reply, "Invalid pci address")) { > qemudReportError (conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, > + _("failed to detach controller: invalid PCI address %.4x:%.2x:%.2x: %s"), > + detach->pci_addr.domain, > + detach->pci_addr.bus, > + detach->pci_addr.slot, > + reply); > + goto cleanup; > + } > + > + if (vm->def->ncontrollers > 1) { > + vm->def->controllers[i] = vm->def->controllers[--vm->def->ncontrollers]; > + if (VIR_REALLOC_N(vm->def->controllers, vm->def->ncontrollers) < 0) { > + virReportOOMError(conn); > + goto cleanup; > + } > + qsort(vm->def->disks, vm->def->ncontrollers, > + sizeof(*vm->def->controllers), > + virDomainControllerQSort); > + } else { > + VIR_FREE(vm->def->controllers[0]); > + vm->def->controllers = 0; > + } > + ret = 0; > + > +cleanup: > + VIR_FREE(reply); > + VIR_FREE(cmd); > + return ret; > +} > + > +static int qemudDomainDetachDiskDevice(virConnectPtr conn, > + virDomainObjPtr vm, virDomainDeviceDefPtr dev) > +{ > + int i, ret = -1; > + char *cmd = NULL; > + char *reply = NULL; > + const char* type; > + virDomainDiskDefPtr detach = NULL; > + int tryOldSyntax = 0; > + > + /* If bus and unit are specified, use these first to identify > + the disk */ > + if (dev->data.disk->controller_pci_addr.domain != -1) { > + for (i = 0 ; i < vm->def->ndisks ; i++) { > + if (dev->data.disk->bus_id != -1 && > + dev->data.disk->bus_id == vm->def->disks[i]->bus_id && > + dev->data.disk->unit_id != -1 && > + dev->data.disk->unit_id == vm->def->disks[i]->unit_id) { > + detach = vm->def->disks[i]; > + break; > + } > + } > + } > + > + /* If the device did not specify a controller explicitely (and therefore > + lacks a bus and unit specification), revert to the explicit target > + device specification to identify a device for removal. */ > + if (!detach) { > + for (i = 0 ; i < vm->def->ndisks ; i++) { > + if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) { > + detach = vm->def->disks[i]; > + break; > + } > + } > + } > + > + if (!detach) { > + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, > + _("disk %s not found"), dev->data.disk->dst); > + goto cleanup; > + } > + > + if (!virDiskHasValidPciAddr(detach) && !virDiskHasValidController(detach)) { > + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, > + _("disk %s cannot be detached - no PCI address for device"), > + detach->dst); > + goto cleanup; > + } > + > + type = virDomainDiskBusTypeToString(detach->bus); > + > +try_command: > + if (tryOldSyntax) { > + if (virAsprintf(&cmd, "drive_del 0 %d:%d:%d bus=%d,unit=%d,if=%s", > + detach->pci_addr.domain, detach->pci_addr.bus, > + detach->pci_addr.slot, detach->bus_id, > + detach->unit_id, type) < 0) { > + virReportOOMError(conn); > + goto cleanup; > + } > + } else { > + if (virAsprintf(&cmd, "drive_del pci_addr=%d:%d:%d bus=%d,unit=%d,if=%s", > + detach->pci_addr.domain, > + detach->pci_addr.bus, > + detach->pci_addr.slot, detach->bus_id, > + detach->unit_id, type) < 0) { > + virReportOOMError(conn); > + goto cleanup; > + } > + } > + > + if (qemudMonitorCommand(vm, cmd, &reply) < 0) { > + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, > + _("failed to execute detach disk %s command"), detach->dst); > + goto cleanup; > + } > + > + DEBUG ("%s: drive_del reply: %s",vm->def->name, reply); > + > + if (!tryOldSyntax && > + strstr(reply, "extraneous characters")) { > + tryOldSyntax = 1; > + goto try_command; > + } > + /* OK bux x, unit x is printed on success, but this does not provide > + any new information to us.*/ > + if (strstr(reply, "Invalid pci address") || > + strstr(reply, "No pci device with address")) { > + qemudReportError (conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, > _("failed to detach disk %s: invalid PCI address %.4x:%.2x:%.2x: %s"), > detach->dst, > detach->pci_addr.domain, > @@ -6274,6 +6393,11 @@ try_command: > detach->pci_addr.slot, > reply); > goto cleanup; > + } else if (strstr(reply, "Need to specify bus") || > + strstr(reply, "Need to specify unit")) { > + qemudReportError (conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, > + _("failed to detach disk %s: bus and unit not (internal error)"), > + detach->dst); > } > > if (vm->def->ndisks > 1) { > @@ -6575,7 +6699,7 @@ static int qemudDomainDetachDevice(virDomainPtr dom, > 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 = qemudDomainDetachPciDiskDevice(dom->conn, vm, dev); > + ret = qemudDomainDetachDiskDevice(dom->conn, vm, dev); > if (driver->securityDriver) > driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn, dev->data.disk); > if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0) > @@ -6584,6 +6708,11 @@ static int qemudDomainDetachDevice(virDomainPtr dom, > ret = qemudDomainDetachNetDevice(dom->conn, vm, dev); > } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { > ret = qemudDomainDetachHostDevice(dom->conn, driver, vm, dev); > + } else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { > + /* NOTE: We can unplug the controller without having to > + check if all disks are unplugged; it is at the user's > + responsibility to use the required OS services first. */ > + ret = qemudDomainDetachDiskController(dom->conn, vm, dev); I wonder if we'd be better off raising an error if the app tries to remove a controller which still has disks attached. ie, force the app to hotunplug each disk first. These days I tend towards avoiding side-effects in operations, and reporting errors whereever there's a situation that the app could have avoided the problem. Any other opinions people... ?