[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

在 2018/8/20 下午6:35, Andrea Bolognani 写道:
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;
uid=0 and fid=0 can't mean zPCI address is empty, because the user might
only define fid with 0. If fid=0 has been assigned, we should report error. If it is not defined by user, fid is also 0, then we should allocate a valid value
for fid.
   * 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.

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]