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

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.