Re: [PATCH V2] libxl: Add "grant_usage" parameter for virtio disk devices

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Tue, Feb 06, 2024 at 02:38:14PM +0200, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> 
> Allow administrators to control whether Xen grant mappings for
> the virtio disk devices should be used. By default (when new
> parameter is not specified), the existing behavior is retained
> (we enable grants if backend-domid != 0).
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> ---
> In addition to "libxl: arm: Add grant_usage parameter for virtio devices"
> https://github.com/xen-project/xen/commit/c14254065ff4826e34f714e1790eab5217368c38
> 
> I wonder, whether I had to also include autogenerated changes to:
>  - tools/libs/util/libxlu_disk_l.c
>  - tools/libs/util/libxlu_disk_l.h

Well, that could be done on commit. The changes are going to be needed
to be committed as they may not be regenerated to include the new feature
in a build.

> ---
> diff --git a/tools/libs/light/libxl_disk.c b/tools/libs/light/libxl_disk.c
> index ea3623dd6f..ed02b655a3 100644
> --- a/tools/libs/light/libxl_disk.c
> +++ b/tools/libs/light/libxl_disk.c
> @@ -623,6 +628,15 @@ static int libxl__disk_from_xenstore(libxl__gc *gc, 
> const char *libxl_path,
>              goto cleanup;
>          }
>          disk->irq = strtoul(tmp, NULL, 10);
> +
> +        tmp = libxl__xs_read(gc, XBT_NULL,
> +                             GCSPRINTF("%s/grant_usage", libxl_path));
> +        if (!tmp) {
> +            LOG(DEBUG, "Missing xenstore node %s/grant_usage, using default 
> value", libxl_path);

Is this information useful for debugging?

It should be easy to find out if the grant_usage node is present or not
by looking at xenstore, and I don't think libxl is going to make use of
that information after this point, so I don't think that's going to be
very useful.

> +            libxl_defbool_set(&disk->grant_usage,
> +                              disk->backend_domid != LIBXL_TOOLSTACK_DOMID);
> +        } else
> +            libxl_defbool_set(&disk->grant_usage, strtoul(tmp, NULL, 0));

Per coding style, it's better to have both side of an if..else to have
{}-block or none of them. So could you add a {} block in the else, or
remove the {} from the true side if we remove the LOG()?

Thanks,

-- 
Anthony PERARD