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

[Andrea Bolognani]

On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote:
> This patch introduces new XML parser/formatter functions. Uid is
> 16-bit and non-zero. Fid is 32-bit. They are the two attributes of zpci
> which is introduced as PCI address element. Zpci element is parsed and
> formatted along with PCI address. And add the related test cases.
> 
> Signed-off-by: Yi Min Zhao <zyimin at linux.ibm.com>
> ---

No internal reviews this time around? :)

[...]
> @@ -227,9 +288,22 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
>          goto cleanup;
>  
>      }
> +
>      if (!virPCIDeviceAddressIsEmpty(addr) && !virPCIDeviceAddressIsValid(addr, true))
>          goto cleanup;

Spurious whitespace change.

[...]
> @@ -6528,7 +6528,19 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
>          break;
>      }
>  
> -    virBufferAddLit(buf, "/>\n");
> +    if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> +        !virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci)) {
> +        virBufferAddLit(buf, ">\n");
> +        virBufferAdjustIndent(buf, 2);
> +        virBufferAsprintf(buf,
> +                          "<zpci uid='0x%.4x' fid='0x%.8x'/>\n",
> +                          info->addr.pci.zpci.uid,
> +                          info->addr.pci.zpci.fid);
> +        virBufferAdjustIndent(buf, -2);
> +        virBufferAddLit(buf, "</address>\n");
> +    } else {
> +        virBufferAddLit(buf, "/>\n");
> +    }

This looks like a perfect candidate for virXMLFormatElement(). You
should probably convert the original code to use that function in a
separate commit, then start actually using a child buffer here.

[...]
> @@ -2566,6 +2566,7 @@ virPCIHeaderTypeToString;
>  virPCIIsVirtualFunction;
>  virPCIStubDriverTypeFromString;
>  virPCIStubDriverTypeToString;
> +virZPCIDeviceAddressIsEmpty;

You can move virZPCIDeviceAddressIsValid() to util/virpci and
export it too, to be consistent with virPCIDeviceAddress*().

[...]
> @@ -272,4 +275,6 @@ VIR_DEFINE_AUTOPTR_FUNC(virPCIDevice, virPCIDeviceFree)
>  VIR_DEFINE_AUTOPTR_FUNC(virPCIDeviceAddress, virPCIDeviceAddressFree)
>  VIR_DEFINE_AUTOPTR_FUNC(virPCIEDeviceInfo, virPCIEDeviceInfoFree)
>  
> +bool virZPCIDeviceAddressIsEmpty(const virZPCIDeviceAddress *addr);
> +

Please place this further up in the file, eg. after
virPCIDeviceAddressParse().


Everything else looks good.

-- 
Andrea Bolognani / Red Hat / Virtualization