[libvirt] [PATCH] qemudDomainAttachSCSIDisk: handle empty controller list

[Daniel P. Berrange]

On Mon, Mar 15, 2010 at 04:46:31PM +0100, Jim Meyering wrote:
> 
> However, there's more to it than that.
> The controller index, while technically "unsigned", may
> be derived from an expression like -1 / 7,
> since virDomainDiskDefAssignAddress does this:
> 
>   void
>   virDomainDiskDefAssignAddress(virDomainDiskDefPtr def)
>   {
>       int idx = virDiskNameToIndex(def->dst);
> 
>       switch (def->bus) {
>       case VIR_DOMAIN_DISK_BUS_SCSI:
>           ...
>           def->info.addr.drive.controller = idx / 7;
>           def->info.addr.drive.bus = 0;
>           def->info.addr.drive.unit = idx % 7;
>           break;
> 
>       case VIR_DOMAIN_DISK_BUS_IDE:
>           ...
> 
>       case VIR_DOMAIN_DISK_BUS_FDC:
>           ...
>       ...
> 
> and virDiskNameToIndex may return -1.
> And "idx" is then -1.
> 
> While we might have gotten lucky with the -1 -> 0 mapping for the
> .controller member, I doubt a .unit (also "unsigned int") value that's
> derived from "-1 % 7" (not portable, btw) will be safe.
> 
> I will propose a patch to change the above function
> to return an indication of success or failure
> and update the few callers.  They're all in
> functions that already return success or failure,
> so this is the only interface that would change.

That sounds a good plan to me.

> Here's the revert:
> 
> From 4d7423fc0e27e86c937dde1a5d62f3b7b6e49451 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering at redhat.com>
> Date: Mon, 15 Mar 2010 16:43:23 +0100
> Subject: [PATCH] Revert f5a6ce44ce8368d4183b69a31b77f67688d9af43
> 
> * src/qemu/qemu_driver.c (qemudDomainAttachSCSIDisk): The ".controller"
> member is an index, and *may* be 0.  Reported by Wolfgang Mauerer.
> ---
>  src/qemu/qemu_driver.c |    6 ------
>  1 files changed, 0 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index f8ab545..04344a8 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6057,12 +6057,6 @@ static int qemudDomainAttachSCSIDisk(struct qemud_driver *driver,
>      if (!(drivestr = qemuBuildDriveStr(disk, 0, qemuCmdFlags)))
>          goto error;
> 
> -    if (disk->info.addr.drive.controller <= 0) {
> -        qemuReportError(VIR_ERR_INTERNAL_ERROR,
> -                        _("no drive controller for %s"), disk->dst);
> -        goto error;
> -    }
> -
>      for (i = 0 ; i <= disk->info.addr.drive.controller ; i++) {
>          cont = qemuDomainFindOrCreateSCSIDiskController(driver, vm, i, qemuCmdFlags);
>          if (!cont)


ACK

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|