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@linux.intel.com> --- 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