From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Mqm36-0001KB-PX for qemu-devel@nongnu.org; Thu, 24 Sep 2009 06:59:44 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Mqm31-0001IM-1M for qemu-devel@nongnu.org; Thu, 24 Sep 2009 06:59:43 -0400 Received: from [199.232.76.173] (port=43055 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Mqm30-0001IB-LI for qemu-devel@nongnu.org; Thu, 24 Sep 2009 06:59:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:19978) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Mqm2z-0006wf-JG for qemu-devel@nongnu.org; Thu, 24 Sep 2009 06:59:38 -0400 Date: Thu, 24 Sep 2009 11:59:34 +0100 From: "Daniel P. Berrange" Message-ID: <20090924105934.GE21831@redhat.com> References: <1253287576-12875-1-git-send-email-wolfgang.mauerer@siemens.com> <1253287576-12875-8-git-send-email-wolfgang.mauerer@siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1253287576-12875-8-git-send-email-wolfgang.mauerer@siemens.com> Subject: [Qemu-devel] Re: [libvirt] [PATCH 7/9] Remove surprises in the semantics of disk-hotadd Reply-To: "Daniel P. Berrange" List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wolfgang Mauerer Cc: libvir-list@redhat.com, jan.kiszka@siemens.com, qemu-devel@nongnu.org On Fri, Sep 18, 2009 at 05:26:14PM +0200, Wolfgang Mauerer wrote: > When a disk is added without an explicitly specified > controller as host, then try to find the first available > controller. If none exists, do not (as in previous versions) > add a new PCI controller device with the disk attached, > but bail out with an error. Notice that this patch changes > the behaviour as compared to older libvirt releases, as > has been discussed on the mailing list (see > http://thread.gmane.org/gmane.comp.emulators.libvirt/15860) Yes, I still think is the good way to go Daniel > > Signed-off-by: Wolfgang Mauerer > Signed-off-by: Jan Kiszka > --- > src/qemu_driver.c | 172 ++++++++++++++++++++++++++--------------------------- > 1 files changed, 85 insertions(+), 87 deletions(-) > > diff --git a/src/qemu_driver.c b/src/qemu_driver.c > index 990f05a..f1c2a45 100644 > --- a/src/qemu_driver.c > +++ b/src/qemu_driver.c > @@ -5417,68 +5417,81 @@ try_command: > controller_specified = 1; > } > > - if (controller_specified) { > - if (dev->data.disk->controller_id) { > - for (i = 0 ; i < vm->def->ncontrollers ; i++) { > - if (STREQ(dev->data.disk->controller_id, > - vm->def->controllers[i]->id)) { > - break; > - } > - } > - > - if (i == vm->def->ncontrollers) { > - qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, > - _("Controller does not exist")); > - return -1; > - } > + if (!controller_specified) { > + /* Find an appropriate controller for disk-hotadd. Notice that > + the admissible controller types (SCSI, virtio) have already > + been checked in qemudDomainAttachDevice. */ > + for (i = 0 ; i < vm->def->ncontrollers ; i++) { > + if (vm->def->controllers[i]->type == dev->data.disk->type) > + break; > + } > + > + if (i == vm->def->ncontrollers) { > + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, > + _("Cannot find appropriate controller for hot-add")); > + return -1; > + } > > - domain = vm->def->controllers[i]->pci_addr.domain; > - bus = vm->def->controllers[i]->pci_addr.bus; > - slot = vm->def->controllers[i]->pci_addr.slot; > - } else { > - domain = dev->data.disk->controller_pci_addr.domain; > - bus = dev->data.disk->controller_pci_addr.bus; > - slot = dev->data.disk->controller_pci_addr.slot; > - > - for (i = 0 ; i < vm->def->ncontrollers ; i++) { > - if (domain == vm->def->controllers[i]->pci_addr.domain && > - bus == vm->def->controllers[i]->pci_addr.bus && > - slot == vm->def->controllers[i]->pci_addr.slot) > - break; > - } > + domain = vm->def->controllers[i]->pci_addr.domain; > + bus = vm->def->controllers[i]->pci_addr.bus; > + slot = vm->def->controllers[i]->pci_addr.slot; > + } > > - if (i == vm->def->ncontrollers) { > - qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, > - _("Controller does not exist")); > - return -1; > + if (controller_specified && dev->data.disk->controller_id) { > + for (i = 0 ; i < vm->def->ncontrollers ; i++) { > + if (STREQ(dev->data.disk->controller_id, > + vm->def->controllers[i]->id)) { > + break; > } > - } > - > - if (dev->data.disk->bus_id != -1) { > - virBufferVSprintf(&buf, ",bus=%d", dev->data.disk->bus_id); > } > > - if (dev->data.disk->unit_id != -1) { > - virBufferVSprintf(&buf, ",unit=%d", dev->data.disk->unit_id); > + if (i == vm->def->ncontrollers) { > + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, > + _("Controller does not exist")); > + return -1; > + } > + > + domain = vm->def->controllers[i]->pci_addr.domain; > + bus = vm->def->controllers[i]->pci_addr.bus; > + slot = vm->def->controllers[i]->pci_addr.slot; > + } else if (controller_specified) { > + domain = dev->data.disk->controller_pci_addr.domain; > + bus = dev->data.disk->controller_pci_addr.bus; > + slot = dev->data.disk->controller_pci_addr.slot; > + > + for (i = 0 ; i < vm->def->ncontrollers ; i++) { > + if (domain == vm->def->controllers[i]->pci_addr.domain && > + bus == vm->def->controllers[i]->pci_addr.bus && > + slot == vm->def->controllers[i]->pci_addr.slot) > + break; > } > + > + if (i == vm->def->ncontrollers) { > + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, > + _("Controller does not exist")); > + return -1; > + } > + } > > - bus_unit_string = virBufferContentAndReset(&buf); > - > - VIR_DEBUG ("%s: drive_add %d:%d:%d file=%s,if=%s%s", vm->def->name, > - domain, bus, slot, safe_path, type, bus_unit_string); > - ret = virAsprintf(&cmd, "drive_add %s%d:%d:%d file=%s,if=%s%s", > - (tryOldSyntax ? "" : "pci_addr="), domain, bus, > - slot, safe_path, type, bus_unit_string); > + /* At this point, we have a valid (domain, bus, slot) triple > + that identifies the controller, regardless if an explicit > + controller has been given or not. */ > + if (dev->data.disk->bus_id != -1) { > + virBufferVSprintf(&buf, ",bus=%d", dev->data.disk->bus_id); > } > - else { > - /* Old-style behaviour: If no reference is given, then > - hotplug a new controller with the disk attached. */ > - VIR_DEBUG ("%s: pci_add %s storage file=%s,if=%s", vm->def->name, > - (tryOldSyntax ? "0": "pci_addr=auto"), safe_path, type); > - ret = virAsprintf(&cmd, "pci_add %s storage file=%s,if=%s", > - (tryOldSyntax ? "0": "pci_addr=auto"), safe_path, type); > + > + if (dev->data.disk->unit_id != -1) { > + virBufferVSprintf(&buf, ",unit=%d", dev->data.disk->unit_id); > } > > + bus_unit_string = virBufferContentAndReset(&buf); > + > + VIR_DEBUG ("%s: drive_add %d:%d:%d file=%s,if=%s%s", vm->def->name, > + domain, bus, slot, safe_path, type, bus_unit_string); > + ret = virAsprintf(&cmd, "drive_add %s%d:%d:%d file=%s,if=%s%s", > + (tryOldSyntax ? "" : "pci_addr="), domain, bus, > + slot, safe_path, type, bus_unit_string); > + > VIR_FREE(safe_path); > if (ret == -1) { > virReportOOMError(conn); > @@ -5494,41 +5507,26 @@ try_command: > > VIR_FREE(cmd); > > - if (controller_specified) { > - if (qemudParseDriveAddReply(vm, reply, &bus_id, &unit_id) < 0) { > - if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { > - VIR_FREE(reply); > - tryOldSyntax = 1; > - goto try_command; > - } > - qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, > - _("adding %s disk failed: %s"), type, reply); > - VIR_FREE(reply); > - return -1; > - } > - > - if (dev->data.disk->bus_id == -1) { > - dev->data.disk->bus_id = bus_id; > - } > - > - if (dev->data.disk->unit_id == -1) { > - dev->data.disk->unit_id = unit_id; > - } > - } else { > - if (qemudParsePciAddReply(vm, reply, &domain, &bus, &slot) < 0) { > - if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { > - VIR_FREE(reply); > - tryOldSyntax = 1; > - goto try_command; > - } > - > - qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, > - _("adding %s disk failed: %s"), type, reply); > - VIR_FREE(reply); > - return -1; > - } > + if (qemudParseDriveAddReply(vm, reply, &bus_id, &unit_id) < 0) { > + if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { > + VIR_FREE(reply); > + tryOldSyntax = 1; > + goto try_command; > + } > + qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, > + _("adding %s disk failed: %s"), type, reply); > + VIR_FREE(reply); > + return -1; > } > - > + > + if (dev->data.disk->bus_id == -1) { > + dev->data.disk->bus_id = bus_id; > + } > + > + if (dev->data.disk->unit_id == -1) { > + dev->data.disk->unit_id = unit_id; > + } > + > dev->data.disk->pci_addr.domain = domain; > dev->data.disk->pci_addr.bus = bus; > dev->data.disk->pci_addr.slot = slot; > -- > 1.6.4 > > -- > Libvir-list mailing list > Libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- |: 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 :|