Re: [libvirt] [PATCH v3 01/12] conf: Add definitions for 'uid' and 'fid' PCI address attributes

On Mon, 2018-08-20 at 16:19 +0800, Yi Min Zhao wrote: > 在 2018/8/16 下午10:38, Andrea Bolognani 写道: >> On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: >>> +struct _virZPCIDeviceAddress { >>> + unsigned int zpci_fid; >>> + unsigned int zpci_uid; >>> + bool fid_assigned; >>> + bool uid_assigned; >>> +}; >> A couple of questions about the approach here, one of which I have >> mentioned already and one of which I probably haven't (my bad): >> >> * do you really need to have separate booleans tracking whether >> or not either id has been assigned? Wouldn't the same approach >> as virPCIDeviceAddressIsEmpty() work, eg. consider the address >> absent if both are zero and present otherwise? > It's OK for uid. But for fid, zero is a valid value, so we need a bool > to track its assignment. See virPCIDeviceAddressIsEmpty() and virPCIDeviceAddressIsValid(), which have very similar requirement but don't use extra booleans to keep track of state. You could do the same thing those functions do: * the zPCI address is empty if both uid and fid are zero;

* the zPCI address is invalid if it's empty or uid is too large. You already have the latter covered, so it's only a matter of implementing the former. > If we add a boolean for fid, why not also add another one for uid to > keep consistency? > This also makes code easy to read and obvious. It doesn't waste much > memory space. I think it actually makes more complicated, which is why I'd rather get rid of it :) >> * especially if you don't need the additional booleans, would it >> be preferable to just add the two ids to the existing struct >> instead of declaring a new one that you'll have to allocate >> and make sure it's not NULL before accessing it? Again, I seem >> to remember Laine feeling somewhat strongly about the topic. > For other platforms, is it OK to waste this unused memory (of course, > it's little) ? I believe adding a couple of unsigned ints is worth it if we can get rid of all the checks on addr->zpci because of it.