Jim Meyering wrote:
> Daniel P. Berrange wrote:
>> On Mon, Mar 15, 2010 at 03:56:55PM +0100, Wolfgang Mauerer wrote:
>>> Jim Meyering wrote:
>>>> Clang found something that might be a real bug.
>>>> I suspect that ...drive.controller will always be at least one,
>>> it can - explanation below.
>>>
>>>> but we should not have to dive into the code trying to figure
>>>> that out. It's easier/better here just to handle the potential trouble:
>>>>
>>>> clang saw that if it *was* zero, then the following "for" loop
>>>> would not be entered, and "cont" would not be initialized.
>>>> On the very next statement "cont" (uninitialized) would be dereferenced.
>>> (...)
>>>> * src/qemu/qemu_driver.c (qemudDomainAttachSCSIDisk): Handle
>>>> the (theoretical) case of an empty controller list, so that
>>>> clang does not think the subsequent dereference of "cont"
>>>> would dereference an undefined variable (due to preceding
>>>> loop not iterating even once).
>>>> ---
>>>> src/qemu/qemu_driver.c | 6 ++++++
>>>> 1 files changed, 6 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>>> index 7f7c459..efb1857 100644
>>>> --- a/src/qemu/qemu_driver.c
>>>> +++ b/src/qemu/qemu_driver.c
>>>> @@ -5671,18 +5671,24 @@ 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++) {
>>>> (...)
>>> disk->info.addr.drive.controller does not denote the number of
>>> available controllers, but an index -- which can very well be zero,
>>> and the loop is always entered. Besides, checking for < 0 in
>>> the test does not make sense since
>>> _virDomainDeviceDriveAddress.controller is unsigned.
>>>
>>> Since this commit breaks SCSI disk hotplug on controller 0,
>>> please revert it.
>> Agreed, this is definitely broken.
>
> Thanks. The patch below reverts it.
>
> 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.
completely agreed.
Thanks, Wolfgang
>
> 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)
> --
> 1.7.0.2.398.g10c2f
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list