Re: [libvirt] [PATCH v5 07/13] conf: Introduce parser, formatter for uid and fid
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
[...]
+static int
+virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci)
This is a predicate, so it should return a bool.
[...]
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid PCI address uid='0x%x', "
+ "must be > 0x0 and <= 0x%x"),
+ zpci->uid,
+ VIR_DOMAIN_DEVICE_ZPCI_MAX_UID);
You should always use "0x%.4x" when printing uids.
[...]
+static int
+virZPCIDeviceAddressParseXML(xmlNodePtr node,
+ virPCIDeviceAddressPtr addr)
+{
+ char *uid, *fid;
+ int ret = -1;
+ virZPCIDeviceAddress def = { 0 };
One variable per line, please.
Also structs are usually declared first and 'ret' is usually last,
but that's admittedly not very important :)
[...]
+ if (uid) {
+ if (virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Cannot parse <address> 'uid'
attribute"));
+ goto cleanup;
+ }
+ }
You can convert the above to
if (uid &&
virStrToLong_uip(...) < 0) {
...
}
and do the same with fid.
[...]
+bool
+virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr)
+{
+ return !(addr->uid || addr->fid);
+}
This function belongs in virpci, besides the struct definition and
the very much related virPCIDeviceAddressIsEmpty().
[...]
@@ -267,6 +333,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
if (!virPCIDeviceAddressIsEmpty(addr) && !virPCIDeviceAddressIsValid(addr,
true))
goto cleanup;
+ if (virZPCIDeviceAddressParseXML(node, addr) < 0)
+ goto cleanup;
I'm not super convinced about this.
On one hand, it makes it so callers can't possibly forget to parse
the zPCI part, and that's literally embedded in the PCI part now;
on the other hand, we have functions to verify separately whether
the PCI and zPCI parts are empty, which is pretty weird.
I guess we can leave as it is for now, but the semantics are muddy
enough that I can pretty much guarantee someone will have to clean
them up at some point down the line.
[...]
@@ -1044,6 +1044,9 @@
virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
dev->isolationGroup, function) < 0)
return -1;
+ if (dev->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI)
+ addr.zpci = dev->addr.pci.zpci;
I'm confused by this part. Shouldn't this either request a new set
of zPCI ids, the same way it's allocating a new PCI address right
above, or do nothing at all because zPCI address allocation is
handled by later calling virDomainZPCIAddressReserveNextAddr()?
This is basically another manifestation of the issue I mentioned
above: we don't seem to have a well-defined and strictly adhered
to idea of how the PCI part and zPCI part should relate to each
other, so they're either considered separate entities or part of
the same entity depending on which APIs you're looking at.
[...]
+ if
(!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) {
+ virBufferAsprintf(buf, " uid='0x%.4x'",
+ info->addr.pci.zpci.uid);
+ virBufferAsprintf(buf, " fid='0x%.8x'",
+ info->addr.pci.zpci.fid);
You only need a single call to virBufferAsprintf() here.
--
Andrea Bolognani / Red Hat / Virtualization