On 13.02.24 14:14, Anthony PERARD wrote:
Hello Anthony > 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://urldefense.com/v3/__https://github.com/xen-project/xen/commit/c14254065ff4826e34f714e1790eab5217368c38__;!!GF_29dbcQIUBPA!172qt30uI7DpsZcK-IpZu25JoQScPeF7do_MD-KEqJlo8gaH-TV1P_H2CYfsGI0j_l0GdUPPO4BjyUD2Q86Lk2IlF5zCiFWMAw$ >> [github[.]com] >> >> 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. Thanks. As V3 is needed anyway, I will include them. > >> --- >> 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. It is not very useful, will drop the log. > >> + 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()? Will do. > > Thanks, >
