在 2018/9/11 下午8:05, Andrea Bolognani 写道:
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. OK.
[...]
+ 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. ditto
[...]
+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 :) ditto. I'm very glad that you could give me so many valuable comments even if it's just about code style.
[...]
+ 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. OK.
+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(). I'm not very clear with your comment. Do you mean I should move it to other place?
[...]
@@ -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. It's weird indeed. But if we merge zPCI part check into PCI code. We might have to merge other code. But PCI address has extension only on S390 at least now.
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. OK.
[...]
@@ -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()? Here, we want to store the defined zPCI address which has been reserved. If we don't do this, we might lose zPCI address and do reservation again but the reserved one are still existing in zPCI set.
For this problem, I once thought about adding extension address into DeviceInfo next to PCI address embraced in a struct. Then here is not a problem.
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.
I think the above idea I thought about before is like what you said?
[...]
+ 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.
OK. -- Yi Min