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... ?
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list