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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

Thanks, Roger.