Daniel P. Berrange wrote:
> 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.
Yes, that's right.
>
>> 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... ?
I'm not sure if this is worth the extra effort: The disk needs to
be removed/unmounted/unplugged from the underlying operating
system before it can be removed from the hypervisor (and before the
controller is unplugged), but I don't see a way for libvirt to check
if the required operations have been performed or not. So if the
user does things wrong and removes the disk from the hypervisor without
any preparatory work in the guest OS, we will get errors in any case.
If, on the other hand, the user does things right (i.e, remove the
disk from the OS and then unplug the controller), removing the
disks from libvirt is just an extra step. And with real hardware,
I would suppose that it's possible to remove a controller with
attached disks of the controller allows for PCI hotplug.
No clear preference for any of these options from my side, but a slight
bias towards not emitting an error message. Any other opinions
people... ? ;-)
Thanks, Wolfgang
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list