[libvirt] [PATCH 2/4] qemu: Use same model when adding hostdev SCSI controller

[John Ferlan]

On 12/20/2017 07:46 AM, Ján Tomko wrote:
> On Wed, Dec 06, 2017 at 08:08:04AM -0500, John Ferlan wrote:
>> When qemuDomainFindOrCreateSCSIDiskController adds a controller,
>> let's use the same model as a currently found controller under the
>> assumption that the reason to add the controller in hotplug is
>> because virDomainHostdevAssignAddress determined that there were
>> too many devices on the existing controller, but only assigned a
>> new controller index and did not add a new controller and we
>> desire to use the same controller model as any existing conroller
> 
> s/conroller/controller/
> 
>> and not take a chance that qemuDomainSetSCSIControllerModel would
>> use a default that may be incompatible.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>> src/qemu/qemu_hotplug.c | 14 ++++++++++----
>> 1 file changed, 10 insertions(+), 4 deletions(-)
>>
> 
> ACK
> 
> I like that this reduces the chances of model -1 appearing in XML,
> (maybe a step closer to removing qemuDomainSetSCSIControllerModel?).
> On the other hand, it does change the 'policy' of choosing the
> controller model. We don't plug one on PCI hotplug.
> 

True... OTOH for this path, we're processing *just* a <hostdev> or
<disk> and will be creating the controller hopefully based on what is
existing and already full. I would hope that wouldn't be considered bad
policy!

I found more problems when having a full virtio-scsi controller and
creating an LSI controller because that's the default.

Beyond that, Michal noted something on qemu-devel about having more than
one LUN on an LSI controller being a problem:

http://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg01125.html

There's also some patches on qemu-devel where iSCSI LUN's on an LSI
controller were causing crashes (perhaps something I ran into, but
didn't spend the time debugging):

http://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg03119.html



>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 9317e134a..90d50e7b1 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
> 
> [...]
> 
>> @@ -596,6 +597,12 @@
>> qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver,
>>
>>         if (cont->idx == controller)
>>             return cont;
>> +
>> +        /* Save off the model - if we end up creating a controller it's
> 
> What does 'save off' mean?
> 

Just an expression... For the most recent text I used see:

https://www.redhat.com/archives/libvir-list/2017-December/msg00483.html

I can still remove the "off" part

John

> Jan
> 
>> +         * because the user didn't provide one and we need to
>> automagically
>> +         * create one because the existing one is full - so let's be
>> sure
>> +         * to keep the same model in that case. */
>> +        model = cont->model;
>>     }
>>
>>     /* No SCSI controller present, for backward compatibility we