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 <jgr...@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 <jgr...@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.per...@citrix.com>
Thanks.
Cheers,
--
Julien Grall
