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

[Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>]

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 <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://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,
>