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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list