[PATCH 00/14] Use const whether we point to literal strings (take 1)

[PATCH 00/14] Use const whether we point to literal strings (take 1)

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Hi all,

By default, both Clang and GCC will happily compile C code where
non-const char * point to literal strings. This means the following
code will be accepted:

    char *str = "test";

    str[0] = 'a';

Literal strings will reside in rodata, so they are not modifiable.
This will result to an permission fault at runtime if the permissions
are enforced in the page-tables (this is the case in Xen).

I am not aware of code trying to modify literal strings in Xen.
However, there is a frequent use of non-const char * to point to
literal strings. Given the size of the codebase, there is a risk
to involuntarily introduce code that will modify literal strings.

Therefore it would be better to enforce using const when pointing
to such strings. Both GCC and Clang provide an option to warn
for such case (see -Wwrite-strings) and therefore could be used
by Xen.

This series doesn't yet make use of -Wwrite-strings because
the tree is not fully converted. Instead, it contains some easy
and likely non-controversial use const in the code.

The major blockers to enable -Wwrite-strings are the following:
    - xen/common/efi: union string is used in both const and
    non-const situation. It doesn't feel right to specific one member
    const and the other non-const.
    - libxl: the major block is the flexarray framework as we would use
    it with string (now const char*). I thought it would be possible to
    make the interface const, but it looks like there are a couple of
    places where we need to modify the content (such as in
    libxl_json.c).

Ideally, I would like to have -Wwrite-strings unconditionally used
tree-wide. But, some of the area may required some heavy refactoring.

One solution would be to enable it tree-wide but turned it off at a
directroy/file level.

Any opinions?

Cheers,

Julien Grall (14):
  xen: Constify the second parameter of rangeset_new()
  xen/sched: Constify name and opt_name in struct scheduler
  xen/x86: shadow: The return type of sh_audit_flags() should be const
  xen/char: console: Use const whenever we point to literal strings
  tools/libs: guest: Use const whenever we point to literal strings
  tools/libs: stat: Use const whenever we point to literal strings
  tools/xl: Use const whenever we point to literal strings
  tools/firmware: hvmloader: Use const in __bug() and __assert_failed()
  tools/console: Use const whenever we point to literal strings
  tools/kdd: Use const whenever we point to literal strings
  tools/misc: Use const whenever we point to literal strings
  tools/top: The string parameter in set_prompt() and set_delay() should
    be const
  tools/xenmon: xenbaked: Mark const the field text in stat_map_t
  tools/xentrace: Use const whenever we point to literal strings

 tools/console/client/main.c         |  4 +-
 tools/console/daemon/io.c           | 10 ++--
 tools/debugger/kdd/kdd.c            | 10 ++--
 tools/firmware/hvmloader/util.c     |  4 +-
 tools/firmware/hvmloader/util.h     |  4 +-
 tools/include/xenguest.h            | 10 ++--
 tools/libs/guest/xg_dom_core.c      |  8 ++--
 tools/libs/guest/xg_dom_elfloader.c |  4 +-
 tools/libs/guest/xg_dom_hvmloader.c |  2 +-
 tools/libs/guest/xg_dom_x86.c       |  9 ++--
 tools/libs/guest/xg_private.h       |  2 +-
 tools/libs/stat/xenstat_linux.c     |  4 +-
 tools/libs/stat/xenstat_qmp.c       | 12 ++---
 tools/misc/xen-detect.c             |  2 +-
 tools/misc/xenhypfs.c               |  6 +--
 tools/xenmon/xenbaked.c             |  2 +-
 tools/xentop/xentop.c               | 12 ++---
 tools/xentrace/xenalyze.c           | 71 +++++++++++++++--------------
 tools/xentrace/xenctx.c             |  4 +-
 tools/xl/xl.h                       |  8 ++--
 tools/xl/xl_console.c               |  2 +-
 tools/xl/xl_utils.c                 |  4 +-
 tools/xl/xl_utils.h                 |  4 +-
 xen/arch/x86/mm/shadow/multi.c      | 12 ++---
 xen/common/rangeset.c               |  2 +-
 xen/common/sched/private.h          |  4 +-
 xen/drivers/char/console.c          |  4 +-
 xen/include/xen/rangeset.h          |  2 +-
 28 files changed, 113 insertions(+), 109 deletions(-)

-- 
2.17.1

[PATCH 01/14] xen: Constify the second parameter of rangeset_new()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

The string 'name' will never get modified by the function, so mark it
as const.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/common/rangeset.c      | 2 +-
 xen/include/xen/rangeset.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c
index 4ebba30ba303..d997d7bce9e2 100644
--- a/xen/common/rangeset.c
+++ b/xen/common/rangeset.c
@@ -421,7 +421,7 @@ bool_t rangeset_is_empty(
 }
 
 struct rangeset *rangeset_new(
-    struct domain *d, char *name, unsigned int flags)
+    struct domain *d, const char *name, unsigned int flags)
 {
     struct rangeset *r;
 
diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h
index 5f62a9797170..135f33f6066f 100644
--- a/xen/include/xen/rangeset.h
+++ b/xen/include/xen/rangeset.h
@@ -36,7 +36,7 @@ void rangeset_domain_destroy(
  * rangeset_destroy(r).
  */
 struct rangeset *rangeset_new(
-    struct domain *d, char *name, unsigned int flags);
+    struct domain *d, const char *name, unsigned int flags);
 void rangeset_destroy(
     struct rangeset *r);
 
-- 
2.17.1

[PATCH 02/14] xen/sched: Constify name and opt_name in struct scheduler

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Both name and opt_name are pointing to literal string. So mark both of
the fields as const.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/common/sched/private.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h
index 92d0d4961063..a870320146ef 100644
--- a/xen/common/sched/private.h
+++ b/xen/common/sched/private.h
@@ -272,8 +272,8 @@ static inline spinlock_t *pcpu_schedule_trylock(unsigned int cpu)
 }
 
 struct scheduler {
-    char *name;             /* full name for this scheduler      */
-    char *opt_name;         /* option name for this scheduler    */
+    const char *name;       /* full name for this scheduler      */
+    const char *opt_name;   /* option name for this scheduler    */
     unsigned int sched_id;  /* ID for this scheduler             */
     void *sched_data;       /* global data pointer               */
     struct cpupool *cpupool;/* points to this scheduler's pool   */
-- 
2.17.1

[PATCH 03/14] xen/x86: shadow: The return type of sh_audit_flags() should be const

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

The function sh_audit_flags() is returning pointer to literal strings.
They should not be modified, so the return is now const and this is
propagated to the callers.

Take the opportunity to fix the coding style in the declaration of
sh_audit_flags.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/x86/mm/shadow/multi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 9b43cb116c47..0342de81d2c1 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -4290,8 +4290,8 @@ static void sh_pagetable_dying(paddr_t gpa)
     done = 1;                                                           \
 } while (0)
 
