[libvirt] [PATCH 1/3] conf: Reposition adding SCSI controller for SCSI hostdev hotplug

[Boris Fiuczynski]

On 12/01/2015 05:21 PM, John Ferlan wrote:
>
>
> On 11/30/2015 06:05 AM, Boris Fiuczynski wrote:
>> This patch reverts a part of commit 0d8b24f6b.
>> With commits 0d8b24g6 and 0785966d automatically adding a required controller
>
> g6 ?  I assume you meant f6b...
yes

>
>> had been moved from the domain xml parsing code section into the device post
>> xml parsing code section and in doing so breaking as a side effect the SCSI
>> hostdev hotplugging in case the required SCSI controller had not been defined
>> in the domain.
>
> What side effect?
The side effect is that the change broke SCSI device hotplug if the 
domain does not have the SCSI controller defined. I thought the sentence 
above covered that.

What was missed by moving the controller add to post
> processing?
I guess you missed when moving that you moved from domain parsing to 
device parsing.

>
>>   This behavior occurs because the SCSI controller gets added but
>> is NOT hotplugged. In the hotplug code section the controller is searched for
>> and if not found would be hotplugged but since a SCSI controller already exists
>> in the domain defintion the required SCSI controller hotplug is never executed.
>
> s/defintion/definition
ok

>
> Hmmm, is this the side effect? It perhaps would have been helpful to
> know what API in the hotplug section of code you're referencing. Are you
> referencing qemuDomainFindOrCreateSCSIDiskController?
Hotplug uses the same device parsing that also domain parsing uses!

>
>> This results later in the internal error:
>>
>> error: Failed to attach device from st0.xml
>> error: internal error: Device alias was not set for scsi controller with index 0
>>
>
> I can reproduce this issue if I define/start a domain with no SCSI
> controllers. Then whether the added hostdev has a drive address or not,
> the hotplug fails. I also get the same results for disk hotplug, so
> perhaps the issue needs a more "generic" solution.
As far as I can see there is no generic solution in this case.

>
>> This patch moves the automatic add of a SCSI controller for a SCSI hostdev
>> device back into the domain xml parsing code section.
>>
>
> I'm still baffled why this "works", but need some time to work through
> the algorithms.
I also had a hard baffled time until I found the code shift was from 
domain to device ...

>
>> 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/conf/domain_conf.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index cbfc41e..69cfd0f 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -16285,6 +16285,9 @@ virDomainDefParseXML(xmlDocPtr xml,
>>           }
>>
>>           def->hostdevs[def->nhostdevs++] = hostdev;
>> +
>> +        if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0)
>> +            goto error;
>
> What if the hotplug <hostdev> xml provided an address using perhaps
> controller==1?  I think this may only work in the default case and/or
> when controller==0 in the provided an address case.
>
> If we were to go ahead with this patch, I'd have to merge in patch2 as
> well. That way a git bisect that falls in between patch 1 and 2 won't
> cause some "other" issue...
Yes, we can merge it for logical means. Leaving them separate should not 
hurt since patch 1 adds first the "add controller" on domain parsing 
back again and patch 2 removes the "add controller" from the device 
parsing. But you are correct that only with patch 1 and 2 applied the 
problem is resolved.

>
> I'm still trying to "internalize" the whole issue, this is where I'm at.
>
> Prior to any of my changes, at parse processing if a <hostdev> didn't
> have an <address> element, it would be added. Then nhostdevs would be
> incremented, and virDomainDefMaybeAddHostdevSCSIcontroller would be
> called. That code may call virDomainDefMaybeAddController or return 0
> (and not call virDomainDefMaybeAddController).
>
> With my changes, address assignment is deferred to post processing as
> well as maybe adding a controller. At post processing, for a scsi
> hostdev without an address defined, virDomainHostdevAssignAddress is
> called which would call virDomainDefMaybeAddController if one wasn't
> found or one was found, but was full. That would seemingly add the
> controller via virDomainDefMaybeAddController as would the
> virDomainDefMaybeAddHostdevSCSIcontroller. But it seems perhaps for some
> reason it shouldn't or didn't.
>
> Let's say it didn't add it (for whatever reason), it seems the
> expectation is that when qemuDomainFindOrCreateSCSIDiskController is
> called it won't find the controller present and create/add it, which I
> think is the side effect referenced in the commit message.
>
> I think I'm just missing some nuance, but I'll keep poking to figure it
> out. I at least wanted to provide some feedback to ensure this wasn't
> reviewed/pushed by someone else!  Although if someone else is looking at
> it and has some ideas that'd be great too.
>
> John
>
>
>
>>       }
>>       VIR_FREE(nodes);
>>
>>
>


-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294