[libvirt] [PATCH v6 09/13] conf: Allocate/release 'uid' and 'fid' in PCI address

[Andrea Bolognani]

On Thu, 2018-10-18 at 15:13 +0800, Yi Min Zhao wrote:
> 在 2018/10/17 下午10:30, Andrea Bolognani 写道:
> > On Tue, 2018-10-16 at 11:28 +0800, Yi Min Zhao wrote:
> > > I think this would make things complex. If either PCI address or
> > > zPCI address exists, we have to do more checks for calling
> > > virDomainPCIAddressReserveAddr(). And there are amounts of
> > > code calling ***IsWanted() to call ***ReserveNext***(). I think
> > > keeping them separately is better.
> > 
> > Again, I might be missing something because I haven't actually tried
> > implementing any of this, but at least from the theoretical point of
> > view I don't see how keeping them separate would make things simpler:
> > if anything, it seems to me like it would make them more complicated
> > for the calling code because now you have to worry about the PCI
> > address extensions *in addition* to the PCI address itself.
> 
> For example, during collection stage, checking both PCI address and 
> extension address
> is requried, and still need to separately do some additional checks for 
> PCI address if it
> is present, at last in reserving addr function we still check if PCI 
> normal address or
> extension address exists to separately reserve present one. So that we 
> have to do the
> check on the same condition repetively. If you don't have strong 
> opposition, I want to
> send the new version ASAP.

So I gave an half-hearted stab at implementing my own suggestion
today, and quite unsurprisingly I have gained more sympathy for your
argument in the process :)

The main problem I see is that, as you noticed, we have a lot of
calls to IsWanted(), IsPresent(), ReserveAddr() and ReserveNextAddr()
where really we should be using EnsureAddr() pretty much all of the
time and hide most of the details from the drivers, which in turn
would make it easier to change them in a transparent manner.

Another big problem, which I highlighted in the past, is that the
current API was not designed with the idea that PCI addresses could
have "parts" in mind, and so it's not nuanced enough: if I call
IsEmpty() on and address where the PCI part itself has been filled
in but the zPCI part hasn't, or vice versa, what should I get back?
The answer is probably that, after we've made sure those functions
are used as little as possible thanks to the changes outlined above,
we should replace them with better named alternatives.

Of course it would be entirely unfair to ask you to perform such
a massive refactoring before your series can be considered for
inclusion, so at this point I'm okay with merging it and cleaning
up the pre-existing mess afterwards.

There's still the question of whether Dan is now okay with the XML
structure as currently implemented; assuming that's the case, it
would be great if Laine could also take a quick look at the series
before it's pushed.

-- 
Andrea Bolognani / Red Hat / Virtualization