Re: [libvirt] [PATCH v5 07/13] conf: Introduce parser, formatter for uid and fid
在 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