On 12/01/2015 05:21 PM, John Ferlan wrote:
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...
yes
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?
The side effect is that the change broke SCSI device hotplug if the
domain does not have the SCSI controller defined. I thought the sentence
above covered that.
What was missed by moving the controller add to post
processing?
I guess you missed when moving that you moved from domain parsing to
device parsing.
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
ok
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?
Hotplug uses the same device parsing that also domain parsing uses!
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.
As far as I can see there is no generic solution in this case.
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.
I also had a hard baffled time until I found the code shift was from
domain to device ...
Signed-off-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxxxxxxx>
Reviewed-by: Bjoern Walk <bwalk@xxxxxxxxxxxxxxxxxx>
Reviewed-by: Stefan Zimmermann <stzi@xxxxxxxxxxxxxxxxxx>
---
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...
Yes, we can merge it for logical means. Leaving them separate should not
hurt since patch 1 adds first the "add controller" on domain parsing
back again and patch 2 removes the "add controller" from the device
parsing. But you are correct that only with patch 1 and 2 applied the
problem is resolved.
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);
--
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