On Thu, Oct 05, 2023 at 16:05:03 -0700, Vivek Kashyap wrote:
> Introduce XML parsing and formatting of vf-token attribute
>
> Signed-off-by: Vivek Kashyap <vivek.kashyap@xxxxxxxxxxxxxxx>
> ---
> src/conf/device_conf.c | 31 +++++++++++++++++++++++++++++--
> src/conf/domain_conf.c | 5 +++++
> src/conf/schemas/basictypes.rng | 11 +++++++++++
> src/conf/schemas/domaincommon.rng | 1 +
> src/libvirt_private.syms | 1 +
> src/util/virpci.c | 5 +++++
> 6 files changed, 52 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
> index f3d977f2b7..d5ed4ef452 100644
> --- a/src/conf/device_conf.c
> +++ b/src/conf/device_conf.c
> @@ -188,11 +188,20 @@ virDeviceInfoPCIAddressExtensionIsWanted(const virDomainDeviceInfo *info)
> virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci);
> }
>
> +bool
> +virDeviceExtensionIsPresent(const virPCIDeviceAddress *pci)
> +{
> + return ((pci->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
> + virZPCIDeviceAddressIsPresent(&pci->zpci)) ||
> + ((pci->extFlags & VIR_PCI_ADDRESS_EXTENSION_VFTOKEN) &&
> + pci->token.isSet);
The code alignment is broken here. Also add a function documentation
describing what this function does as it's really not obvious.
> +}
> +
Code style dictates two empty lines between functions.
> bool
> virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info)
> {
> - return (info->addr.pci.extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
> - virZPCIDeviceAddressIsPresent(&info->addr.pci.zpci);
> + return (info->addr.pci.extFlags != VIR_PCI_ADDRESS_EXTENSION_NONE) &&
> + virDeviceExtensionIsPresent(&info->addr.pci);
This change is suspicious and should be justified. I'm not going to
comment on whether it's correct or not.
> }
>
> int
> @@ -200,6 +209,7 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
> virPCIDeviceAddress *addr)
> {
> xmlNodePtr zpci;
> + xmlNodePtr token;
>
> memset(addr, 0, sizeof(*addr));
>
> @@ -231,6 +241,10 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
> return -1;
> }
>
> + if ((token = virXMLNodeGetSubelement(node, "vf-token")))
Multi-line condition bodies dictate use of a { } block around it.
> + if (virPCIDeviceTokenParseXML(token, addr) < 0)
> + return -1;
This one is okay though.
> +
> return 0;
> }
>
> @@ -248,6 +262,19 @@ virPCIDeviceAddressFormat(virBuffer *buf,
> addr.function);
> }
>
> +int
> +virPCIDeviceTokenParseXML(xmlNodePtr node,
> + virPCIDeviceAddress *addr)
> +{
> + if (virXMLPropUUID(node, "uuid", VIR_XML_PROP_NONE,
> + addr->token.uuid) < 0)
virXMLPropUUID parses into a buffer which is not a string (NUL-byte
terminated) but a byte array ...
> + return -1;
> +
> + addr->token.isSet = 1;
> +
> + return 0;
> +}
> +
Two empty lines between functions.
> int
> virCCWDeviceAddressParseXML(xmlNodePtr node,
> virCCWDeviceAddress *addr)
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 4435ee2ad4..cceb1d3b83 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5389,6 +5389,11 @@ virDomainDeviceInfoFormat(virBuffer *buf,
> info->addr.pci.zpci.uid.value,
> info->addr.pci.zpci.fid.value);
> }
> + if (virPCIVFIOTokenIDIsPresent(&info->addr.pci.token)) {
> + virBufferAsprintf(&childBuf,
> + "<vf-token uuid='%s'/>\n",
> + info->addr.pci.token.uuid);
... but here you use a *string* formatting function. So this will either
not print any reasonable output or potentially crash if the UUID
contains no NUL-bytes.
You would figure this out if you'd add any tests for this feature which
is in fact required. It is okay to add the tests at the end though.
> + }
> break;
>
> case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE:
> diff --git a/src/conf/schemas/basictypes.rng b/src/conf/schemas/basictypes.rng
> index 26eb538077..bf2c30dd18 100644
> --- a/src/conf/schemas/basictypes.rng
> +++ b/src/conf/schemas/basictypes.rng
> @@ -138,6 +138,17 @@
> </element>
> </optional>
> </define>
> + <define name="vf-tokenid">
> + <optional>
> + <element name="vf-token">
> + <optional>
> + <attribute name="uuid">
> + <ref name="UUID"/>
> + </attribute>
> + </optional>
> + </element>
> + </optional>
> + </define>
>
> <!-- a 6 byte MAC address in ASCII-hex format, eg "12:34:56:78:9A:BC" -->
> <!-- The lowest bit of the 1st byte is the "multicast" bit. a -->
> diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng
> index a26986b5ce..b228a3ca73 100644
> --- a/src/conf/schemas/domaincommon.rng
> +++ b/src/conf/schemas/domaincommon.rng
> @@ -7034,6 +7034,7 @@
> </attribute>
> <ref name="pciaddress"/>
> <ref name="zpciaddress"/>
> + <ref name="vf-tokenid"/>
> </group>
> <group>
> <attribute name="type">
Any XML addition _must_ come with correspondign documentation update.
Please document the new flag and how it's supposed to be used in
docs/formatdomain.rst.
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 4e475d5b1a..0d4866cd56 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -3138,6 +3138,7 @@ virPCIHeaderTypeToString;
> virPCIIsVirtualFunction;
> virPCIStubDriverTypeFromString;
> virPCIStubDriverTypeToString;
> +virPCIVFIOTokenIDIsPresent;
> virPCIVirtualFunctionListFree;
> virZPCIDeviceAddressIsIncomplete;
> virZPCIDeviceAddressIsPresent;
This hunk and all others most likely belong to the patch which was
defining the function or vice-versa.
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 08b82708b1..dfb0f29182 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -2312,6 +2312,11 @@ virZPCIDeviceAddressIsPresent(const virZPCIDeviceAddress *addr)
> return addr->uid.isSet || addr->fid.isSet;
> }
>
> +bool
> +virPCIVFIOTokenIDIsPresent(const virPCIDeviceToken *token)
> +{
> + return token->isSet;
> +}
>
> void
> virPCIVirtualFunctionListFree(virPCIVirtualFunctionList *list)
> --
> 2.25.1
>
