On 12/04/2015 12:07 PM, Boris Fiuczynski wrote:
> 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@xxxxxxxxxxxxxxxxxx>
>>> Reviewed-by: Bjoern Walk <bwalk@xxxxxxxxxxxxxxxxxx>
>>> Reviewed-by: Stefan Zimmermann <stzi@xxxxxxxxxxxxxxxxxx>
>>> ---
>>> 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.
It's meant to point out the place in the code where the address could
have been generated; however, I suppose a user could have provided an
address and that doesn't make sense. I used your suggestion and pushed...
Thanks for finding and resolving this...
John
>>
>>>
>>> + 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
>>
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list