Re: [RFC: vf-token 4/5] conf: vf-token parsing and formatting

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> 




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux