Re: [libvirt] [PATCH 3/3] Automatic SCSI controller creation in SCSI disk hotplug broken

On 11/30/2015 06:06 AM, Boris Fiuczynski wrote: > When a SCSI disk is hotplugged to a domain that does not have the required > scsi controller already defined the following internal error occurs > > error: Failed to attach device from scsi_disk.xml > error: internal error: Could not find scsi controller with index 0 required for device > > Commit 0260506c added in method qemuBuildDriveDevStr a lookup of the controller > alias. The internal error occurs because in method qemuDomainAttachSCSIDisk > the automatic creation of the potentially missing SCSI controller occurs after > calling qemuBuildDriveDevStr. > > This patch reverses the calling sequence. > > Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; > Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; > Reviewed-by: Stefan Zimmermann <stzi(a)linux.vnet.ibm.com&gt; > --- > src/qemu/qemu_hotplug.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index 8804d3d..210d485 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -607,6 +607,12 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, > goto error; > } Would you say the following is an accurate description to add? /* Let's make sure our disk has a controller defined and loaded * before trying add the disk. The virDomainDiskDefAssignAddress * doesn't try to add a controller if one doesn't exist, it just * assigns the disk to the calculated spot. */

> > + for (i = 0; i <= disk->info.addr.drive.controller; i++) { > + cont = qemuDomainFindOrCreateSCSIDiskController(driver, vm, i); > + if (!cont) > + goto error; > + } > + > if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { > if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) > goto error; > @@ -617,12 +623,6 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, > if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->qemuCaps))) > goto error; > > - for (i = 0; i <= disk->info.addr.drive.controller; i++) { > - cont = qemuDomainFindOrCreateSCSIDiskController(driver, vm, i); > - if (!cont) > - goto error; > - } > - > /* Tell clang that "cont" is non-NULL. > This is because disk->info.addr.driver.controller is unsigned, > and hence the above loop must iterate at least once. */ > Is there a reason you chose to not grab the clang check too?

John