On Thu, Oct 05, 2023 at 16:05:04 -0700, Vivek Kashyap wrote:
> Introduce a validation function for vf-token support in qemu
> and generate vf-token device attribute in qmeu command line.
>
> Signed-off-by: Vivek Kashyap <vivek.kashyap@xxxxxxxxxxxxxxx>
> ---
> src/qemu/qemu_command.c | 27 ++++++++++++++++++++++++---
> src/qemu/qemu_validate.c | 19 +++++++++++++++++++
> 2 files changed, 43 insertions(+), 3 deletions(-)
Missing qemuxml2argvtest and qemuxml2xmtest additions, which are
absolutely required for anything that adds/changes the commandline syntax.
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 8a7b80719f..91d7836f5c 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1979,7 +1979,6 @@ qemuBuildZPCIDevProps(virDomainDeviceInfo *dev)
> return props;
> }
>
> -
Spurious whitespace change. As noted we do 2 lines between functions.
> static int
> qemuCommandAddExtDevice(virCommand *cmd,
> virDomainDeviceInfo *dev,
> @@ -4729,6 +4728,8 @@ qemuBuildPCIHostdevDevProps(const virDomainDef *def,
> pcisrc->addr.slot,
> pcisrc->addr.function);
> const char *failover_pair_id = NULL;
> + g_autofree char *token = NULL;
> + int ret = 0;
>
> /* caller has to assign proper passthrough backend type */
> switch (pcisrc->backend) {
> @@ -4755,13 +4756,33 @@ qemuBuildPCIHostdevDevProps(const virDomainDef *def,
> teaming->persistent)
> failover_pair_id = teaming->persistent;
>
> - if (virJSONValueObjectAdd(&props,
> + if ((dev->info->pciAddrExtFlags & VIR_PCI_ADDRESS_EXTENSION_VFTOKEN) &&
> + pcisrc->addr.token.isSet) {
Broken alignment/code style.
> + const unsigned char *uuid = pcisrc->addr.token.uuid;
> +
> + token = g_strdup_printf(VIR_PCI_DEVICE_TOKEN_FMT,
> + uuid[0], uuid[1], uuid[2], uuid[3],
> + uuid[4], uuid[5], uuid[6], uuid[7],
> + uuid[8], uuid[9], uuid[10], uuid[11],
> + uuid[12], uuid[13], uuid[14], uuid[15]);
Broken alignment/code style.
> +
> + ret = virJSONValueObjectAdd(&props,
> "s:driver", "vfio-pci",
> "s:host", host,
> + "s:vf-token", token,
> "s:id", dev->info->alias,
> "p:bootindex", dev->info->effectiveBootIndex,
> "S:failover_pair_id", failover_pair_id,
> - NULL) < 0)
> + NULL);
So firstly you break the code style here by your addition.
Secondly there's no need to duplicate the whole virJSONValueObjectAdd
block in the first place as we allow conditional formatting of fields.
You need to fill the 'token' field only optionally what you do, but then
you can unconditionally format it using:
"S:vf-token", token,
argument tuple in virJSONValueObjectAdd. The 'S' modifier formats the
attribute only if the string argument is non-NULL.
> + } else
Trailing whitespace, but this block is not needed ... and in fact not
desired as described above.
> + ret = virJSONValueObjectAdd(&props,
> + "s:driver", "vfio-pci",
> + "s:host", host,
> + "s:id", dev->info->alias,
> + "p:bootindex", dev->info->effectiveBootIndex,
> + "S:failover_pair_id", failover_pair_id,
> + NULL);
> + if (ret < 0)
> return NULL;
>
> if (qemuBuildDeviceAddressProps(props, def, dev->info) < 0)
