| Message ID | 20210405155713.29754-8-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use const whether we point to literal strings (take 1) | expand |
On Mon, Apr 05, 2021 at 04:57:06PM +0100, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > 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@amazon.com> > --- > 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. 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@citrix.com> Thanks,
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@amazon.com> >> >> 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@amazon.com> >> --- >> 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@citrix.com> Thanks. Cheers,
On Tue, Apr 27, 2021 at 05:28:30PM +0100, Julien Grall wrote: > On 27/04/2021 17:04, Anthony PERARD wrote: > > On Mon, Apr 05, 2021 at 04:57:06PM +0100, Julien Grall wrote: > > > 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 [...] Well, they are no literal string here as we only describe a struct, and I though that having "const struct cmd_spec" would have been enough. But I gave a try with only "const struct" and found that the strings could be modified. So I don't have anything anymore to say about the patch, and "const char" in the "struct" are necessary. I just need to fix my intuition about how const works in C, even if I already know the rule about it. Cheers,
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; }; struct domain_create { diff --git a/tools/xl/xl_console.c b/tools/xl/xl_console.c index 4e65d7386733..b27f9e013697 100644 --- a/tools/xl/xl_console.c +++ b/tools/xl/xl_console.c @@ -27,7 +27,7 @@ int main_console(int argc, char **argv) uint32_t domid; int opt = 0, num = 0; libxl_console_type type = 0; - char *console_names = "pv, serial, vuart"; + const char *console_names = "pv, serial, vuart"; SWITCH_FOREACH_OPT(opt, "n:t:", NULL, "console", 1) { case 't': diff --git a/tools/xl/xl_utils.c b/tools/xl/xl_utils.c index 4503ac7ea03c..17489d182954 100644 --- a/tools/xl/xl_utils.c +++ b/tools/xl/xl_utils.c @@ -27,7 +27,7 @@ #include "xl.h" #include "xl_utils.h" -void dolog(const char *file, int line, const char *func, char *fmt, ...) +void dolog(const char *file, int line, const char *func, const char *fmt, ...) { va_list ap; char *s = NULL; @@ -248,7 +248,7 @@ void print_bitmap(uint8_t *map, int maplen, FILE *stream) } } -int do_daemonize(char *name, const char *pidfile) +int do_daemonize(const char *name, const char *pidfile) { char *fullname; pid_t child1; diff --git a/tools/xl/xl_utils.h b/tools/xl/xl_utils.h index d98b419f1075..0c337ede954b 100644 --- a/tools/xl/xl_utils.h +++ b/tools/xl/xl_utils.h @@ -123,7 +123,7 @@ int def_getopt(int argc, char * const argv[], const struct option *longopts, const char* helpstr, int reqargs); -void dolog(const char *file, int line, const char *func, char *fmt, ...) +void dolog(const char *file, int line, const char *func, const char *fmt, ...) __attribute__((format(printf,4,5))); void xvasprintf(char **strp, const char *fmt, va_list ap) @@ -143,7 +143,7 @@ uint32_t find_domain(const char *p) __attribute__((warn_unused_result)); void print_bitmap(uint8_t *map, int maplen, FILE *stream); -int do_daemonize(char *name, const char *pidfile); +int do_daemonize(const char *name, const char *pidfile); #endif /* XL_UTILS_H */ /*