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

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

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

processing?

> 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

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?

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

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

> Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; > Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; > Reviewed-by: Stefan Zimmermann <stzi(a)linux.vnet.ibm.com&gt; > --- > 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...

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