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

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.