-static char * sh_audit_flags(struct vcpu *v, int level,
-                              int gflags, int sflags)
+static const char *sh_audit_flags(struct vcpu *v, int level,
+                                  int gflags, int sflags)
 /* Common code for auditing flag bits */
 {
     if ( (sflags & _PAGE_PRESENT) && !(gflags & _PAGE_PRESENT) )
@@ -4324,7 +4324,7 @@ int sh_audit_l1_table(struct vcpu *v, mfn_t sl1mfn, mfn_t x)
     mfn_t mfn, gmfn, gl1mfn;
     gfn_t gfn;
     p2m_type_t p2mt;
-    char *s;
+    const char *s;
     int done = 0;
 
     /* Follow the backpointer */
@@ -4419,7 +4419,7 @@ int sh_audit_l2_table(struct vcpu *v, mfn_t sl2mfn, mfn_t x)
     mfn_t mfn, gmfn, gl2mfn;
     gfn_t gfn;
     p2m_type_t p2mt;
-    char *s;
+    const char *s;
     int done = 0;
 
     /* Follow the backpointer */
@@ -4471,7 +4471,7 @@ int sh_audit_l3_table(struct vcpu *v, mfn_t sl3mfn, mfn_t x)
     mfn_t mfn, gmfn, gl3mfn;
     gfn_t gfn;
     p2m_type_t p2mt;
-    char *s;
+    const char *s;
     int done = 0;
 
     /* Follow the backpointer */
@@ -4521,7 +4521,7 @@ int sh_audit_l4_table(struct vcpu *v, mfn_t sl4mfn, mfn_t x)
     mfn_t mfn, gmfn, gl4mfn;
     gfn_t gfn;
     p2m_type_t p2mt;
-    char *s;
+    const char *s;
     int done = 0;
 
     /* Follow the backpointer */
-- 
2.17.1

[PATCH 04/14] xen/char: console: Use const whenever we point to literal strings

[Julien Grall]

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>
---
 xen/drivers/char/console.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 23583751709c..72a7cd1c32c1 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -168,7 +168,7 @@ static int parse_guest_loglvl(const char *s);
 static char xenlog_val[LOGLVL_VAL_SZ];
 static char xenlog_guest_val[LOGLVL_VAL_SZ];
 
-static char *lvl2opt[] = { "none", "error", "warning", "info", "all" };
+static const char *lvl2opt[] = { "none", "error", "warning", "info", "all" };
 
 static void xenlog_update_val(int lower, int upper, char *val)
 {
@@ -262,7 +262,7 @@ static int parse_guest_loglvl(const char *s)
     return ret;
 }
 
-static char *loglvl_str(int lvl)
+static const char *loglvl_str(int lvl)
 {
     switch ( lvl )
     {
-- 
2.17.1

[PATCH 05/14] tools/libs: guest: Use const whenever we point to literal strings

[Julien Grall]

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>
---
 tools/include/xenguest.h            | 10 +++++-----
 tools/libs/guest/xg_dom_core.c      |  8 ++++----
 tools/libs/guest/xg_dom_elfloader.c |  4 ++--
 tools/libs/guest/xg_dom_hvmloader.c |  2 +-
 tools/libs/guest/xg_dom_x86.c       |  9 +++++----
 tools/libs/guest/xg_private.h       |  2 +-
 6 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index 217022b6e767..a4492038cf3a 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -108,7 +108,7 @@ struct xc_dom_image {
 
     /* info from (elf) kernel image */
     struct elf_dom_parms *parms;
-    char *guest_type;
+    const char *guest_type;
 
     /* memory layout */
     struct xc_dom_seg kernel_seg;
@@ -266,8 +266,8 @@ struct xc_dom_arch {
     /* arch-specific memory initialization. */
     int (*meminit) (struct xc_dom_image * dom);
 
-    char *guest_type;
-    char *native_protocol;
+    const char *guest_type;
+    const char *native_protocol;
     int page_shift;
     int sizeof_pfn;
     int p2m_base_supported;
@@ -374,9 +374,9 @@ char *xc_dom_strdup(struct xc_dom_image *dom, const char *str);
 
 /* --- alloc memory pool ------------------------------------------- */
 
-xen_pfn_t xc_dom_alloc_page(struct xc_dom_image *dom, char *name);
+xen_pfn_t xc_dom_alloc_page(struct xc_dom_image *dom, const char *name);
 int xc_dom_alloc_segment(struct xc_dom_image *dom,
-                         struct xc_dom_seg *seg, char *name,
+                         struct xc_dom_seg *seg, const char *name,
                          xen_vaddr_t start, xen_vaddr_t size);
 
 /* --- misc bits --------------------------------------------------- */
diff --git a/tools/libs/guest/xg_dom_core.c b/tools/libs/guest/xg_dom_core.c
index 98ef8e8fc9ca..4918ee517bdd 100644
--- a/tools/libs/guest/xg_dom_core.c
+++ b/tools/libs/guest/xg_dom_core.c
@@ -422,7 +422,7 @@ void *xc_dom_pfn_to_ptr_retcount(struct xc_dom_image *dom, xen_pfn_t pfn,
     struct xc_dom_phys *phys;
     xen_pfn_t offset;
     unsigned int page_shift = XC_DOM_PAGE_SHIFT(dom);
-    char *mode = "unset";
+    const char *mode = "unset";
 
     *count_out = 0;
 
@@ -525,7 +525,7 @@ void *xc_dom_pfn_to_ptr_retcount(struct xc_dom_image *dom, xen_pfn_t pfn,
     return phys->ptr;
 }
 
-static int xc_dom_chk_alloc_pages(struct xc_dom_image *dom, char *name,
+static int xc_dom_chk_alloc_pages(struct xc_dom_image *dom, const char *name,
                                   xen_pfn_t pages)
 {
     unsigned int page_size = XC_DOM_PAGE_SIZE(dom);
@@ -576,7 +576,7 @@ static int xc_dom_alloc_pad(struct xc_dom_image *dom, xen_vaddr_t boundary)
 }
 
 int xc_dom_alloc_segment(struct xc_dom_image *dom,
-                         struct xc_dom_seg *seg, char *name,
+                         struct xc_dom_seg *seg, const char *name,
                          xen_vaddr_t start, xen_vaddr_t size)
 {
     unsigned int page_size = XC_DOM_PAGE_SIZE(dom);
@@ -611,7 +611,7 @@ int xc_dom_alloc_segment(struct xc_dom_image *dom,
     return 0;
 }
 
-xen_pfn_t xc_dom_alloc_page(struct xc_dom_image *dom, char *name)
+xen_pfn_t xc_dom_alloc_page(struct xc_dom_image *dom, const char *name)
 {
     xen_vaddr_t start;
     xen_pfn_t pfn;
diff --git a/tools/libs/guest/xg_dom_elfloader.c b/tools/libs/guest/xg_dom_elfloader.c
index 06e713fe1119..0d6247db5d08 100644
--- a/tools/libs/guest/xg_dom_elfloader.c
+++ b/tools/libs/guest/xg_dom_elfloader.c
@@ -50,8 +50,8 @@ void xc_elf_set_logfile(xc_interface *xch, struct elf_binary *elf,
 
 /* ------------------------------------------------------------------------ */
 
-static char *xc_dom_guest_type(struct xc_dom_image *dom,
-                               struct elf_binary *elf)
+static const char *xc_dom_guest_type(struct xc_dom_image *dom,
+                                     struct elf_binary *elf)
 {
     uint64_t machine = elf_uval(elf, elf->ehdr, e_machine);
 
diff --git a/tools/libs/guest/xg_dom_hvmloader.c b/tools/libs/guest/xg_dom_hvmloader.c
index ec6ebad7fd52..4e6f30858a59 100644
--- a/tools/libs/guest/xg_dom_hvmloader.c
+++ b/tools/libs/guest/xg_dom_hvmloader.c
@@ -130,7 +130,7 @@ static elf_errorstatus xc_dom_parse_hvm_kernel(struct xc_dom_image *dom)
 
 static int module_init_one(struct xc_dom_image *dom,
                            struct xc_hvm_firmware_module *module,
-                           char *name)
+                           const char *name)
 {
     struct xc_dom_seg seg;
     void *dest;
diff --git a/tools/libs/guest/xg_dom_x86.c b/tools/libs/guest/xg_dom_x86.c
index 2953aeb90b35..e379b07f9945 100644
--- a/tools/libs/guest/xg_dom_x86.c
+++ b/tools/libs/guest/xg_dom_x86.c
@@ -1148,11 +1148,12 @@ static int vcpu_hvm(struct xc_dom_image *dom)
 
 /* ------------------------------------------------------------------------ */
 
-static int x86_compat(xc_interface *xch, uint32_t domid, char *guest_type)
+static int x86_compat(xc_interface *xch, uint32_t domid,
+                      const char *guest_type)
 {
     static const struct {
-        char           *guest;
-        uint32_t        size;
+        const char      *guest;
+        uint32_t       size;
     } types[] = {
         { "xen-3.0-x86_32p", 32 },
         { "xen-3.0-x86_64",  64 },
@@ -1664,7 +1665,7 @@ static int bootearly(struct xc_dom_image *dom)
 static int bootlate_pv(struct xc_dom_image *dom)
 {
     static const struct {
-        char *guest;
+        const char *guest;
         unsigned long pgd_type;
     } types[] = {
         { "xen-3.0-x86_32",  MMUEXT_PIN_L2_TABLE},
diff --git a/tools/libs/guest/xg_private.h b/tools/libs/guest/xg_private.h
index 8f9b257a2f3d..25e46d7ce195 100644
--- a/tools/libs/guest/xg_private.h
+++ b/tools/libs/guest/xg_private.h
@@ -43,7 +43,7 @@
 #endif
 
 struct xc_dom_loader {
-    char *name;
+    const char *name;
     /* Sadly the error returns from these functions are not consistent: */
     elf_negerrnoval (*probe) (struct xc_dom_image * dom);
     elf_negerrnoval (*parser) (struct xc_dom_image * dom);
-- 
2.17.1

[PATCH 06/14] tools/libs: stat: Use const whenever we point to literal strings

[Julien Grall]

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>
---
 tools/libs/stat/xenstat_linux.c |  4 ++--
 tools/libs/stat/xenstat_qmp.c   | 12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/libs/stat/xenstat_linux.c b/tools/libs/stat/xenstat_linux.c
index c00b26d4d898..875a0617ade8 100644
--- a/tools/libs/stat/xenstat_linux.c
+++ b/tools/libs/stat/xenstat_linux.c
@@ -66,7 +66,7 @@ static const char PROCNETDEV_HEADER[] =
 
 /* We need to get the name of the bridge interface for use with bonding interfaces */
 /* Use excludeName parameter to avoid adding bridges we don't care about, eg. virbr0 */
-static void getBridge(char *excludeName, char *result, size_t resultLen)
+static void getBridge(const char *excludeName, char *result, size_t resultLen)
 {
 	struct dirent *de;
 	DIR *d;
@@ -113,7 +113,7 @@ static int parseNetDevLine(char *line, char *iface, unsigned long long *rxBytes,
 	int num = 19;
 
 	/* Regular exception to parse all the information from /proc/net/dev line */
-	char *regex = "([^:]*):([^ ]*)[ ]*([^ ]*)[ ]*([^ ]*)[ ]*([^ ]*)[ ]*([^ ]*)[ ]*([^ ]*)"
+	const char *regex = "([^:]*):([^ ]*)[ ]*([^ ]*)[ ]*([^ ]*)[ ]*([^ ]*)[ ]*([^ ]*)[ ]*([^ ]*)"
 			"[ ]*([^ ]*)[ ]*([^ ]*)[ ]*([^ ]*)[ ]*([^ ]*)[ ]*([^ ]*)[ ]*([^ ]*)[ ]*"
 			"([^ ]*)[ ]*([^ ]*)[ ]*([^ ]*)[ ]*([^ ]*)[ ]*([^ ]*)";
 
diff --git a/tools/libs/stat/xenstat_qmp.c b/tools/libs/stat/xenstat_qmp.c
index 0c5748ba68b3..2205a041313b 100644
--- a/tools/libs/stat/xenstat_qmp.c
+++ b/tools/libs/stat/xenstat_qmp.c
@@ -38,7 +38,7 @@
 
 #include <yajl/yajl_tree.h>
 
-static unsigned char *qmp_query(int, char *);
+static unsigned char *qmp_query(int, const char *);
 
 enum query_blockstats {
     QMP_STATS_RETURN  = 0,
@@ -80,7 +80,7 @@ enum query_block {
 static char *qmp_get_block_image(xenstat_node *node, char *qmp_devname, int qfd)
 {
 	char *tmp, *file = NULL;
-	char *query_block_cmd = "{ \"execute\": \"query-block\" }";
+	const char *query_block_cmd = "{ \"execute\": \"query-block\" }";
 	static const char *const qblock[] = {
 		[ QMP_BLOCK_RETURN  ] = "return",
 		[ QMP_BLOCK_DEVICE  ] = "device",
@@ -264,7 +264,7 @@ done:
 }
 
 /* Write a command via the QMP. Returns number of bytes written */
-static size_t qmp_write(int qfd, char *cmd, size_t cmd_len)
+static size_t qmp_write(int qfd, const char *cmd, size_t cmd_len)
 {
 	size_t pos = 0;
 	ssize_t res;
@@ -317,7 +317,7 @@ static int qmp_read(int qfd, unsigned char **qstats)
 }
 
 /* With the given cmd, query QMP for requested data. Returns allocated buffer containing data or NULL */
-static unsigned char *qmp_query(int qfd, char *cmd)
+static unsigned char *qmp_query(int qfd, const char *cmd)
 {
 	unsigned char *qstats = NULL;
 	int n;
@@ -385,8 +385,8 @@ static int qmp_connect(char *path)
 */
 static void read_attributes_qdisk_dom(xenstat_node *node, domid_t domain)
 {
-	char *cmd_mode = "{ \"execute\": \"qmp_capabilities\" }";
-	char *query_blockstats_cmd = "{ \"execute\": \"query-blockstats\" }";
+	const char *cmd_mode = "{ \"execute\": \"qmp_capabilities\" }";
+	const char *query_blockstats_cmd = "{ \"execute\": \"query-blockstats\" }";
 	unsigned char *qmp_stats, *val;
 	char path[80];
 	int qfd;
-- 
2.17.1

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

[Julien Grall]

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>
---
 tools/xl/xl.h         | 8 ++++----
 tools/xl/xl_console.c | 2 +-
 tools/xl/xl_utils.c   | 4 ++--
 tools/xl/xl_utils.h   | 4 ++--
 4 files changed, 9 insertions(+), 9 deletions(-)

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 */
 
 /*
-- 
2.17.1

[PATCH 08/14] tools/firmware: hvmloader: Use const in __bug() and __assert_failed()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

__bug() and __assert_failed() are not meant to modify the string
parameters. So mark them as const.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 tools/firmware/hvmloader/util.c | 4 ++--
 tools/firmware/hvmloader/util.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index 7da144b0bb15..581b35e5cfb5 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -722,14 +722,14 @@ static void __attribute__((noreturn)) crash(void)
         asm volatile ( "hlt" );
 }
 
-void __assert_failed(char *assertion, char *file, int line)
+void __assert_failed(const char *assertion, const char *file, int line)
 {
     printf("*** HVMLoader assertion '%s' failed at %s:%d\n",
            assertion, file, line);
     crash();
 }
 
-void __bug(char *file, int line)
+void __bug(const char *file, int line)
 {
     printf("*** HVMLoader bug at %s:%d\n", file, line);
     crash();
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index 4f0baade0e6c..8d95eab28a65 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -34,11 +34,11 @@ enum {
 #undef NULL
 #define NULL ((void*)0)
 
-void __assert_failed(char *assertion, char *file, int line)
+void __assert_failed(const char *assertion, const char *file, int line)
     __attribute__((noreturn));
 #define ASSERT(p) \
     do { if (!(p)) __assert_failed(#p, __FILE__, __LINE__); } while (0)
-void __bug(char *file, int line) __attribute__((noreturn));
+void __bug(const char *file, int line) __attribute__((noreturn));
 #define BUG() __bug(__FILE__, __LINE__)
 #define BUG_ON(p) do { if (p) BUG(); } while (0)
 #define BUILD_BUG_ON(p) ((void)sizeof(char[1 - 2 * !!(p)]))
-- 
2.17.1

[PATCH 09/14] tools/console: Use const whenever we point to literal strings

[Julien Grall]

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>
---
 tools/console/client/main.c |  4 ++--
 tools/console/daemon/io.c   | 10 +++++-----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/console/client/main.c b/tools/console/client/main.c
index 088be28dff02..80157be42144 100644
--- a/tools/console/client/main.c
+++ b/tools/console/client/main.c
@@ -325,7 +325,7 @@ int main(int argc, char **argv)
 {
 	struct termios attr;
 	int domid;
-	char *sopt = "hn:";
+	const char *sopt = "hn:";
 	int ch;
 	unsigned int num = 0;
 	int opt_ind=0;
@@ -345,7 +345,7 @@ int main(int argc, char **argv)
 	char *end;
 	console_type type = CONSOLE_INVAL;
 	bool interactive = 0;
-	char *console_names = "serial, pv, vuart";
+	const char *console_names = "serial, pv, vuart";
 
 	while((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
 		switch(ch) {
diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 4af27ffc5d02..6a8a94e31b65 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -87,14 +87,14 @@ struct buffer {
 };
 
 struct console {
-	char *ttyname;
+	const char *ttyname;
 	int master_fd;
 	int master_pollfd_idx;
 	int slave_fd;
 	int log_fd;
 	struct buffer buffer;
 	char *xspath;
-	char *log_suffix;
+	const char *log_suffix;
 	int ring_ref;
 	xenevtchn_handle *xce_handle;
 	int xce_pollfd_idx;
@@ -109,9 +109,9 @@ struct console {
 };
 
 struct console_type {
-	char *xsname;
-	char *ttyname;
-	char *log_suffix;
+	const char *xsname;
+	const char *ttyname;
+	const char *log_suffix;
 	bool optional;
 	bool use_gnttab;
 };
-- 
2.17.1

[PATCH 10/14] tools/kdd: Use const whenever we point to literal strings

[Julien Grall]

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 shore a pointer to them.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 tools/debugger/kdd/kdd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/debugger/kdd/kdd.c b/tools/debugger/kdd/kdd.c
index a7d0976ea4a8..17513c26505e 100644
--- a/tools/debugger/kdd/kdd.c
+++ b/tools/debugger/kdd/kdd.c
@@ -67,7 +67,7 @@ typedef struct {
     uint32_t build;             
     int w64;
     int mp;
-    char *name;
+    const char *name;
     uint64_t base;              /* KernBase: start looking here */
     uint32_t range;             /* |         and search an area this size */
     uint32_t version;           /* +-> NtBuildNumber */
@@ -237,7 +237,7 @@ static size_t blocking_write(int fd, const void *buf, size_t count)
 }
 
 /* Dump the contents of a complete serial packet into a log file. */
-static void kdd_log_pkt(kdd_state *s, char *name, kdd_pkt *p)
+static void kdd_log_pkt(kdd_state *s, const char *name, kdd_pkt *p)
 {
     uint32_t sum = 0;
     unsigned int i, j;
@@ -504,8 +504,8 @@ static int check_os(kdd_state *s)
  * @return -1 on failure to find the section name
  * @return 0 on success
  */
-static int get_pe64_sections(kdd_state *s, uint64_t filebase, char *sectname,
-        uint64_t *vaddr, uint32_t *vsize)
+static int get_pe64_sections(kdd_state *s, uint64_t filebase,
+        const char *sectname, uint64_t *vaddr, uint32_t *vsize)
 {
     uint64_t pe_hdr = 0;
     uint64_t sect_start = 0;
@@ -781,7 +781,7 @@ static void kdd_send_cmd(kdd_state *s, uint32_t subtype, size_t extra)
 }
 
 /* Cause the client to print a string */
-static void kdd_send_string(kdd_state *s, char *fmt, ...)
+static void kdd_send_string(kdd_state *s, const char *fmt, ...)
 {
     uint32_t len = 0xffff - sizeof (kdd_msg);
     char *buf = (char *) s->txb + sizeof (kdd_hdr) + sizeof (kdd_msg);
-- 
2.17.1

[PATCH 11/14] tools/misc: Use const whenever we point to literal strings

[Julien Grall]

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 we to store a pointer to them.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 tools/misc/xen-detect.c | 2 +-
 tools/misc/xenhypfs.c   | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/misc/xen-detect.c b/tools/misc/xen-detect.c
index eac9e46a35bb..18b28dabf311 100644
--- a/tools/misc/xen-detect.c
+++ b/tools/misc/xen-detect.c
@@ -44,7 +44,7 @@ enum guest_type {
     XEN_NONE = 3
 };
 
-static char *type;
+static const char *type;
 static char *ver;
 
 static void cpuid(uint32_t idx, uint32_t *regs, int pv_context)
diff --git a/tools/misc/xenhypfs.c b/tools/misc/xenhypfs.c
index 5da24aed905c..df398b07bdc0 100644
--- a/tools/misc/xenhypfs.c
+++ b/tools/misc/xenhypfs.c
@@ -81,9 +81,9 @@ static int xenhypfs_wr(char *path, char *val)
     return ret;
 }
 
-static char *xenhypfs_type(struct xenhypfs_dirent *ent)
+static const char *xenhypfs_type(struct xenhypfs_dirent *ent)
 {
-    char *res;
+    const char *res;
 
     switch (ent->type) {
     case xenhypfs_type_dir:
@@ -134,7 +134,7 @@ static int xenhypfs_ls(char *path)
     return ret;
 }
 
-static int xenhypfs_tree_sub(char *path, unsigned int depth)
+static int xenhypfs_tree_sub(const char *path, unsigned int depth)
 {
     struct xenhypfs_dirent *ent;
     unsigned int n, i;
-- 
2.17.1

[PATCH 12/14] tools/top: The string parameter in set_prompt() and set_delay() should be const

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Neither string parameter in set_prompt() and set_delay() are meant to
be modified. In particular, new_prompt can point to a literal string.

So mark the two parameters as const and propagate it.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 tools/xentop/xentop.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/xentop/xentop.c b/tools/xentop/xentop.c
index ebed070c0fa2..950e8935c4c1 100644
--- a/tools/xentop/xentop.c
+++ b/tools/xentop/xentop.c
@@ -81,8 +81,8 @@ static int current_row(void);
 static int lines(void);
 static void print(const char *, ...) __attribute__((format(printf,1,2)));
 static void attr_addstr(int attr, const char *str);
-static void set_delay(char *value);
-static void set_prompt(char *new_prompt, void (*func)(char *));
+static void set_delay(const char *value);
+static void set_prompt(const char *new_prompt, void (*func)(const char *));
 static int handle_key(int);
 static int compare(unsigned long long, unsigned long long);
 static int compare_domains(xenstat_domain **, xenstat_domain **);
@@ -212,10 +212,10 @@ int show_vbds = 0;
 int repeat_header = 0;
 int show_full_name = 0;
 #define PROMPT_VAL_LEN 80
-char *prompt = NULL;
+const char *prompt = NULL;
 char prompt_val[PROMPT_VAL_LEN];
 int prompt_val_len = 0;
-void (*prompt_complete_func)(char *);
+void (*prompt_complete_func)(const char *);
 
 static WINDOW *cwin;
 
@@ -331,7 +331,7 @@ static void attr_addstr(int attr, const char *str)
 }
 
 /* Handle setting the delay from the user-supplied value in prompt_val */
-static void set_delay(char *value)
+static void set_delay(const char *value)
 {
 	int new_delay;
 	new_delay = atoi(value);
@@ -341,7 +341,7 @@ static void set_delay(char *value)
 
 /* Enable prompting mode with the given prompt string; call the given function
  * when a value is available. */
-static void set_prompt(char *new_prompt, void (*func)(char *))
+static void set_prompt(const char *new_prompt, void (*func)(const char *))
 {
 	prompt = new_prompt;
 	prompt_val[0] = '\0';
-- 
2.17.1

[PATCH 13/14] tools/xenmon: xenbaked: Mark const the field text in stat_map_t

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

The field text in stat_map_t will point to string literals. So mark it
as const to allow the compiler to catch any modified of the string.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 tools/xenmon/xenbaked.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/xenmon/xenbaked.c b/tools/xenmon/xenbaked.c
index d3f940a26bb2..1ed34334c824 100644
--- a/tools/xenmon/xenbaked.c
+++ b/tools/xenmon/xenbaked.c
@@ -182,7 +182,7 @@ typedef struct
 {
     int event_count;
     int event_id;
-    char *text;
+    const char *text;
 } stat_map_t;
 
 stat_map_t stat_map[] = {
-- 
2.17.1

[PATCH 14/14] tools/xentrace: Use const whenever we point to literal strings

[Julien Grall]

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>
---
 tools/xentrace/xenalyze.c | 71 ++++++++++++++++++++-------------------
 tools/xentrace/xenctx.c   |  4 +--
 2 files changed, 39 insertions(+), 36 deletions(-)

diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
index b7f4e2bea83d..5de167031e01 100644
--- a/tools/xentrace/xenalyze.c
+++ b/tools/xentrace/xenalyze.c
@@ -356,7 +356,7 @@ void parse_symbol_file(char *fn) {
 char * find_symbol(unsigned long long addr) {
     struct symbol_struct * p=G.symbols;
     int i;
-    char * lastname="ZERO";
+    const char * lastname="ZERO";
     unsigned long long offset=addr;
     static char name[144];
 
@@ -495,7 +495,7 @@ struct {
 
 #define HVM_VMX_EXIT_REASON_MAX (EXIT_REASON_XSETBV+1)
 
-char * hvm_vmx_exit_reason_name[HVM_VMX_EXIT_REASON_MAX] = {
+const char * hvm_vmx_exit_reason_name[HVM_VMX_EXIT_REASON_MAX] = {
     [EXIT_REASON_EXCEPTION_NMI]="EXCEPTION_NMI",
     [EXIT_REASON_EXTERNAL_INTERRUPT]="EXTERNAL_INTERRUPT",
     [EXIT_REASON_TRIPLE_FAULT]="TRIPLE_FAULT",
@@ -698,7 +698,7 @@ enum VMEXIT_EXITCODE
 };
 
 #define HVM_SVM_EXIT_REASON_MAX 1025
-char * hvm_svm_exit_reason_name[HVM_SVM_EXIT_REASON_MAX] = {
+const char * hvm_svm_exit_reason_name[HVM_SVM_EXIT_REASON_MAX] = {
     /* 0-15 */
     "VMEXIT_CR0_READ",
     "VMEXIT_CR1_READ",
@@ -875,7 +875,7 @@ char * hvm_svm_exit_reason_name[HVM_SVM_EXIT_REASON_MAX] = {
 #define EXTERNAL_INTERRUPT_MAX 256
 
 /* Stringify numbers */
-char * hvm_extint_vector_name[EXTERNAL_INTERRUPT_MAX] = {
+const char * hvm_extint_vector_name[EXTERNAL_INTERRUPT_MAX] = {
     [SPURIOUS_APIC_VECTOR] = "SPURIOS_APIC",
     [ERROR_APIC_VECTOR] =    "ERROR_APIC",
     [INVALIDATE_TLB_VECTOR]= "INVALIDATE_TLB",
@@ -887,7 +887,7 @@ char * hvm_extint_vector_name[EXTERNAL_INTERRUPT_MAX] = {
 
 #define HVM_TRAP_MAX 20
 
-char * hvm_trap_name[HVM_TRAP_MAX] = {
+const char * hvm_trap_name[HVM_TRAP_MAX] = {
     [0] =  "Divide",
     [1] =  "RESERVED",
     [2] =  "NMI",
@@ -947,7 +947,7 @@ enum {
     HVM_EVENT_VLAPIC,
     HVM_EVENT_HANDLER_MAX
 };
-char * hvm_event_handler_name[HVM_EVENT_HANDLER_MAX] = {
+const char * hvm_event_handler_name[HVM_EVENT_HANDLER_MAX] = {
     "(no handler)",
     "pf_xen",
     "pf_inject",
@@ -1001,7 +1001,7 @@ enum {
     GUEST_INTERRUPT_CASE_MAX,
 };
 
-char *guest_interrupt_case_name[] = {
+const char *guest_interrupt_case_name[] = {
     [GUEST_INTERRUPT_CASE_WAKE_TO_HALT_ALONE]="wake to halt alone",
     /* This interrupt woke, maybe another interrupt before halt */
     [GUEST_INTERRUPT_CASE_WAKE_TO_HALT_ANY]  ="wake to halt any  ",
@@ -1009,7 +1009,7 @@ char *guest_interrupt_case_name[] = {
     [GUEST_INTERRUPT_CASE_INTERRUPT_TO_HALT] ="intr to halt      ",
 };
 
-char *hvm_vol_name[HVM_VOL_MAX] = {
+const char *hvm_vol_name[HVM_VOL_MAX] = {
     [HVM_VOL_VMENTRY]="vmentry",
     [HVM_VOL_VMEXIT] ="vmexit",
     [HVM_VOL_HANDLER]="handler",
@@ -1056,7 +1056,7 @@ enum {
     HYPERCALL_MAX
 };
 
-char *hypercall_name[HYPERCALL_MAX] = {
+const char *hypercall_name[HYPERCALL_MAX] = {
     [HYPERCALL_set_trap_table]="set_trap_table",
     [HYPERCALL_mmu_update]="mmu_update",
     [HYPERCALL_set_gdt]="set_gdt",
@@ -1114,7 +1114,7 @@ enum {
     PF_XEN_EMUL_MAX,
 };
 
-char * pf_xen_emul_name[PF_XEN_EMUL_MAX] = {
+const char * pf_xen_emul_name[PF_XEN_EMUL_MAX] = {
     [PF_XEN_EMUL_LVL_0]="non-linmap",
     [PF_XEN_EMUL_LVL_1]="linmap l1",
     [PF_XEN_EMUL_LVL_2]="linmap l2",
@@ -1140,7 +1140,7 @@ enum {
     PF_XEN_NON_EMUL_MAX,
 };
 
-char * pf_xen_non_emul_name[PF_XEN_NON_EMUL_MAX] = {
+const char * pf_xen_non_emul_name[PF_XEN_NON_EMUL_MAX] = {
     [PF_XEN_NON_EMUL_VA_USER]="va user",
     [PF_XEN_NON_EMUL_VA_KERNEL]="va kernel",
     [PF_XEN_NON_EMUL_EIP_USER]="eip user",
@@ -1160,7 +1160,7 @@ enum {
     PF_XEN_FIXUP_MAX,
 };
 
-char * pf_xen_fixup_name[PF_XEN_FIXUP_MAX] = {
+const char * pf_xen_fixup_name[PF_XEN_FIXUP_MAX] = {
     [PF_XEN_FIXUP_PREALLOC_UNPIN] = "unpin",
     [PF_XEN_FIXUP_PREALLOC_UNHOOK] = "unhook",
     [PF_XEN_FIXUP_UNSYNC] = "unsync",
@@ -1195,7 +1195,7 @@ enum {
 #define SHADOW_RESYNC_FULL    14
 #define SHADOW_RESYNC_ONLY    15
 
-char * pf_xen_name[PF_XEN_MAX] = {
+const char * pf_xen_name[PF_XEN_MAX] = {
     [PF_XEN_NOT_SHADOW]="propagate",
     [PF_XEN_FAST_PROPAGATE]="fast propagate",
     [PF_XEN_FAST_MMIO]="fast mmio",
@@ -1304,7 +1304,7 @@ struct hvm_data {
     struct vcpu_data *v; /* up-pointer */
 
     /* SVM / VMX compatibility. FIXME - should be global */
-    char ** exit_reason_name;
+    const char ** exit_reason_name;
     int exit_reason_max;
     struct hvm_summary_handler_node *exit_reason_summary_handler_list[HVM_EXIT_REASON_MAX];
 
@@ -1408,7 +1408,7 @@ enum {
     HVM_SHORT_SUMMARY_MAX,
 };
 
-char *hvm_short_summary_name[HVM_SHORT_SUMMARY_MAX] = {
+const char *hvm_short_summary_name[HVM_SHORT_SUMMARY_MAX] = {
     [HVM_SHORT_SUMMARY_EMULATE]  ="emulate",
     [HVM_SHORT_SUMMARY_UNSYNC]   ="unsync",
     [HVM_SHORT_SUMMARY_FIXUP]    ="fixup",
@@ -1478,7 +1478,7 @@ enum {
     PV_MAX
 };
 
-char *pv_name[PV_MAX] = {
+const char *pv_name[PV_MAX] = {
     [PV_HYPERCALL]="hypercall",
     [PV_TRAP]="trap",
     [PV_PAGE_FAULT]="page_fault",
@@ -1527,7 +1527,7 @@ int runstate_graph[RUNSTATE_MAX] =
     [RUNSTATE_INIT]=-2,
 };
 
-char * runstate_name[RUNSTATE_MAX]={
+const char * runstate_name[RUNSTATE_MAX]={
     [RUNSTATE_RUNNING]= "running",
     [RUNSTATE_RUNNABLE]="runnable",
     [RUNSTATE_BLOCKED]= "blocked", /* to be blocked */
@@ -1545,7 +1545,7 @@ enum {
     RUNNABLE_STATE_MAX
 };
 
-char * runnable_state_name[RUNNABLE_STATE_MAX]={
+const char * runnable_state_name[RUNNABLE_STATE_MAX]={
     [RUNNABLE_STATE_INVALID]="invalid", /* Should never show up */
     [RUNNABLE_STATE_WAKE]="wake",
     [RUNNABLE_STATE_PREEMPT]="preempt",
@@ -1565,7 +1565,7 @@ enum {
     MEM_MAX
 };
 
-char *mem_name[MEM_MAX] = {
+const char *mem_name[MEM_MAX] = {
     [MEM_PAGE_GRANT_MAP]         = "grant-map",
     [MEM_PAGE_GRANT_UNMAP]       = "grant-unmap",
     [MEM_PAGE_GRANT_TRANSFER]    = "grant-transfer",
@@ -1681,7 +1681,7 @@ enum {
     DOMAIN_RUNSTATE_MAX
 };
 
-char * domain_runstate_name[] = {
+const char * domain_runstate_name[] = {
     [DOMAIN_RUNSTATE_BLOCKED]="blocked",
     [DOMAIN_RUNSTATE_PARTIAL_RUN]="partial run",
     [DOMAIN_RUNSTATE_FULL_RUN]="full run",
@@ -1698,7 +1698,7 @@ enum {
     POD_RECLAIM_CONTEXT_MAX
 };
 
-char * pod_reclaim_context_name[] = {
+const char * pod_reclaim_context_name[] = {
     [POD_RECLAIM_CONTEXT_UNKNOWN]="unknown",
     [POD_RECLAIM_CONTEXT_FAULT]="fault",
     [POD_RECLAIM_CONTEXT_BALLOON]="balloon",
@@ -1756,7 +1756,7 @@ enum {
     TOPLEVEL_MAX=TOPLEVEL_HW+1,
 };
 
-char * toplevel_name[TOPLEVEL_MAX] = {
+const char * toplevel_name[TOPLEVEL_MAX] = {
     [TOPLEVEL_GEN]="gen",
     [TOPLEVEL_SCHED]="sched",
     [TOPLEVEL_DOM0OP]="dom0op",
@@ -2263,7 +2263,7 @@ static inline void clear_interval_cycles(struct interval_element *e) {
     e->instructions = 0;
 }
 
-static inline void print_cpu_affinity(struct cycle_summary *s, char *p) {
+static inline void print_cpu_affinity(struct cycle_summary *s, const char *p) {
     if(s->count) {
         long long avg;
 
@@ -2326,7 +2326,8 @@ static inline void print_cycle_percent_summary(struct cycle_summary *s,
     }
 }
 
-static inline void print_cycle_summary(struct cycle_summary *s, char *p) {
+static inline void print_cycle_summary(struct cycle_summary *s,
+                                       const char *p) {
     if(s->count) {
         long long avg;
 
@@ -2938,7 +2939,7 @@ void hvm_update_short_summary(struct hvm_data *h, int element) {
 }
 
 void hvm_short_summary(struct hvm_short_summary_struct *hss,
-                       tsc_t total, char *prefix) {
+                       tsc_t total, const char *prefix) {
     char desc[80];
     int i;
 
@@ -3352,7 +3353,7 @@ void hvm_pf_xen_process(struct record_info *ri, struct hvm_data *h) {
          fprintf(warn, "%s: Strange, postprocess already set\n", __func__);
 }
 
-char * hvm_vlapic_icr_dest_shorthand_name[4] = {
+const char * hvm_vlapic_icr_dest_shorthand_name[4] = {
     "dest_field", "self", "all-inc", "all-exc"
 };
 
@@ -3800,7 +3801,7 @@ void update_io_address(struct io_address ** list, unsigned int pa, int dir,
     update_cycles(&p->summary[dir], arc_cycles);
 }
 
-void hvm_io_address_summary(struct io_address *list, char * s) {
+void hvm_io_address_summary(struct io_address *list, const char * s) {
     if(!list)
         return;
 
@@ -4484,7 +4485,7 @@ void hvm_intr_window_process(struct record_info *ri, struct hvm_data *h)
         int32_t intr;
     } *r = (typeof(r))h->d;
 
-    char *intsrc_name[] = {
+    const char *intsrc_name[] = {
         "none",
         "pic",
         "lapic",
@@ -4687,14 +4688,15 @@ void hvm_generic_postprocess(struct hvm_data *h)
     }
 }
 
-void hvm_generic_dump(struct record_info *ri, char * prefix)
+void hvm_generic_dump(struct record_info *ri, const char * prefix)
 {
     struct {
         unsigned vcpu:16, domain:16;
         unsigned d[4];
     } *cr = (typeof(cr))ri->d;
 
-    char *evt_string, evt_number[256];
+    const char *evt_string;
+    char evt_number[256];
     int i, evt, is_64 = 0;
 
     evt = ri->event - TRC_HVM_HANDLER;
@@ -6042,10 +6044,11 @@ void shadow_propagate_process(struct record_info *ri, struct hvm_data *h)
         fprintf(warn, "%s: Strange, postprocess already set\n", __func__);
 }
 
-void shadow_fault_generic_dump(unsigned int event, uint32_t *d, char *prefix,
-                         char * dump_header)
+void shadow_fault_generic_dump(unsigned int event, uint32_t *d,
+                               const char *prefix, const char * dump_header)
 {
-    char *evt_string, evt_number[10];
+    const char *evt_string;
+    char evt_number[10];
     union shadow_event sevt = { .event = event };
     int i;
 
@@ -8643,7 +8646,7 @@ void dump_generic(FILE * f, struct record_info *ri)
     fprintf(f, " ]\n");
 }
 
-void dump_raw(char * s, struct record_info *ri)
+void dump_raw(const char * s, struct record_info *ri)
 {
     int i;
 
diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
index 2fa864f86723..972f473dbf02 100644
--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -302,7 +302,7 @@ static void read_symbol_table(const char *symtab)
 
 #if defined(__i386__) || defined(__x86_64__)
 #define CR0_PE  0x1
-char *flag_values[22][2] =
+const char *flag_values[22][2] =
 {/*  clear,     set,       bit# */
     { NULL,     "c"    }, // 0        Carry
     { NULL,     NULL   }, // 1
@@ -334,7 +334,7 @@ static void print_flags(uint64_t flags)
 
     printf("\nflags: %08" PRIx64, flags);
     for (i = 21; i >= 0; i--) {
-        char *s = flag_values[i][(flags >> i) & 1];
+        const char *s = flag_values[i][(flags >> i) & 1];
         if (s != NULL)
             printf(" %s", s);
     }
-- 
2.17.1

Re: [PATCH 00/14] Use const whether we point to literal strings (take 1)

[Elliott Mitchell]

On Mon, Apr 05, 2021 at 04:56:59PM +0100, Julien Grall wrote:
> I am not aware of code trying to modify literal strings in Xen.
> However, there is a frequent use of non-const char * to point to
> literal strings. Given the size of the codebase, there is a risk
> to involuntarily introduce code that will modify literal strings.
> 
> Therefore it would be better to enforce using const when pointing
> to such strings. Both GCC and Clang provide an option to warn
> for such case (see -Wwrite-strings) and therefore could be used
> by Xen.
> 
> This series doesn't yet make use of -Wwrite-strings because
> the tree is not fully converted. Instead, it contains some easy
> and likely non-controversial use const in the code.
> 
> The major blockers to enable -Wwrite-strings are the following:
>     - xen/common/efi: union string is used in both const and
>     non-const situation. It doesn't feel right to specific one member
>     const and the other non-const.
>     - libxl: the major block is the flexarray framework as we would use
>     it with string (now const char*). I thought it would be possible to
>     make the interface const, but it looks like there are a couple of
>     places where we need to modify the content (such as in
>     libxl_json.c).
> 
> Ideally, I would like to have -Wwrite-strings unconditionally used
> tree-wide. But, some of the area may required some heavy refactoring.
> 
> One solution would be to enable it tree-wide but turned it off at a
> directroy/file level.
> 
> Any opinions?

I think doing such is a Good Idea(tm).  Adding consts adds opportunities
for greater optimization.  Both by compilers which can avoid extra
copies, and developers who then know they don't need to generate extra
copies to sacrific them to an API.  In particular the consts also
function as documentation.

So you're certainly not the only person who thinks additional consts
would be a good thing:

https://lists.xenproject.org/archives/html/xen-devel/2021-01/msg00132.html


Alas merely getting the two const patches into the latest release
apparently wasn't even worthy of response:

https://lists.xenproject.org/archives/html/xen-devel/2021-02/msg01040.html


I agree this should be done, though I'm aware many developers hate
bothering with constants.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

Re: [PATCH 03/14] xen/x86: shadow: The return type of sh_audit_flags() should be const

[Roger Pau Monné]

On Mon, Apr 05, 2021 at 04:57:02PM +0100, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The function sh_audit_flags() is returning pointer to literal strings.
> They should not be modified, so the return is now const and this is
> propagated to the callers.
> 
> Take the opportunity to fix the coding style in the declaration of
> sh_audit_flags.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

While doing the cleanup I think you could narrow the scope of the 's'
variables also, but doesn't need to be part of this patch:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 08/14] tools/firmware: hvmloader: Use const in __bug() and __assert_failed()

[Roger Pau Monné]

On Mon, Apr 05, 2021 at 04:57:07PM +0100, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> __bug() and __assert_failed() are not meant to modify the string
> parameters. So mark them as const.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

While looking at this I think we should also make the line parameter
unsigned, but again doesn't need to be part of this patch.

Thanks, Roger.

Re: [PATCH 00/14] Use const whether we point to literal strings (take 1)

[Jan Beulich]

On 05.04.2021 17:56, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Hi all,
> 
> By default, both Clang and GCC will happily compile C code where
> non-const char * point to literal strings. This means the following
> code will be accepted:
> 
>     char *str = "test";
> 
>     str[0] = 'a';
> 
> Literal strings will reside in rodata, so they are not modifiable.
> This will result to an permission fault at runtime if the permissions
> are enforced in the page-tables (this is the case in Xen).
> 
> I am not aware of code trying to modify literal strings in Xen.
> However, there is a frequent use of non-const char * to point to
> literal strings. Given the size of the codebase, there is a risk
> to involuntarily introduce code that will modify literal strings.
> 
> Therefore it would be better to enforce using const when pointing
> to such strings. Both GCC and Clang provide an option to warn
> for such case (see -Wwrite-strings) and therefore could be used
> by Xen.
> 
> This series doesn't yet make use of -Wwrite-strings because
> the tree is not fully converted. Instead, it contains some easy
> and likely non-controversial use const in the code.
> 
> The major blockers to enable -Wwrite-strings are the following:
>     - xen/common/efi: union string is used in both const and
>     non-const situation. It doesn't feel right to specific one member
>     const and the other non-const.

I'd be happy to see a suggestion of how to avoid this in a not overly
intrusive way.

>     - libxl: the major block is the flexarray framework as we would use
>     it with string (now const char*). I thought it would be possible to
>     make the interface const, but it looks like there are a couple of
>     places where we need to modify the content (such as in
>     libxl_json.c).
> 
> Ideally, I would like to have -Wwrite-strings unconditionally used
> tree-wide. But, some of the area may required some heavy refactoring.
> 
> One solution would be to enable it tree-wide but turned it off at a
> directroy/file level.

At least as a transient approach I think this would make sense. EFI in
particular has other reasons already to specify a custom option
(-fshort-wchar).

Jan

Re: [PATCH 01/14] xen: Constify the second parameter of rangeset_new()

[Jan Beulich]

On 05.04.2021 17:57, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The string 'name' will never get modified by the function, so mark it
> as const.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> --- a/xen/common/rangeset.c
> +++ b/xen/common/rangeset.c
> @@ -421,7 +421,7 @@ bool_t rangeset_is_empty(
>  }
>  
>  struct rangeset *rangeset_new(
> -    struct domain *d, char *name, unsigned int flags)
> +    struct domain *d, const char *name, unsigned int flags)
>  {
>      struct rangeset *r;

Remotely along these lines the function also has no need anymore to
use snprintf() - safe_strcpy() very well fits both purposes. But
quite likely for another patch.

Jan

Re: [PATCH 02/14] xen/sched: Constify name and opt_name in struct scheduler

[Jan Beulich]

On 05.04.2021 17:57, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Both name and opt_name are pointing to literal string. So mark both of
> the fields as const.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit ...

> --- a/xen/common/sched/private.h
> +++ b/xen/common/sched/private.h
> @@ -272,8 +272,8 @@ static inline spinlock_t *pcpu_schedule_trylock(unsigned int cpu)
>  }
>  
>  struct scheduler {
> -    char *name;             /* full name for this scheduler      */
> -    char *opt_name;         /* option name for this scheduler    */
> +    const char *name;       /* full name for this scheduler      */
> +    const char *opt_name;   /* option name for this scheduler    */

... I'd like to suggest considering at least the latter to become
an array instead of a pointer - there's little point wasting 8
bytes of storage for the pointer when the strings pointed to are
all at most 9 bytes long (right now; I don't expect much longer
ones to appear).

Jan

Re: [PATCH 04/14] xen/char: console: Use const whenever we point to literal strings

[Jan Beulich]

On 05.04.2021 17:57, Julien Grall wrote:
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -168,7 +168,7 @@ static int parse_guest_loglvl(const char *s);
>  static char xenlog_val[LOGLVL_VAL_SZ];
>  static char xenlog_guest_val[LOGLVL_VAL_SZ];
>  
> -static char *lvl2opt[] = { "none", "error", "warning", "info", "all" };
> +static const char *lvl2opt[] = { "none", "error", "warning", "info", "all" };

If you add any const here, then I think you should go to full way
and also add the second missing one. Then
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Arguably the array should also be local to xenlog_update_val().

Jan

Re: [PATCH 03/14] xen/x86: shadow: The return type of sh_audit_flags() should be const

[Tim Deegan]

At 16:57 +0100 on 05 Apr (1617641822), Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The function sh_audit_flags() is returning pointer to literal strings.
> They should not be modified, so the return is now const and this is
> propagated to the callers.
> 
> Take the opportunity to fix the coding style in the declaration of
> sh_audit_flags.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Acked-by: Tim Deegan <tim@xen.org>

Thanks,

Tim.

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

[Tim Deegan]

At 16:57 +0100 on 05 Apr (1617641829), 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 shore a pointer to them.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Acked-by: Tim Deegan <tim@xen.org>

Thanks,

Tim.

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

[George Dunlap]

> On Apr 5, 2021, at 4:57 PM, Julien Grall <julien@xen.org> 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>

Acked-by: George Dunlap <george.dunlap@citrix.com>

Re: [PATCH 02/14] xen/sched: Constify name and opt_name in struct scheduler

[George Dunlap]

> On Apr 5, 2021, at 4:57 PM, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Both name and opt_name are pointing to literal string. So mark both of
> the fields as const.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Acked-by: George Dunlap <george.dunlap@citrix.com>

Re: [PATCH 00/14] Use const whether we point to literal strings (take 1)

[Julien Grall]

Hi Elliott,

Thanks for the input!

On 05/04/2021 18:01, Elliott Mitchell wrote:
> On Mon, Apr 05, 2021 at 04:56:59PM +0100, Julien Grall wrote:
>> I am not aware of code trying to modify literal strings in Xen.
>> However, there is a frequent use of non-const char * to point to
>> literal strings. Given the size of the codebase, there is a risk
>> to involuntarily introduce code that will modify literal strings.
>>
>> Therefore it would be better to enforce using const when pointing
>> to such strings. Both GCC and Clang provide an option to warn
>> for such case (see -Wwrite-strings) and therefore could be used
>> by Xen.
>>
>> This series doesn't yet make use of -Wwrite-strings because
>> the tree is not fully converted. Instead, it contains some easy
>> and likely non-controversial use const in the code.
>>
>> The major blockers to enable -Wwrite-strings are the following:
>>      - xen/common/efi: union string is used in both const and
>>      non-const situation. It doesn't feel right to specific one member
>>      const and the other non-const.
>>      - libxl: the major block is the flexarray framework as we would use
>>      it with string (now const char*). I thought it would be possible to
>>      make the interface const, but it looks like there are a couple of
>>      places where we need to modify the content (such as in
>>      libxl_json.c).
>>
>> Ideally, I would like to have -Wwrite-strings unconditionally used
>> tree-wide. But, some of the area may required some heavy refactoring.
>>
>> One solution would be to enable it tree-wide but turned it off at a
>> directroy/file level.
>>
>> Any opinions?
> 
> I think doing such is a Good Idea(tm).  Adding consts adds opportunities
> for greater optimization.  Both by compilers which can avoid extra
> copies, and developers who then know they don't need to generate extra
> copies to sacrific them to an API.  In particular the consts also
> function as documentation.
> 
> So you're certainly not the only person who thinks additional consts
> would be a good thing:
> 
> https://lists.xenproject.org/archives/html/xen-devel/2021-01/msg00132.html
> 
> 
> Alas merely getting the two const patches into the latest release
> apparently wasn't even worthy of response:
> 
> https://lists.xenproject.org/archives/html/xen-devel/2021-02/msg01040.html

Sory to hear you had no response. Would you be able to give a nudge to 
the maintainers?

> 
> 
> I agree this should be done, though I'm aware many developers hate
> bothering with constants.
> 
> 

Cheers,

-- 
Julien Grall

Re: [PATCH 01/14] xen: Constify the second parameter of rangeset_new()

[Julien Grall]

Hi Jan,

On 06/04/2021 08:57, Jan Beulich wrote:
> On 05.04.2021 17:57, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> The string 'name' will never get modified by the function, so mark it
>> as const.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks!

> 
>> --- a/xen/common/rangeset.c
>> +++ b/xen/common/rangeset.c
>> @@ -421,7 +421,7 @@ bool_t rangeset_is_empty(
>>   }
>>   
>>   struct rangeset *rangeset_new(
>> -    struct domain *d, char *name, unsigned int flags)
>> +    struct domain *d, const char *name, unsigned int flags)
>>   {
>>       struct rangeset *r;
> 
> Remotely along these lines the function also has no need anymore to
> use snprintf() - safe_strcpy() very well fits both purposes. But
> quite likely for another patch.

I saw you already sent the patch for that. So I am assuming there is no 
action for me here.

Cheers,

-- 
Julien Grall

Re: [PATCH 02/14] xen/sched: Constify name and opt_name in struct scheduler

[Julien Grall]

Hi Jan,

On 06/04/2021 09:07, Jan Beulich wrote:
> On 05.04.2021 17:57, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Both name and opt_name are pointing to literal string. So mark both of
>> the fields as const.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> albeit ...
> 
>> --- a/xen/common/sched/private.h
>> +++ b/xen/common/sched/private.h
>> @@ -272,8 +272,8 @@ static inline spinlock_t *pcpu_schedule_trylock(unsigned int cpu)
>>   }
>>   
>>   struct scheduler {
>> -    char *name;             /* full name for this scheduler      */
>> -    char *opt_name;         /* option name for this scheduler    */
>> +    const char *name;       /* full name for this scheduler      */
>> +    const char *opt_name;   /* option name for this scheduler    */
> 
> ... I'd like to suggest considering at least the latter to become
> an array instead of a pointer - there's little point wasting 8
> bytes of storage for the pointer when the strings pointed to are
> all at most 9 bytes long (right now; I don't expect much longer
> ones to appear).

I have tried this simple/dumb change on top of my patch:

diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h
index a870320146ef..ab2236874217 100644
--- a/xen/common/sched/private.h
+++ b/xen/common/sched/private.h
@@ -273,7 +273,7 @@ static inline spinlock_t 
*pcpu_schedule_trylock(unsigned int cpu)

  struct scheduler {
      const char *name;       /* full name for this scheduler      */
-    const char *opt_name;   /* option name for this scheduler    */
+    const char opt_name[9]; /* option name for this scheduler    */
      unsigned int sched_id;  /* ID for this scheduler             */
      void *sched_data;       /* global data pointer               */
      struct cpupool *cpupool;/* points to this scheduler's pool   */

GCC will throw an error:

core.c: In function ‘scheduler_init’:
core.c:2987:17: error: assignment of read-only variable ‘ops’
              ops = *schedulers[i];
                  ^
core.c:2997:21: error: assignment of read-only variable ‘ops’
                  ops = *schedulers[i];
                      ^

I don't particularly want to drop the const. So the code would probably 
need some rework.

My patch doesn't change the size of the structure, so I would prefer to 
keep this patch as-is.

Cheers,

-- 
Julien Grall

Re: [PATCH 03/14] xen/x86: shadow: The return type of sh_audit_flags() should be const

[Julien Grall]

Hi Roger,

On 06/04/2021 08:24, Roger Pau Monné wrote:
> On Mon, Apr 05, 2021 at 04:57:02PM +0100, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> The function sh_audit_flags() is returning pointer to literal strings.
>> They should not be modified, so the return is now const and this is
>> propagated to the callers.
>>
>> Take the opportunity to fix the coding style in the declaration of
>> sh_audit_flags.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> While doing the cleanup I think you could narrow the scope of the 's'
> variables also, but doesn't need to be part of this patch:

I think you are right. I will look at it as a follow-up.

> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks for the review!

Cheers,

-- 
Julien Grall

Re: [PATCH 04/14] xen/char: console: Use const whenever we point to literal strings

[Julien Grall]

Hi Jan,

On 06/04/2021 09:10, Jan Beulich wrote:
> On 05.04.2021 17:57, Julien Grall wrote:
>> --- a/xen/drivers/char/console.c
>> +++ b/xen/drivers/char/console.c
>> @@ -168,7 +168,7 @@ static int parse_guest_loglvl(const char *s);
>>   static char xenlog_val[LOGLVL_VAL_SZ];
>>   static char xenlog_guest_val[LOGLVL_VAL_SZ];
>>   
>> -static char *lvl2opt[] = { "none", "error", "warning", "info", "all" };
>> +static const char *lvl2opt[] = { "none", "error", "warning", "info", "all" };
> 
> If you add any const here, then I think you should go to full way
> and also add the second missing one. Then
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Arguably the array should also be local to xenlog_update_val().

I will look at it and send a new version.

Cheers,

-- 
Julien Grall

Re: [PATCH 08/14] tools/firmware: hvmloader: Use const in __bug() and __assert_failed()

[Julien Grall]

Hi Roger,

On 06/04/2021 08:29, Roger Pau Monné wrote:
> On Mon, Apr 05, 2021 at 04:57:07PM +0100, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> __bug() and __assert_failed() are not meant to modify the string
>> parameters. So mark them as const.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks!

> 
> While looking at this I think we should also make the line parameter
> unsigned, but again doesn't need to be part of this patch.
I would prefer if this is done in a follow-up patch.

Cheers,

-- 
Julien Grall

Re: [PATCH 00/14] Use const whether we point to literal strings (take 1)

[Julien Grall]

Hi,

On 05/04/2021 16:56, Julien Grall wrote:
> Julien Grall (14):
>    xen: Constify the second parameter of rangeset_new()
>    xen/sched: Constify name and opt_name in struct scheduler
>    xen/x86: shadow: The return type of sh_audit_flags() should be const >    tools/firmware: hvmloader: Use const in __bug() and __assert_failed()
>    tools/kdd: Use const whenever we point to literal strings
>    tools/xentrace: Use const whenever we point to literal strings

I have merged the above patches. The rest still needs some reviews or 
respin (for patch #4).

Cheers,

-- 
Julien Grall

Re: [PATCH 02/14] xen/sched: Constify name and opt_name in struct scheduler

[Jan Beulich]

On 06.04.2021 20:24, Julien Grall wrote:
> Hi Jan,
> 
> On 06/04/2021 09:07, Jan Beulich wrote:
>> On 05.04.2021 17:57, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> Both name and opt_name are pointing to literal string. So mark both of
>>> the fields as const.
>>>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> albeit ...
>>
>>> --- a/xen/common/sched/private.h
>>> +++ b/xen/common/sched/private.h
>>> @@ -272,8 +272,8 @@ static inline spinlock_t *pcpu_schedule_trylock(unsigned int cpu)
>>>   }
>>>   
>>>   struct scheduler {
>>> -    char *name;             /* full name for this scheduler      */
>>> -    char *opt_name;         /* option name for this scheduler    */
>>> +    const char *name;       /* full name for this scheduler      */
>>> +    const char *opt_name;   /* option name for this scheduler    */
>>
>> ... I'd like to suggest considering at least the latter to become
>> an array instead of a pointer - there's little point wasting 8
>> bytes of storage for the pointer when the strings pointed to are
>> all at most 9 bytes long (right now; I don't expect much longer
>> ones to appear).
> 
> I have tried this simple/dumb change on top of my patch:
> 
> diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h
> index a870320146ef..ab2236874217 100644
> --- a/xen/common/sched/private.h
> +++ b/xen/common/sched/private.h
> @@ -273,7 +273,7 @@ static inline spinlock_t 
> *pcpu_schedule_trylock(unsigned int cpu)
> 
>   struct scheduler {
>       const char *name;       /* full name for this scheduler      */
> -    const char *opt_name;   /* option name for this scheduler    */
> +    const char opt_name[9]; /* option name for this scheduler    */
>       unsigned int sched_id;  /* ID for this scheduler             */
>       void *sched_data;       /* global data pointer               */
>       struct cpupool *cpupool;/* points to this scheduler's pool   */
> 
> GCC will throw an error:
> 
> core.c: In function ‘scheduler_init’:
> core.c:2987:17: error: assignment of read-only variable ‘ops’
>               ops = *schedulers[i];
>                   ^
> core.c:2997:21: error: assignment of read-only variable ‘ops’
>                   ops = *schedulers[i];
>                       ^
> 
> I don't particularly want to drop the const. So the code would probably 
> need some rework.

What's wrong with not having the const when the field is an array?
The more that all original (build-time, i.e. contain in the binary)
instances of the struct are already const as a whole?

Jan

Re: [PATCH 02/14] xen/sched: Constify name and opt_name in struct scheduler

[Julien Grall]

Hi Jan,

On 07/04/2021 09:22, Jan Beulich wrote:
> On 06.04.2021 20:24, Julien Grall wrote:
>> Hi Jan,
>>
>> On 06/04/2021 09:07, Jan Beulich wrote:
>>> On 05.04.2021 17:57, Julien Grall wrote:
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> Both name and opt_name are pointing to literal string. So mark both of
>>>> the fields as const.
>>>>
>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> albeit ...
>>>
>>>> --- a/xen/common/sched/private.h
>>>> +++ b/xen/common/sched/private.h
>>>> @@ -272,8 +272,8 @@ static inline spinlock_t *pcpu_schedule_trylock(unsigned int cpu)
>>>>    }
>>>>    
>>>>    struct scheduler {
>>>> -    char *name;             /* full name for this scheduler      */
>>>> -    char *opt_name;         /* option name for this scheduler    */
>>>> +    const char *name;       /* full name for this scheduler      */
>>>> +    const char *opt_name;   /* option name for this scheduler    */
>>>
>>> ... I'd like to suggest considering at least the latter to become
>>> an array instead of a pointer - there's little point wasting 8
>>> bytes of storage for the pointer when the strings pointed to are
>>> all at most 9 bytes long (right now; I don't expect much longer
>>> ones to appear).
>>
>> I have tried this simple/dumb change on top of my patch:
>>
>> diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h
>> index a870320146ef..ab2236874217 100644
>> --- a/xen/common/sched/private.h
>> +++ b/xen/common/sched/private.h
>> @@ -273,7 +273,7 @@ static inline spinlock_t
>> *pcpu_schedule_trylock(unsigned int cpu)
>>
>>    struct scheduler {
>>        const char *name;       /* full name for this scheduler      */
>> -    const char *opt_name;   /* option name for this scheduler    */
>> +    const char opt_name[9]; /* option name for this scheduler    */
>>        unsigned int sched_id;  /* ID for this scheduler             */
>>        void *sched_data;       /* global data pointer               */
>>        struct cpupool *cpupool;/* points to this scheduler's pool   */
>>
>> GCC will throw an error:
>>
>> core.c: In function ‘scheduler_init’:
>> core.c:2987:17: error: assignment of read-only variable ‘ops’
>>                ops = *schedulers[i];
>>                    ^
>> core.c:2997:21: error: assignment of read-only variable ‘ops’
>>                    ops = *schedulers[i];
>>                        ^
>>
>> I don't particularly want to drop the const. So the code would probably
>> need some rework.
> 
> What's wrong with not having the const when the field is an array?
> The more that all original (build-time, i.e. contain in the binary)
> instances of the struct are already const as a whole?

The scheduler will do a shallow copy of the structure that will be 
non-const. So opt_name can be modified afterwards (one can argue this is 
unlikely).

If I have to chose between saving overall 20 bytes in the binary and 
const. I would chose the latter.

Cheers,

-- 
Julien Grall

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

[Anthony PERARD]

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,

-- 
Anthony PERARD

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

[Julien Grall]

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,

-- 
Julien Grall

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

[Anthony PERARD]

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,

-- 
Anthony PERARD

PING Re: [PATCH 00/14] Use const whether we point to literal strings (take 1)

[Julien Grall]

Hi,

Ian, Wei, Anthony, can I get some feedbacks on the tools side?

Cheers,

On 05/04/2021 16:56, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Hi all,
> 
> By default, both Clang and GCC will happily compile C code where
> non-const char * point to literal strings. This means the following
> code will be accepted:
> 
>      char *str = "test";
> 
>      str[0] = 'a';
> 
> Literal strings will reside in rodata, so they are not modifiable.
> This will result to an permission fault at runtime if the permissions
> are enforced in the page-tables (this is the case in Xen).
> 
> I am not aware of code trying to modify literal strings in Xen.
> However, there is a frequent use of non-const char * to point to
> literal strings. Given the size of the codebase, there is a risk
> to involuntarily introduce code that will modify literal strings.
> 
> Therefore it would be better to enforce using const when pointing
> to such strings. Both GCC and Clang provide an option to warn
> for such case (see -Wwrite-strings) and therefore could be used
> by Xen.
> 
> This series doesn't yet make use of -Wwrite-strings because
> the tree is not fully converted. Instead, it contains some easy
> and likely non-controversial use const in the code.
> 
> The major blockers to enable -Wwrite-strings are the following:
>      - xen/common/efi: union string is used in both const and
>      non-const situation. It doesn't feel right to specific one member
>      const and the other non-const.
>      - libxl: the major block is the flexarray framework as we would use
>      it with string (now const char*). I thought it would be possible to
>      make the interface const, but it looks like there are a couple of
>      places where we need to modify the content (such as in
>      libxl_json.c).
> 
> Ideally, I would like to have -Wwrite-strings unconditionally used
> tree-wide. But, some of the area may required some heavy refactoring.
> 
> One solution would be to enable it tree-wide but turned it off at a
> directroy/file level.
> 
> Any opinions?
> 
> Cheers,
> 
> Julien Grall (14):
>    xen: Constify the second parameter of rangeset_new()
>    xen/sched: Constify name and opt_name in struct scheduler
>    xen/x86: shadow: The return type of sh_audit_flags() should be const
>    xen/char: console: Use const whenever we point to literal strings
>    tools/libs: guest: Use const whenever we point to literal strings
>    tools/libs: stat: Use const whenever we point to literal strings
>    tools/xl: Use const whenever we point to literal strings
>    tools/firmware: hvmloader: Use const in __bug() and __assert_failed()
>    tools/console: Use const whenever we point to literal strings
>    tools/kdd: Use const whenever we point to literal strings
>    tools/misc: Use const whenever we point to literal strings
>    tools/top: The string parameter in set_prompt() and set_delay() should
>      be const
>    tools/xenmon: xenbaked: Mark const the field text in stat_map_t
>    tools/xentrace: Use const whenever we point to literal strings
> 
>   tools/console/client/main.c         |  4 +-
>   tools/console/daemon/io.c           | 10 ++--
>   tools/debugger/kdd/kdd.c            | 10 ++--
>   tools/firmware/hvmloader/util.c     |  4 +-
>   tools/firmware/hvmloader/util.h     |  4 +-
>   tools/include/xenguest.h            | 10 ++--
>   tools/libs/guest/xg_dom_core.c      |  8 ++--
>   tools/libs/guest/xg_dom_elfloader.c |  4 +-
>   tools/libs/guest/xg_dom_hvmloader.c |  2 +-
>   tools/libs/guest/xg_dom_x86.c       |  9 ++--
>   tools/libs/guest/xg_private.h       |  2 +-
>   tools/libs/stat/xenstat_linux.c     |  4 +-
>   tools/libs/stat/xenstat_qmp.c       | 12 ++---
>   tools/misc/xen-detect.c             |  2 +-
>   tools/misc/xenhypfs.c               |  6 +--
>   tools/xenmon/xenbaked.c             |  2 +-
>   tools/xentop/xentop.c               | 12 ++---
>   tools/xentrace/xenalyze.c           | 71 +++++++++++++++--------------
>   tools/xentrace/xenctx.c             |  4 +-
>   tools/xl/xl.h                       |  8 ++--
>   tools/xl/xl_console.c               |  2 +-
>   tools/xl/xl_utils.c                 |  4 +-
>   tools/xl/xl_utils.h                 |  4 +-
>   xen/arch/x86/mm/shadow/multi.c      | 12 ++---
>   xen/common/rangeset.c               |  2 +-
>   xen/common/sched/private.h          |  4 +-
>   xen/drivers/char/console.c          |  4 +-
>   xen/include/xen/rangeset.h          |  2 +-
>   28 files changed, 113 insertions(+), 109 deletions(-)
> 

-- 
Julien Grall

Re: [PATCH 05/14] tools/libs: guest: Use const whenever we point to literal strings

[Anthony PERARD]

On Mon, Apr 05, 2021 at 04:57:04PM +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/libs/guest/xg_dom_x86.c b/tools/libs/guest/xg_dom_x86.c
> index 2953aeb90b35..e379b07f9945 100644
> --- a/tools/libs/guest/xg_dom_x86.c
> +++ b/tools/libs/guest/xg_dom_x86.c
> @@ -1148,11 +1148,12 @@ static int vcpu_hvm(struct xc_dom_image *dom)
>  
>  /* ------------------------------------------------------------------------ */
>  
> -static int x86_compat(xc_interface *xch, uint32_t domid, char *guest_type)
> +static int x86_compat(xc_interface *xch, uint32_t domid,
> +                      const char *guest_type)
>  {
>      static const struct {
> -        char           *guest;
> -        uint32_t        size;
> +        const char      *guest;
> +        uint32_t       size;

It seems that one space have been removed by mistake just before "size".

The rest looks good:
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [PATCH 06/14] tools/libs: stat: Use const whenever we point to literal strings

[Anthony PERARD]

On Mon, Apr 05, 2021 at 04:57:05PM +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>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

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

[Anthony PERARD]

On Mon, Apr 05, 2021 at 04:57:08PM +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/console/daemon/io.c b/tools/console/daemon/io.c
> index 4af27ffc5d02..6a8a94e31b65 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -109,9 +109,9 @@ struct console {
>  };
>  
>  struct console_type {
> -	char *xsname;
> -	char *ttyname;
> -	char *log_suffix;
> +	const char *xsname;

I think that const of `xsname` is lost in console_init() in the same
file.
We have:

    static int console_init(.. )
    {
        struct console_type **con_type = (struct console_type **)data;
        char *xsname, *xspath;
        xsname = (char *)(*con_type)->xsname;
    }

So constify "xsname" in console_init() should be part of the patch, I
think.

Thanks,

-- 
Anthony PERARD

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

[Anthony PERARD]

On Mon, Apr 05, 2021 at 04:57:10PM +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 we to store a pointer to them.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [PATCH 12/14] tools/top: The string parameter in set_prompt() and set_delay() should be const

[Anthony PERARD]

On Mon, Apr 05, 2021 at 04:57:11PM +0100, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Neither string parameter in set_prompt() and set_delay() are meant to
> be modified. In particular, new_prompt can point to a literal string.
> 
> So mark the two parameters as const and propagate it.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [PATCH 13/14] tools/xenmon: xenbaked: Mark const the field text in stat_map_t

[Anthony PERARD]

On Mon, Apr 05, 2021 at 04:57:12PM +0100, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The field text in stat_map_t will point to string literals. So mark it
> as const to allow the compiler to catch any modified of the string.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: PING Re: [PATCH 00/14] Use const whether we point to literal strings (take 1)

[Wei Liu]

On Mon, May 10, 2021 at 06:49:01PM +0100, Julien Grall wrote:
> Hi,
> 
> Ian, Wei, Anthony, can I get some feedbacks on the tools side?

I think this is moving to the right direction so

Acked-by: Wei Liu <wl@xen.org>

Re: [PATCH 05/14] tools/libs: guest: Use const whenever we point to literal strings

[Julien Grall]

Hi Anthony,

On 11/05/2021 15:58, Anthony PERARD wrote:
> On Mon, Apr 05, 2021 at 04:57:04PM +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/libs/guest/xg_dom_x86.c b/tools/libs/guest/xg_dom_x86.c
>> index 2953aeb90b35..e379b07f9945 100644
>> --- a/tools/libs/guest/xg_dom_x86.c
>> +++ b/tools/libs/guest/xg_dom_x86.c
>> @@ -1148,11 +1148,12 @@ static int vcpu_hvm(struct xc_dom_image *dom)
>>   
>>   /* ------------------------------------------------------------------------ */
>>   
>> -static int x86_compat(xc_interface *xch, uint32_t domid, char *guest_type)
>> +static int x86_compat(xc_interface *xch, uint32_t domid,
>> +                      const char *guest_type)
>>   {
>>       static const struct {
>> -        char           *guest;
>> -        uint32_t        size;
>> +        const char      *guest;
>> +        uint32_t       size;
> 
> It seems that one space have been removed by mistake just before "size".

Well spotted. I will fix on commit.

> 
> The rest looks good:
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thank you!

Cheers,

-- 
Julien Grall

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

[Julien Grall]

Hi Anthony,

On 11/05/2021 16:18, Anthony PERARD wrote:
> On Mon, Apr 05, 2021 at 04:57:08PM +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/console/daemon/io.c b/tools/console/daemon/io.c
>> index 4af27ffc5d02..6a8a94e31b65 100644
>> --- a/tools/console/daemon/io.c
>> +++ b/tools/console/daemon/io.c
>> @@ -109,9 +109,9 @@ struct console {
>>   };
>>   
>>   struct console_type {
>> -	char *xsname;
>> -	char *ttyname;
>> -	char *log_suffix;
>> +	const char *xsname;
> 
> I think that const of `xsname` is lost in console_init() in the same
> file.
> We have:
> 
>      static int console_init(.. )
>      {
>          struct console_type **con_type = (struct console_type **)data;
>          char *xsname, *xspath;
>          xsname = (char *)(*con_type)->xsname;
>      }
> 
> So constify "xsname" in console_init() should be part of the patch, I
> think.

Good point. I am not entirely sure why the cast has been added because 
the field has always been a char *.

Anyway, I will remove it.

Cheers,

-- 
Julien Grall

Re: PING Re: [PATCH 00/14] Use const whether we point to literal strings (take 1)

[Julien Grall]

Hi Wei,

On 17/05/2021 19:41, Wei Liu wrote:
> On Mon, May 10, 2021 at 06:49:01PM +0100, Julien Grall wrote:
>> Hi,
>>
>> Ian, Wei, Anthony, can I get some feedbacks on the tools side?
> 
> I think this is moving to the right direction so
> 
> Acked-by: Wei Liu <wl@xen.org>

Thanks! I had committed most of the tools code but one patch. I have 
kept your acked-by on the patch and will wait Anthony to do a full review.

Cheers,

-- 
Julien Grall