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

[John Ferlan]

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? What was missed by moving the controller add to post
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 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...

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