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

On 12/03/2015 05:56 PM, John Ferlan wrote: > > > 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. > */ First sentence I can agree to. The second sentence I am not sure I understand. Did you mean: The controller the disk is going to use must exist before a qemu command line string is being generated.

> >> >> + 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? yes, it's a miss... good catch! ;-) > > John >