Re: [PATCH 07/14] tools/xl: Use const whenever we point to literal strings

[Julien Grall <julien@xxxxxxx>]

Hi Anthony,

On 27/04/2021 17:04, Anthony PERARD wrote:
> On Mon, Apr 05, 2021 at 04:57:06PM +0100, Julien Grall wrote:
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > literal strings are not meant to be modified. So we should use const
> > char * rather than char * when we want to store a pointer to them.
> >
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> > ---
> > diff --git a/tools/xl/xl.h b/tools/xl/xl.h
> > index 137a29077c1e..3052e3db0072 100644
> > --- a/tools/xl/xl.h
> > +++ b/tools/xl/xl.h
> > @@ -21,13 +21,13 @@
> >   #include <xentoollog.h>
> >   
> >   struct cmd_spec {
> > -    char *cmd_name;
> > +    const char *cmd_name;
> >       int (*cmd_impl)(int argc, char **argv);
> >       int can_dryrun;
> >       int modifies;
> > -    char *cmd_desc;
> > -    char *cmd_usage;
> > -    char *cmd_option;
> > +    const char *cmd_desc;
> > +    const char *cmd_usage;
> > +    const char *cmd_option;
> >   };
> Those const in cmd_spec feels almost the wrong things to do, but it is
> also unlikely that we would want to modify the strings in a cmd_spec so
> I guess that's fine.
May I ask why you think it feels wrong things to do?

Using char * to point to literal string is a recipe for disaster because 
the compiler will not warn you if you end up to write in them. Instead, 
you will get a runtime error. xl only deals with a single domain, so the 
consequences will not be too bad, but for other piece of the userspace 
(e.g. libxl, xenstored) this would be a nice host DoS.
Both GCC and Clang provide an option (see -Wwrite-strings) to throw an 
error at compile time if char * points to literal string. I would like 
to enable it because it will harden our code.
The price to pay to use const char * for some fo the field. This is not 
that bad compare to the other options (e.g strdup(), casting...) or the 
potential fallout with the existing code.
> I'm thinking that `cmd_table` should be const as well (I mean
> const struct cmd_spec cmd_table[];) as there is no reason to modify the
> entries once they are in the table. I'll send a patch.
>
>
> The rest looks good.
> Reviewed-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
Thanks.

Cheers,

--
Julien Grall