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

[Yi Min Zhao]

在 2018/10/18 下午11:44, 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.
>
>
Do you mean expect Laine to take a quick look at this series, or the 
next one?

-- 
Yi Min