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

John Ferlan jferlan at redhat.com
Fri Dec 4 18:57:16 UTC 2015



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 at linux.vnet.ibm.com>
>>> Reviewed-by: Bjoern Walk <bwalk at linux.vnet.ibm.com>
>>> Reviewed-by: Stefan Zimmermann <stzi at linux.vnet.ibm.com>
>>> ---
>>>   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
>>
> 
> 




More information about the libvir-list mailing list