All of lore.kernel.org
 help / color / mirror / Atom feed
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 :|

  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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.