[libvirt] [PATCH v5 07/13] conf: Introduce parser, formatter for uid and fid

[Yi Min Zhao]

在 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