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

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

 





在 2018/8/16 下午10:38, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
+typedef struct _virZPCIDeviceAddress virZPCIDeviceAddress;
+typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr;
+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. 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.

   * 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) ?

Cosmetic issues:

   * uid should be listed before fid;

   * the zpci_ prefix is unnecessary if you have a separate struct
     that contains "ZPCI" in the name. But see above :)


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux