Re: [PATCH] tools/libxl: remove usage of VLA arrays

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.10.2024 18:26, Roger Pau Monné wrote:
> On Mon, Oct 28, 2024 at 12:57:30PM +0000, Frediano Ziglio wrote:
>> On Mon, Oct 28, 2024 at 12:48 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>
>>> On 28.10.2024 13:03, Andrew Cooper wrote:
>>>> On 28/10/2024 11:48 am, Roger Pau Monne wrote:
>>>>> Clang 19 complains with the following error when building libxl:
>>>>>
>>>>> libxl_utils.c:48:15: error: variable length array folded to constant 
>>>>> array as an extension [-Werror,-Wgnu-folding-constant]
>>>>>    48 |     char path[strlen("/local/domain") + 12];
>>>>>       |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>
>>>>> Replace the usage of strlen() with ARRAY_SIZE(), which allows the literal
>>>>> string length to be known at build time.  Note ARRAY_SIZE() accounts for 
>>>>> the
>>>>> NUL terminator while strlen() didn't, hence subtract 1 from the total size
>>>>> calculation.
>>>>>
>>>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>>>> ---
>>>>>  tools/libs/light/libxl_utils.c | 4 ++--
>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/tools/libs/light/libxl_utils.c 
>>>>> b/tools/libs/light/libxl_utils.c
>>>>> index 10398a6c8611..b3f5e751cc3f 100644
>>>>> --- a/tools/libs/light/libxl_utils.c
>>>>> +++ b/tools/libs/light/libxl_utils.c
>>>>> @@ -45,7 +45,7 @@ unsigned long libxl_get_required_shadow_memory(unsigned 
>>>>> long maxmem_kb, unsigned
>>>>>  char *libxl_domid_to_name(libxl_ctx *ctx, uint32_t domid)
>>>>>  {
>>>>>      unsigned int len;
>>>>> -    char path[strlen("/local/domain") + 12];
>>>>> +    char path[ARRAY_SIZE("/local/domain") + 11];
>>>>>      char *s;
>>>>>
>>>>>      snprintf(path, sizeof(path), "/local/domain/%d/name", domid);
>>>>> @@ -141,7 +141,7 @@ int libxl_cpupool_qualifier_to_cpupoolid(libxl_ctx 
>>>>> *ctx, const char *p,
>>>>>  char *libxl_cpupoolid_to_name(libxl_ctx *ctx, uint32_t poolid)
>>>>>  {
>>>>>      unsigned int len;
>>>>> -    char path[strlen("/local/pool") + 12];
>>>>> +    char path[ARRAY_SIZE("/local/pool") + 11];
>>>>>      char *s;
>>>>>
>>>>>      snprintf(path, sizeof(path), "/local/pool/%d/name", poolid);
>>>>
>>>> Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>
>>>> Although I have a minor preference for sizeof() as suggested by Frediano.
>>>>
>>>> Can fix on commit, if you're happy?
>>>
>>> Please can we stick to ARRAY_SIZE() when it comes to strings? It's the
>>> same as sizeof() when the base type is char, but the difference becomes
>>> relevant if the base type was e.g. wchar_t.
>>>
>>> Jan
>>>
>>
>> But "literal" is not a wide string, and the type is "char" which is
>> not wide too.
>>
>> BTW, both me and Andrew are not strong about.
> 
> No strong opinion either, I've assumed it was clearer to not make
> implicit assumptions about the size of the string literal array
> elements.  I would rather like to get this committed, and Jan seems to
> prefer to use ARRAY_SIZE(), so I suggest we get the patch committed
> as-is.

Well, ultimately it's Anthony's call, for still being the sole maintainer.

Jan