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

[Boris Fiuczynski]

On 12/02/2015 06:13 PM, John Ferlan wrote:
>
>
> On 12/02/2015 11:37 AM, Boris Fiuczynski wrote:
>
> [...]
>
>>> So, what if qemuDomainFindOrCreateSCSIDiskController checked the "found"
>>> cont in the first loop for "cont->info.alias". If it doesn't exist, then
>>> fall through under the "assumption" that the controller was added by
>>> some other mechanism into the list?  That would require some more
>>> adjustments in qemuDomainAttachControllerDevice to know the controller
>>> was already in the list, but not active in the domain.
>>
>> I thought about that as well and decided against it because you would
>> add controllers in the domain data structure and than decide based on
>> the aliases if the controller needs to be hotplugged afterwards. I
>> thought that using the alias to detect is the domain is running in such
>> way would not be proper!
>> Anyhow what are you going to do if hotplugging the controller fails?
>> Would this require an additional cleanup method?
>> qemuDomainAttachControllerDevice is also used when adding a new scsi
>> controller directly from virsh attach-device (scsi-controller.xml). What
>> additional special casing would be required there...?
>>
>
> I had just started down that path... not that I like that option, but it
> was an idea.  I was able to get it to work in the one case, but other
> issues showed up, so I'll just abandon it...
>
>
> Although we're now in freeze, I will go with these two patches. I think
I would like to encourage you to also pick the third patch fixing the 
SCSI disk hotplug scenario.

> it's better to combine them - mostly to make it "clearer" for anyone
> venturing down this path in the future to see the relationship(s).
Agreed!

>
> The combined commit message would be (the weird line breaks only occur
> here because of my settings on line width/break for sent emails):
>
> conf: Revert some code to resolve issues for hostdev hotplug
>
> This patch reverts parts of commits 0d8b24f6b and 0785966d dealing with
> the addition of a controller during virDomainHostdevAssignAddress. This
> caused a regression for the hostdev hotplug path which assumes the
> qemuDomainFindOrCreateSCSIDiskController will add the new controller
> during qemuDomainAttachHostSCSIDevice to both the running domain and
> the domain def controller list when the controller doesn't yet exist
> (whether due to no SCSI controllers existing or the addition of a new
> controller because existing ones are full).
>
> Since commit id 0d8b24f6 will call virDomainHostdevAssignAddress during
> virDomainDeviceDefPostParseInternal which is called either during domain
> definition post processing (via an iterator during virDomainDefPostParse)
> or directly from virDomainDeviceDefParse during hotplug, the change
> broke the "side effect" of being able to add both a hostdev and controller
> to the running domain.
>
> The regression would only be seen if the running domain didn't have a
> SCSI controller alrady defined or if the existing SCSI controller was
> "full" of devices and a new controller needed to be created.
>
> This patch will also add some extra comments to the code to avoid a
> similar future change.
Reads good.

>
>
> I'd like to also add a pair of comments to also leave a second trail of
> breadcrumbs...
>
> In virDomainHostdevAssignAddress after the initial loop:
>
>     /* NB: Do not attempt calling virDomainDefMaybeAddController to
>      * automagically add a "new" controller. Doing so will result in
>      * qemuDomainFindOrCreateSCSIDiskController "finding" the controller
>      * in the domain def list and thus not hotplugging the controller as
>      * well as the hostdev in the event that there are either no SCSI
>      * controllers defined or there was no space on an existing one.
>      */
>
> and in virDomainDefParseXML prior to maybe adding the controller:
>
>     /* For a domain definition, we need to check if the controller
>      * for this hostdev exists yet and if not add it. This cannot be
>      * done during virDomainHostdevAssignAddress (as part of device
>      * post processing) because that will result in the failure to
>      * load the controller during hostdev hotplug.
>      */
>
>
> Does this all seem reasonable?
Yes, and (again) I would like to ask to also add the third patch as the 
second patch to fix the scsi disk hotplug problem.


-- 
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