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

[Wolfgang Mauerer]

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.

Thanks, Wolfgang