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

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

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

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.

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?