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

Re: [libvirt] [PATCH 7/9] Remove surprises in the semantics of disk-hotadd



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


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