Re: [libvirt] [PATCHv3 00/13] Add new PCIe controllers
On 07/25/2015 03:58 PM, Laine Stump wrote:
Since the first 3 patches of V2 were ACKed and uncontroversial, I
fixed the small problems pointed out in the reviews and pushed
them. Thus, Patches 01-13 here correspond to Patches 04-17 in V2.
Most of these patches were already ACKed in V2 (pending my making
small fixes pointed out in review), but the main things that need
review are:
1) changing of model name from a char* to an enum in Patch 01, and
corresponding blowback in patches 02, 07, 10, and 13
2) range checking of chassisNr in Patch 03 and chassis+port in patch
06.
3) check for duplicate <model> in patch 01, and duplicate <target> in
patch 03.
I did add one new negative test, and reworded some documentation, but I'm
about to go mostly offline for 10 days, and would rather not have
these patches bitrotting during that time if they are okay other than
that. (also, I see both of those tasks as having no practical end, but
do give my word to add more to both in later followups).
If by chance everything is ACKed before DV freezes for RC1, but after
I'm already offline (which will happen Sunday morning U.S. east coast
time), I would appreciate if the reviewer could push the patches so
they'll get the RC testing and be in the 1.2.18 release. (If not, I'll
take care of it when I return).
P.S. I ran "git rebase -i master -x "make -j8 check && make -j8
syntax-check" before sending.
Like Martin noted, I've been looking through this series as well. I too
haven't had a ton of time to devote to it over the last week or so since
I'm in the middle of a move. Also being so close to freeze - it just
didn't feel right to shimmy it in there late. This should go in as soon
as possible for 1.2.19 to hopefully weed out any weird issues!
Some more specific/general comments...
The 1.2.18 -> 1.2.19 everywhere will need to be done.
The commit message in 2/13 seems to have incurred a disjoint thought or
perhaps a forgotten edit in the paragraph that starts "The defaults are
set..." - the last sentance just doesn't read well.
I had some concern over confusion with 'chassis' and 'chassisNr'... I
also had concern over whether the parser could confuse chassis values
depending on order seen - as in was a parse of 'chassis' an exact match
or a prefix match... I think it's ok especially since one is used for
pci-bridge and the other for pcie-root-port...
I'm still not sure if it's "best" to set chassisNr or make it some sort
of tristate where if we know it wasn't read in from the XML, then we
default or use the 'cont->idx' internally when formulating the qemu
command. That way at least we don't run into the situation where if
"index" changes, then we don't run into some issue since we don't
follow
on with the chassisNr change (because we cannot be sure who set it).
This is patch 4/13 stuff. I think it's fine as a followup to adjust
that logic if necessary - hopefully something that can be done in the
1.2.19 timeframe though. This would change the logic in
qemuBuildControllerDevStr in the def->model switch for
VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE to perhaps set a local chassisNr
value based on opts.pciopts.chassisNr == -1, then use index, else use
what was set.
Beyond that and the duplicate 'chassis' parse in patch 6, things were
find and ready to push with a few adjustments. Since I know you're on
the road too, I'll watch to see if you get to it - if not, I will make a
couple of adjustments for you.
John