From: "Daniel P. Berrange" <berrange@redhat.com>
To: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
Cc: libvir-list@redhat.com, jan.kiszka@siemens.com, qemu-devel@nongnu.org
Subject: [Qemu-devel] Re: [libvirt] [PATCH 7/9] Remove surprises in the semantics of disk-hotadd
Date: Thu, 24 Sep 2009 11:59:34 +0100 [thread overview]
Message-ID: <20090924105934.GE21831@redhat.com> (raw)
In-Reply-To: <1253287576-12875-8-git-send-email-wolfgang.mauerer@siemens.com>
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 <wolfgang.mauerer@siemens.com>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 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 <controller> 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 :|
next prev parent reply other threads:[~2009-09-24 10:59 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-09-18 15:26 [Qemu-devel] [PATCH 0/9] Support disk-hotremove and controller hotplugging Wolfgang Mauerer
2009-09-18 15:26 ` [Qemu-devel] [PATCH 1/9] Clarify documentation for private symbols Wolfgang Mauerer
2009-09-18 15:26 ` [Qemu-devel] [PATCH 2/9] Extend <disk> element with controller information Wolfgang Mauerer
2009-09-18 15:26 ` [Qemu-devel] [PATCH 3/9] Add new domain device: "controller" Wolfgang Mauerer
2009-09-24 10:52 ` [Qemu-devel] Re: [libvirt] " Daniel P. Berrange
2009-09-18 15:26 ` [Qemu-devel] [PATCH 4/9] Add disk-based hotplugging for the qemu backend Wolfgang Mauerer
2009-09-18 15:26 ` [Qemu-devel] [PATCH 5/9] Implement controller hotplugging Wolfgang Mauerer
2009-09-18 15:26 ` [Qemu-devel] [PATCH 6/9] Allow controller selection by ID Wolfgang Mauerer
2009-09-18 15:26 ` [Qemu-devel] [PATCH 7/9] Remove surprises in the semantics of disk-hotadd Wolfgang Mauerer
2009-09-24 10:59 ` Daniel P. Berrange [this message]
2009-09-18 15:26 ` [Qemu-devel] [PATCH 8/9] Factor out the method to get the PCI address of a controller for a given disk Wolfgang Mauerer
2009-09-18 15:26 ` [Qemu-devel] [PATCH 9/9] Implement disk- and controller hotremove Wolfgang Mauerer
[not found] ` <20091009163203.GA30218@redhat.com>
2009-10-13 9:08 ` [Qemu-devel] Re: [libvirt] [PATCH 0/9] Support disk-hotremove and controller hotplugging Gerd Hoffmann
2009-10-13 9:34 ` Wolfgang Mauerer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20090924105934.GE21831@redhat.com \
--to=berrange@redhat.com \
--cc=jan.kiszka@siemens.com \
--cc=libvir-list@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=wolfgang.mauerer@siemens.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).