On Tue, Feb 06, 2024 at 02:38:14PM +0200, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
