在 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
