Re: [libvirt] [PATCH v6 07/13] conf: Introduce parser, formatter for uid and fid
在 2018/10/11 下午7:45, 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(a)linux.ibm.com>
> ---
No internal reviews this time around? :)
Yes, I just want to quickly know if this
change is exact.
[...]
> @@ -227,9 +288,22 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
> goto cleanup;
>
> }
> +
> if (!virPCIDeviceAddressIsEmpty(addr) &&
!virPCIDeviceAddressIsValid(addr, true))
> goto cleanup;
Spurious whitespace change.
Oh...good catch.
[...]
> @@ -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.
Sure.
[...]
> @@ -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*().
It has been moved to
util/virpci. Isn't it?
[...]
> @@ -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().
Sure.
Everything else looks good.
--
Yi Min