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@xxxxxxxxxx>
> 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list