[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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... ?

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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]