[PATCH 5/5] tools/xl: Fix potential deallocation bug

[PATCH 5/5] tools/xl: Fix potential deallocation bug

[Elliott Mitchell]

There is potential for the info and info_free variable's purposes to
diverge.  If info was overwritten with a distinct value, yet info_free
still needed deallocation a bug would occur on this line.  Preemptively
address this issue (making use of divergent info/info_free values is
under consideration).

Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
---
 tools/xl/xl_info.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
index 3647468420..938f06f1a8 100644
--- a/tools/xl/xl_info.c
+++ b/tools/xl/xl_info.c
@@ -579,7 +579,7 @@ int main_list(int argc, char **argv)
                      info, nb_domain);
 
     if (info_free)
-        libxl_dominfo_list_free(info, nb_domain);
+        libxl_dominfo_list_free(info_free, nb_domain);
 
     libxl_dominfo_dispose(&info_buf);
 
-- 
2.30.2

[PATCH 3/5] tools/xl: Rename printf_info()/list_domains_details() to dump_by_...()

[Elliott Mitchell]

printf_info()/list_domains_details() had been serving fairly similar
purposes.  Increase their consistency (add file-handle and output_format
arguments to list_domains_details(), reorder arguments) and then rename
to better reflect their functionality.

Both were simply outputting full domain information.  As this is more of
a dump operation, "dump" is a better name.

Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
---
 tools/xl/xl.h      |  8 ++++++++
 tools/xl/xl_info.c | 30 ++++++++++++++++--------------
 tools/xl/xl_misc.c |  5 +----
 3 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index 720adb0048..be5f4e11fe 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -300,6 +300,14 @@ typedef enum {
     DOMAIN_RESTART_SOFT_RESET,   /* Soft reset should be performed */
 } domain_restart_type;
 
+extern void dump_by_config(enum output_format output_format,
+                           FILE *fh,
+                           const libxl_domain_config *d_config,
+                           int domid);
+extern void dump_by_dominfo_list(enum output_format output_format,
+                                 FILE *fh,
+                                 const libxl_dominfo info[],
+                                 int nb_domain);
 extern void printf_info_sexp(int domid, const libxl_domain_config *d_config, FILE *fh);
 extern void apply_global_affinity_masks(libxl_domain_type type,
                                         libxl_bitmap *vcpu_affinity_array,
diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
index 23d82ce2a2..3647468420 100644
--- a/tools/xl/xl_info.c
+++ b/tools/xl/xl_info.c
@@ -94,12 +94,10 @@ out:
     return s;
 }
 
-void printf_info(enum output_format output_format,
-                 int domid,
-                 libxl_domain_config *d_config, FILE *fh);
-void printf_info(enum output_format output_format,
-                 int domid,
-                 libxl_domain_config *d_config, FILE *fh)
+void dump_by_config(enum output_format output_format,
+                    FILE *fh,
+                    const libxl_domain_config *const d_config,
+                    int domid)
 {
     if (output_format == OUTPUT_FORMAT_SXP)
         return printf_info_sexp(domid, d_config, fh);
@@ -442,7 +440,10 @@ static void list_domains(bool verbose, bool context, bool claim, bool numa,
     libxl_physinfo_dispose(&physinfo);
 }
 
-static void list_domains_details(const libxl_dominfo *info, int nb_domain)
+void dump_by_dominfo_list(enum output_format output_format,
+                          FILE *fh,
+                          const libxl_dominfo info[],
+                          int nb_domain)
 {
     libxl_domain_config d_config;
 
@@ -453,7 +454,7 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
     const char *buf;
     libxl_yajl_length yajl_len = 0;
 
-    if (default_output_format == OUTPUT_FORMAT_JSON) {
+    if (output_format == OUTPUT_FORMAT_JSON) {
         hand = libxl_yajl_gen_alloc(NULL);
         if (!hand) {
             fprintf(stderr, "unable to allocate JSON generator\n");
@@ -472,16 +473,16 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
                                                  &d_config, NULL);
         if (rc)
             continue;
-        if (default_output_format == OUTPUT_FORMAT_JSON)
+        if (output_format == OUTPUT_FORMAT_JSON)
             s = printf_info_one_json(hand, info[i].domid, &d_config);
         else
-            printf_info_sexp(info[i].domid, &d_config, stdout);
+            printf_info_sexp(info[i].domid, &d_config, fh);
         libxl_domain_config_dispose(&d_config);
         if (s != yajl_gen_status_ok)
             goto out;
     }
 
-    if (default_output_format == OUTPUT_FORMAT_JSON) {
+    if (output_format == OUTPUT_FORMAT_JSON) {
         s = yajl_gen_array_close(hand);
         if (s != yajl_gen_status_ok)
             goto out;
@@ -490,11 +491,12 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
         if (s != yajl_gen_status_ok)
             goto out;
 
-        puts(buf);
+        fputs(buf, fh);
+        fputc('\n', fh);
     }
 
 out:
-    if (default_output_format == OUTPUT_FORMAT_JSON) {
+    if (output_format == OUTPUT_FORMAT_JSON) {
         yajl_gen_free(hand);
         if (s != yajl_gen_status_ok)
             fprintf(stderr,
@@ -571,7 +573,7 @@ int main_list(int argc, char **argv)
     }
 
     if (details)
-        list_domains_details(info, nb_domain);
+        dump_by_dominfo_list(default_output_format, stdout, info, nb_domain);
     else
         list_domains(verbose, context, false /* claim */, numa, cpupool,
                      info, nb_domain);
diff --git a/tools/xl/xl_misc.c b/tools/xl/xl_misc.c
index 08f0fb6dc9..bcf178762b 100644
--- a/tools/xl/xl_misc.c
+++ b/tools/xl/xl_misc.c
@@ -256,9 +256,6 @@ int main_dump_core(int argc, char **argv)
     return EXIT_SUCCESS;
 }
 
-extern void printf_info(enum output_format output_format,
-                        int domid,
-                        libxl_domain_config *d_config, FILE *fh);
 int main_config_update(int argc, char **argv)
 {
     uint32_t domid;
@@ -344,7 +341,7 @@ int main_config_update(int argc, char **argv)
     parse_config_data(filename, config_data, config_len, &d_config);
 
     if (debug || dryrun_only)
-        printf_info(default_output_format, -1, &d_config, stdout);
+        dump_by_config(default_output_format, stdout, &d_config, -1);
 
     if (!dryrun_only) {
         fprintf(stderr, "setting dom%u configuration\n", domid);
-- 
2.30.2

[PATCH 4/5] tools/xl: Merge down debug/dry-run section of create_domain()

[Elliott Mitchell]

create_domain()'s use of printf_info_sexp() could be merged down to a
single dump_by_config(), do so.  This results in an extra JSON dictionary
in output, but I doubt that is an issue for dry-run or debugging output.

Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
---
 tools/xl/xl_vmcontrol.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 435155a033..4b95e7e463 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -856,19 +856,7 @@ int create_domain(struct domain_create *dom_info)
 
     if (debug || dom_info->dryrun) {
         FILE *cfg_print_fh = (debug && !dom_info->dryrun) ? stderr : stdout;
-        if (default_output_format == OUTPUT_FORMAT_SXP) {
-            printf_info_sexp(-1, &d_config, cfg_print_fh);
-        } else {
-            char *json = libxl_domain_config_to_json(ctx, &d_config);
-            if (!json) {
-                fprintf(stderr,
-                        "Failed to convert domain configuration to JSON\n");
-                exit(1);
-            }
-            fputs(json, cfg_print_fh);
-            free(json);
-            flush_stream(cfg_print_fh);
-        }
+        dump_by_config(default_output_format, cfg_print_fh, &d_config, -1);
     }
 
 
-- 
2.30.2

[PATCH 2/5] tools/xl: Mark libxl_domain_config * arg of printf_info_*() const

[Elliott Mitchell]

With libxl having gotten a lot more constant, now printf_info_sexp() and
printf_info_one_json() can add consts.  May not be particularly
important, but it is best to mark things constant when they are known to
be so.

Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
---
 tools/xl/xl.h      | 2 +-
 tools/xl/xl_info.c | 2 +-
 tools/xl/xl_sxp.c  | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index c5c4bedbdd..720adb0048 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -300,7 +300,7 @@ typedef enum {
     DOMAIN_RESTART_SOFT_RESET,   /* Soft reset should be performed */
 } domain_restart_type;
 
-extern void printf_info_sexp(int domid, libxl_domain_config *d_config, FILE *fh);
+extern void printf_info_sexp(int domid, const libxl_domain_config *d_config, FILE *fh);
 extern void apply_global_affinity_masks(libxl_domain_type type,
                                         libxl_bitmap *vcpu_affinity_array,
                                         unsigned int size);
diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
index 712b7638b0..23d82ce2a2 100644
--- a/tools/xl/xl_info.c
+++ b/tools/xl/xl_info.c
@@ -59,7 +59,7 @@ static int maybe_printf(const char *fmt, ...)
 }
 
 static yajl_gen_status printf_info_one_json(yajl_gen hand, int domid,
-                                            libxl_domain_config *d_config)
+                                            const libxl_domain_config *d_config)
 {
     yajl_gen_status s;
 
diff --git a/tools/xl/xl_sxp.c b/tools/xl/xl_sxp.c
index 359a001570..d5b9051dfc 100644
--- a/tools/xl/xl_sxp.c
+++ b/tools/xl/xl_sxp.c
@@ -26,13 +26,13 @@
 /* In general you should not add new output to this function since it
  * is intended only for legacy use.
  */
-void printf_info_sexp(int domid, libxl_domain_config *d_config, FILE *fh)
+void printf_info_sexp(int domid, const libxl_domain_config *d_config, FILE *fh)
 {
     int i;
     libxl_dominfo info;
 
-    libxl_domain_create_info *c_info = &d_config->c_info;
-    libxl_domain_build_info *b_info = &d_config->b_info;
+    const libxl_domain_create_info *c_info = &d_config->c_info;
+    const libxl_domain_build_info *b_info = &d_config->b_info;
 
     fprintf(fh, "(domain\n\t(domid %d)\n", domid);
     fprintf(fh, "\t(create_info)\n");
-- 
2.30.2

[PATCH 1/5] tools/libxl: Mark pointer args of many functions constant

[Elliott Mitchell]

Anything *_is_empty(), *_is_default(), or *_gen_json() is going to be
examining the pointed to thing, not modifying it.  This potentially
results in higher-performance output.  This also allows spreading
constants further, allowing more checking and security.

Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
---
 tools/include/libxl_json.h        | 22 ++++++++++++----------
 tools/libs/light/gentypes.py      |  8 ++++----
 tools/libs/light/libxl_cpuid.c    |  2 +-
 tools/libs/light/libxl_internal.c |  4 ++--
 tools/libs/light/libxl_internal.h | 18 +++++++++---------
 tools/libs/light/libxl_json.c     | 18 ++++++++++--------
 tools/libs/light/libxl_nocpuid.c  |  4 ++--
 7 files changed, 40 insertions(+), 36 deletions(-)

diff --git a/tools/include/libxl_json.h b/tools/include/libxl_json.h
index 260783bfde..63f0e58fe1 100644
--- a/tools/include/libxl_json.h
+++ b/tools/include/libxl_json.h
@@ -23,17 +23,19 @@
 #endif
 
 yajl_gen_status libxl__uint64_gen_json(yajl_gen hand, uint64_t val);
-yajl_gen_status libxl_defbool_gen_json(yajl_gen hand, libxl_defbool *p);
-yajl_gen_status libxl_uuid_gen_json(yajl_gen hand, libxl_uuid *p);
-yajl_gen_status libxl_mac_gen_json(yajl_gen hand, libxl_mac *p);
-yajl_gen_status libxl_bitmap_gen_json(yajl_gen hand, libxl_bitmap *p);
+yajl_gen_status libxl_defbool_gen_json(yajl_gen hand, const libxl_defbool *p);
+yajl_gen_status libxl_uuid_gen_json(yajl_gen hand, const libxl_uuid *p);
+yajl_gen_status libxl_mac_gen_json(yajl_gen hand, const libxl_mac *p);
+yajl_gen_status libxl_bitmap_gen_json(yajl_gen hand, const libxl_bitmap *p);
 yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
-                                                 libxl_cpuid_policy_list *p);
-yajl_gen_status libxl_string_list_gen_json(yajl_gen hand, libxl_string_list *p);
+                                                 const libxl_cpuid_policy_list *p);
+yajl_gen_status libxl_string_list_gen_json(yajl_gen hand,
+                                           const libxl_string_list *p);
 yajl_gen_status libxl_key_value_list_gen_json(yajl_gen hand,
-                                              libxl_key_value_list *p);
-yajl_gen_status libxl_hwcap_gen_json(yajl_gen hand, libxl_hwcap *p);
-yajl_gen_status libxl_ms_vm_genid_gen_json(yajl_gen hand, libxl_ms_vm_genid *p);
+                                              const libxl_key_value_list *p);
+yajl_gen_status libxl_hwcap_gen_json(yajl_gen hand, const libxl_hwcap *p);
+yajl_gen_status libxl_ms_vm_genid_gen_json(yajl_gen hand,
+                                           const libxl_ms_vm_genid *p);
 
 #include <_libxl_types_json.h>
 
@@ -91,6 +93,6 @@ static inline yajl_gen libxl_yajl_gen_alloc(const yajl_alloc_funcs *allocFuncs)
 #endif /* !HAVE_YAJL_V2 */
 
 yajl_gen_status libxl_domain_config_gen_json(yajl_gen hand,
-                                             libxl_domain_config *p);
+                                             const libxl_domain_config *p);
 
 #endif /* LIBXL_JSON_H */
diff --git a/tools/libs/light/gentypes.py b/tools/libs/light/gentypes.py
index 9a45e45acc..7e02a5366f 100644
--- a/tools/libs/light/gentypes.py
+++ b/tools/libs/light/gentypes.py
@@ -632,7 +632,7 @@ if __name__ == '__main__':
                                                ty.make_arg("p"),
                                                ku.keyvar.type.make_arg(ku.keyvar.name)))
         if ty.json_gen_fn is not None:
-            f.write("%schar *%s_to_json(libxl_ctx *ctx, %s);\n" % (ty.hidden(), ty.typename, ty.make_arg("p")))
+            f.write("%schar *%s_to_json(libxl_ctx *ctx, const %s);\n" % (ty.hidden(), ty.typename, ty.make_arg("p")))
         if ty.json_parse_fn is not None:
             f.write("%sint %s_from_json(libxl_ctx *ctx, %s, const char *s);\n" % (ty.hidden(), ty.typename, ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
         if isinstance(ty, idl.Enumeration):
@@ -662,7 +662,7 @@ if __name__ == '__main__':
 """ % (header_json_define, header_json_define, " ".join(sys.argv)))
 
     for ty in [ty for ty in types if ty.json_gen_fn is not None]:
-        f.write("%syajl_gen_status %s_gen_json(yajl_gen hand, %s);\n" % (ty.hidden(), ty.typename, ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
+        f.write("%syajl_gen_status %s_gen_json(yajl_gen hand, const %s);\n" % (ty.hidden(), ty.typename, ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
 
     f.write("\n")
     f.write("""#endif /* %s */\n""" % header_json_define)
@@ -766,13 +766,13 @@ if __name__ == '__main__':
         f.write("\n")
 
     for ty in [t for t in types if t.json_gen_fn is not None]:
-        f.write("yajl_gen_status %s_gen_json(yajl_gen hand, %s)\n" % (ty.typename, ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
+        f.write("yajl_gen_status %s_gen_json(yajl_gen hand, const %s)\n" % (ty.typename, ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
         f.write("{\n")
         f.write(libxl_C_type_gen_json(ty, "p"))
         f.write("}\n")
         f.write("\n")
 
-        f.write("char *%s_to_json(libxl_ctx *ctx, %s)\n" % (ty.typename, ty.make_arg("p")))
+        f.write("char *%s_to_json(libxl_ctx *ctx, const %s)\n" % (ty.typename, ty.make_arg("p")))
         f.write("{\n")
         f.write(libxl_C_type_to_json(ty, "p"))
         f.write("}\n")
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index e1acf6648d..b076d7f4a3 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -14,7 +14,7 @@
 
 #include "libxl_internal.h"
 
-int libxl__cpuid_policy_is_empty(libxl_cpuid_policy_list *pl)
+int libxl__cpuid_policy_is_empty(const libxl_cpuid_policy_list *pl)
 {
     return !libxl_cpuid_policy_list_length(pl);
 }
diff --git a/tools/libs/light/libxl_internal.c b/tools/libs/light/libxl_internal.c
index 86556b6113..da2dbd67ad 100644
--- a/tools/libs/light/libxl_internal.c
+++ b/tools/libs/light/libxl_internal.c
@@ -333,7 +333,7 @@ _hidden int libxl__parse_mac(const char *s, libxl_mac mac)
     return 0;
 }
 
-_hidden int libxl__compare_macs(libxl_mac *a, libxl_mac *b)
+_hidden int libxl__compare_macs(const libxl_mac *a, const libxl_mac *b)
 {
     int i;
 
@@ -345,7 +345,7 @@ _hidden int libxl__compare_macs(libxl_mac *a, libxl_mac *b)
     return 0;
 }
 
-_hidden int libxl__mac_is_default(libxl_mac *mac)
+_hidden int libxl__mac_is_default(const libxl_mac *mac)
 {
     return (!(*mac)[0] && !(*mac)[1] && !(*mac)[2] &&
             !(*mac)[3] && !(*mac)[4] && !(*mac)[5]);
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index 37d5c27756..117a98acab 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -2080,9 +2080,9 @@ struct libxl__xen_console_reader {
 /* parse the string @s as a sequence of 6 colon separated bytes in to @mac */
 _hidden int libxl__parse_mac(const char *s, libxl_mac mac);
 /* compare mac address @a and @b. 0 if the same, -ve if a<b and +ve if a>b */
-_hidden int libxl__compare_macs(libxl_mac *a, libxl_mac *b);
+_hidden int libxl__compare_macs(const libxl_mac *a, const libxl_mac *b);
 /* return true if mac address is all zero (the default value) */
-_hidden int libxl__mac_is_default(libxl_mac *mac);
+_hidden int libxl__mac_is_default(const libxl_mac *mac);
 /* init a recursive mutex */
 _hidden int libxl__init_recursive_mutex(libxl_ctx *ctx, pthread_mutex_t *lock);
 
@@ -4580,7 +4580,7 @@ _hidden int libxl__ms_vm_genid_set(libxl__gc *gc, uint32_t domid,
 #define LIBXL__DEFBOOL_STR_DEFAULT "<default>"
 #define LIBXL__DEFBOOL_STR_FALSE   "False"
 #define LIBXL__DEFBOOL_STR_TRUE    "True"
-static inline int libxl__defbool_is_default(libxl_defbool *db)
+static inline int libxl__defbool_is_default(const libxl_defbool *db)
 {
     return !db->val;
 }
@@ -4675,22 +4675,22 @@ int libxl__random_bytes(libxl__gc *gc, uint8_t *buf, size_t len);
 #include "_libxl_types_internal_private.h"
 
 /* This always return false, there's no "default value" for hw cap */
-static inline int libxl__hwcap_is_default(libxl_hwcap *hwcap)
+static inline int libxl__hwcap_is_default(const libxl_hwcap *hwcap)
 {
     return 0;
 }
 
-static inline int libxl__string_list_is_empty(libxl_string_list *psl)
+static inline int libxl__string_list_is_empty(const libxl_string_list *psl)
 {
     return !libxl_string_list_length(psl);
 }
 
-static inline int libxl__key_value_list_is_empty(libxl_key_value_list *pkvl)
+static inline int libxl__key_value_list_is_empty(const libxl_key_value_list *pkvl)
 {
     return !libxl_key_value_list_length(pkvl);
 }
 
-int libxl__cpuid_policy_is_empty(libxl_cpuid_policy_list *pl);
+int libxl__cpuid_policy_is_empty(const libxl_cpuid_policy_list *pl);
 
 /* Portability note: a proper flock(2) implementation is required */
 typedef struct {
@@ -4821,12 +4821,12 @@ void* libxl__device_list(libxl__gc *gc, const libxl__device_type *dt,
 void libxl__device_list_free(const libxl__device_type *dt,
                              void *list, int num);
 
-static inline bool libxl__timer_mode_is_default(libxl_timer_mode *tm)
+static inline bool libxl__timer_mode_is_default(const libxl_timer_mode *tm)
 {
     return *tm == LIBXL_TIMER_MODE_DEFAULT;
 }
 
-static inline bool libxl__string_is_default(char **s)
+static inline bool libxl__string_is_default(char *const *s)
 {
     return *s == NULL;
 }
diff --git a/tools/libs/light/libxl_json.c b/tools/libs/light/libxl_json.c
index 9b8ef2cab9..88e81f9905 100644
--- a/tools/libs/light/libxl_json.c
+++ b/tools/libs/light/libxl_json.c
@@ -95,7 +95,7 @@ yajl_gen_status libxl__yajl_gen_enum(yajl_gen hand, const char *str)
  * YAJL generators for builtin libxl types.
  */
 yajl_gen_status libxl_defbool_gen_json(yajl_gen hand,
-                                       libxl_defbool *db)
+                                       const libxl_defbool *db)
 {
     return libxl__yajl_gen_asciiz(hand, libxl_defbool_to_string(*db));
 }
@@ -137,7 +137,7 @@ int libxl__bool_parse_json(libxl__gc *gc, const libxl__json_object *o,
 }
 
 yajl_gen_status libxl_uuid_gen_json(yajl_gen hand,
-                                    libxl_uuid *uuid)
+                                    const libxl_uuid *uuid)
 {
     char buf[LIBXL_UUID_FMTLEN+1];
     snprintf(buf, sizeof(buf), LIBXL_UUID_FMT, LIBXL_UUID_BYTES((*uuid)));
@@ -154,7 +154,7 @@ int libxl__uuid_parse_json(libxl__gc *gc, const libxl__json_object *o,
 }
 
 yajl_gen_status libxl_bitmap_gen_json(yajl_gen hand,
-                                      libxl_bitmap *bitmap)
+                                      const libxl_bitmap *bitmap)
 {
     yajl_gen_status s;
     int i;
@@ -208,7 +208,7 @@ int libxl__bitmap_parse_json(libxl__gc *gc, const libxl__json_object *o,
 }
 
 yajl_gen_status libxl_key_value_list_gen_json(yajl_gen hand,
-                                              libxl_key_value_list *pkvl)
+                                              const libxl_key_value_list *pkvl)
 {
     libxl_key_value_list kvl = *pkvl;
     yajl_gen_status s;
@@ -269,7 +269,8 @@ int libxl__key_value_list_parse_json(libxl__gc *gc, const libxl__json_object *o,
     return 0;
 }
 
-yajl_gen_status libxl_string_list_gen_json(yajl_gen hand, libxl_string_list *pl)
+yajl_gen_status libxl_string_list_gen_json(yajl_gen hand,
+                                           const libxl_string_list *pl)
 {
     libxl_string_list l = *pl;
     yajl_gen_status s;
@@ -322,7 +323,7 @@ int libxl__string_list_parse_json(libxl__gc *gc, const libxl__json_object *o,
     return 0;
 }
 
-yajl_gen_status libxl_mac_gen_json(yajl_gen hand, libxl_mac *mac)
+yajl_gen_status libxl_mac_gen_json(yajl_gen hand, const libxl_mac *mac)
 {
     char buf[LIBXL_MAC_FMTLEN+1];
     snprintf(buf, sizeof(buf), LIBXL_MAC_FMT, LIBXL_MAC_BYTES((*mac)));
@@ -339,7 +340,7 @@ int libxl__mac_parse_json(libxl__gc *gc, const libxl__json_object *o,
 }
 
 yajl_gen_status libxl_hwcap_gen_json(yajl_gen hand,
-                                     libxl_hwcap *p)
+                                     const libxl_hwcap *p)
 {
     yajl_gen_status s;
     int i;
@@ -377,7 +378,8 @@ int libxl__hwcap_parse_json(libxl__gc *gc, const libxl__json_object *o,
     return 0;
 }
 
-yajl_gen_status libxl_ms_vm_genid_gen_json(yajl_gen hand, libxl_ms_vm_genid *p)
+yajl_gen_status libxl_ms_vm_genid_gen_json(yajl_gen hand,
+                                           const libxl_ms_vm_genid *p)
 {
     yajl_gen_status s;
     int i;
diff --git a/tools/libs/light/libxl_nocpuid.c b/tools/libs/light/libxl_nocpuid.c
index 0630959e76..f40a004e95 100644
--- a/tools/libs/light/libxl_nocpuid.c
+++ b/tools/libs/light/libxl_nocpuid.c
@@ -14,7 +14,7 @@
 
 #include "libxl_internal.h"
 
-int libxl__cpuid_policy_is_empty(libxl_cpuid_policy_list *pl)
+int libxl__cpuid_policy_is_empty(const libxl_cpuid_policy_list *pl)
 {
     return 1;
 }
@@ -41,7 +41,7 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
 }
 
 yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
-                                libxl_cpuid_policy_list *pcpuid)
+                                const libxl_cpuid_policy_list *pcpuid)
 {
     return 0;
 }
-- 
2.30.2

[PATCH 0/5] Some misc from my tree

[Elliott Mitchell]

Some bits I had run across when looking at a piece of libxl.

Even though the purpose of the functions suggests they won't modify the
memory passed in via pointer, marking them constant provides better
reassurance.

While list_domains_details() and printf_info() had similar purposes their
interfaces were wildly different.  Make them more consistent.  I was
aiming to turn dump_by_dominfo_list() into a wrapper around
dump_by_config(), but didn't quite get there.

Having the functions named "printf" is wrong.  They are generating output
in a fixed format, there is no control over the format.  That is a write
or dump operation, not a formatted print.

Elliott Mitchell (5):
  tools/libxl: Mark pointer args of many functions constant
  tools/xl: Mark libxl_domain_config * arg of printf_info_*() const
  tools/xl: Rename printf_info()/list_domains_details() to dump_by_...()
  tools/xl: Merge down debug/dry-run section of create_domain()
  tools/xl: Fix potential deallocation bug

 tools/include/libxl_json.h        | 22 +++++++++++---------
 tools/libs/light/gentypes.py      |  8 ++++----
 tools/libs/light/libxl_cpuid.c    |  2 +-
 tools/libs/light/libxl_internal.c |  4 ++--
 tools/libs/light/libxl_internal.h | 18 ++++++++--------
 tools/libs/light/libxl_json.c     | 18 ++++++++--------
 tools/libs/light/libxl_nocpuid.c  |  4 ++--
 tools/xl/xl.h                     | 10 ++++++++-
 tools/xl/xl_info.c                | 34 ++++++++++++++++---------------
 tools/xl/xl_misc.c                |  5 +----
 tools/xl/xl_sxp.c                 |  6 +++---
 tools/xl/xl_vmcontrol.c           | 14 +------------
 12 files changed, 72 insertions(+), 73 deletions(-)

-- 
2.30.2

Re: [PATCH 1/5] tools/libxl: Mark pointer args of many functions constant

[Luca Fancellu]

> On 18 Dec 2020, at 21:37, Elliott Mitchell <ehem+xen@m5p.com> wrote:
> 
> Anything *_is_empty(), *_is_default(), or *_gen_json() is going to be
> examining the pointed to thing, not modifying it.  This potentially
> results in higher-performance output.  This also allows spreading
> constants further, allowing more checking and security.

Looks ok to me
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

> 
> Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
> ---
> tools/include/libxl_json.h        | 22 ++++++++++++----------
> tools/libs/light/gentypes.py      |  8 ++++----
> tools/libs/light/libxl_cpuid.c    |  2 +-
> tools/libs/light/libxl_internal.c |  4 ++--
> tools/libs/light/libxl_internal.h | 18 +++++++++---------
> tools/libs/light/libxl_json.c     | 18 ++++++++++--------
> tools/libs/light/libxl_nocpuid.c  |  4 ++--
> 7 files changed, 40 insertions(+), 36 deletions(-)
> 
> diff --git a/tools/include/libxl_json.h b/tools/include/libxl_json.h
> index 260783bfde..63f0e58fe1 100644
> --- a/tools/include/libxl_json.h
> +++ b/tools/include/libxl_json.h
> @@ -23,17 +23,19 @@
> #endif
> 
> yajl_gen_status libxl__uint64_gen_json(yajl_gen hand, uint64_t val);
> -yajl_gen_status libxl_defbool_gen_json(yajl_gen hand, libxl_defbool *p);
> -yajl_gen_status libxl_uuid_gen_json(yajl_gen hand, libxl_uuid *p);
> -yajl_gen_status libxl_mac_gen_json(yajl_gen hand, libxl_mac *p);
> -yajl_gen_status libxl_bitmap_gen_json(yajl_gen hand, libxl_bitmap *p);
> +yajl_gen_status libxl_defbool_gen_json(yajl_gen hand, const libxl_defbool *p);
> +yajl_gen_status libxl_uuid_gen_json(yajl_gen hand, const libxl_uuid *p);
> +yajl_gen_status libxl_mac_gen_json(yajl_gen hand, const libxl_mac *p);
> +yajl_gen_status libxl_bitmap_gen_json(yajl_gen hand, const libxl_bitmap *p);
> yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
> -                                                 libxl_cpuid_policy_list *p);
> -yajl_gen_status libxl_string_list_gen_json(yajl_gen hand, libxl_string_list *p);
> +                                                 const libxl_cpuid_policy_list *p);
> +yajl_gen_status libxl_string_list_gen_json(yajl_gen hand,
> +                                           const libxl_string_list *p);
> yajl_gen_status libxl_key_value_list_gen_json(yajl_gen hand,
> -                                              libxl_key_value_list *p);
> -yajl_gen_status libxl_hwcap_gen_json(yajl_gen hand, libxl_hwcap *p);
> -yajl_gen_status libxl_ms_vm_genid_gen_json(yajl_gen hand, libxl_ms_vm_genid *p);
> +                                              const libxl_key_value_list *p);
> +yajl_gen_status libxl_hwcap_gen_json(yajl_gen hand, const libxl_hwcap *p);
> +yajl_gen_status libxl_ms_vm_genid_gen_json(yajl_gen hand,
> +                                           const libxl_ms_vm_genid *p);
> 
> #include <_libxl_types_json.h>
> 
> @@ -91,6 +93,6 @@ static inline yajl_gen libxl_yajl_gen_alloc(const yajl_alloc_funcs *allocFuncs)
> #endif /* !HAVE_YAJL_V2 */
> 
> yajl_gen_status libxl_domain_config_gen_json(yajl_gen hand,
> -                                             libxl_domain_config *p);
> +                                             const libxl_domain_config *p);
> 
> #endif /* LIBXL_JSON_H */
> diff --git a/tools/libs/light/gentypes.py b/tools/libs/light/gentypes.py
> index 9a45e45acc..7e02a5366f 100644
> --- a/tools/libs/light/gentypes.py
> +++ b/tools/libs/light/gentypes.py
> @@ -632,7 +632,7 @@ if __name__ == '__main__':
>                                                ty.make_arg("p"),
>                                                ku.keyvar.type.make_arg(ku.keyvar.name)))
>         if ty.json_gen_fn is not None:
> -            f.write("%schar *%s_to_json(libxl_ctx *ctx, %s);\n" % (ty.hidden(), ty.typename, ty.make_arg("p")))
> +            f.write("%schar *%s_to_json(libxl_ctx *ctx, const %s);\n" % (ty.hidden(), ty.typename, ty.make_arg("p")))
>         if ty.json_parse_fn is not None:
>             f.write("%sint %s_from_json(libxl_ctx *ctx, %s, const char *s);\n" % (ty.hidden(), ty.typename, ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
>         if isinstance(ty, idl.Enumeration):
> @@ -662,7 +662,7 @@ if __name__ == '__main__':
> """ % (header_json_define, header_json_define, " ".join(sys.argv)))
> 
>     for ty in [ty for ty in types if ty.json_gen_fn is not None]:
> -        f.write("%syajl_gen_status %s_gen_json(yajl_gen hand, %s);\n" % (ty.hidden(), ty.typename, ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
> +        f.write("%syajl_gen_status %s_gen_json(yajl_gen hand, const %s);\n" % (ty.hidden(), ty.typename, ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
> 
>     f.write("\n")
>     f.write("""#endif /* %s */\n""" % header_json_define)
> @@ -766,13 +766,13 @@ if __name__ == '__main__':
>         f.write("\n")
> 
>     for ty in [t for t in types if t.json_gen_fn is not None]:
> -        f.write("yajl_gen_status %s_gen_json(yajl_gen hand, %s)\n" % (ty.typename, ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
> +        f.write("yajl_gen_status %s_gen_json(yajl_gen hand, const %s)\n" % (ty.typename, ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
>         f.write("{\n")
>         f.write(libxl_C_type_gen_json(ty, "p"))
>         f.write("}\n")
>         f.write("\n")
> 
> -        f.write("char *%s_to_json(libxl_ctx *ctx, %s)\n" % (ty.typename, ty.make_arg("p")))
> +        f.write("char *%s_to_json(libxl_ctx *ctx, const %s)\n" % (ty.typename, ty.make_arg("p")))
>         f.write("{\n")
>         f.write(libxl_C_type_to_json(ty, "p"))
>         f.write("}\n")
> diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> index e1acf6648d..b076d7f4a3 100644
> --- a/tools/libs/light/libxl_cpuid.c
> +++ b/tools/libs/light/libxl_cpuid.c
> @@ -14,7 +14,7 @@
> 
> #include "libxl_internal.h"
> 
> -int libxl__cpuid_policy_is_empty(libxl_cpuid_policy_list *pl)
> +int libxl__cpuid_policy_is_empty(const libxl_cpuid_policy_list *pl)
> {
>     return !libxl_cpuid_policy_list_length(pl);
> }
> diff --git a/tools/libs/light/libxl_internal.c b/tools/libs/light/libxl_internal.c
> index 86556b6113..da2dbd67ad 100644
> --- a/tools/libs/light/libxl_internal.c
> +++ b/tools/libs/light/libxl_internal.c
> @@ -333,7 +333,7 @@ _hidden int libxl__parse_mac(const char *s, libxl_mac mac)
>     return 0;
> }
> 
> -_hidden int libxl__compare_macs(libxl_mac *a, libxl_mac *b)
> +_hidden int libxl__compare_macs(const libxl_mac *a, const libxl_mac *b)
> {
>     int i;
> 
> @@ -345,7 +345,7 @@ _hidden int libxl__compare_macs(libxl_mac *a, libxl_mac *b)
>     return 0;
> }
> 
> -_hidden int libxl__mac_is_default(libxl_mac *mac)
> +_hidden int libxl__mac_is_default(const libxl_mac *mac)
> {
>     return (!(*mac)[0] && !(*mac)[1] && !(*mac)[2] &&
>             !(*mac)[3] && !(*mac)[4] && !(*mac)[5]);
> diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
> index 37d5c27756..117a98acab 100644
> --- a/tools/libs/light/libxl_internal.h
> +++ b/tools/libs/light/libxl_internal.h
> @@ -2080,9 +2080,9 @@ struct libxl__xen_console_reader {
> /* parse the string @s as a sequence of 6 colon separated bytes in to @mac */
> _hidden int libxl__parse_mac(const char *s, libxl_mac mac);
> /* compare mac address @a and @b. 0 if the same, -ve if a<b and +ve if a>b */
> -_hidden int libxl__compare_macs(libxl_mac *a, libxl_mac *b);
> +_hidden int libxl__compare_macs(const libxl_mac *a, const libxl_mac *b);
> /* return true if mac address is all zero (the default value) */
> -_hidden int libxl__mac_is_default(libxl_mac *mac);
> +_hidden int libxl__mac_is_default(const libxl_mac *mac);
> /* init a recursive mutex */
> _hidden int libxl__init_recursive_mutex(libxl_ctx *ctx, pthread_mutex_t *lock);
> 
> @@ -4580,7 +4580,7 @@ _hidden int libxl__ms_vm_genid_set(libxl__gc *gc, uint32_t domid,
> #define LIBXL__DEFBOOL_STR_DEFAULT "<default>"
> #define LIBXL__DEFBOOL_STR_FALSE   "False"
> #define LIBXL__DEFBOOL_STR_TRUE    "True"
> -static inline int libxl__defbool_is_default(libxl_defbool *db)
> +static inline int libxl__defbool_is_default(const libxl_defbool *db)
> {
>     return !db->val;
> }
> @@ -4675,22 +4675,22 @@ int libxl__random_bytes(libxl__gc *gc, uint8_t *buf, size_t len);
> #include "_libxl_types_internal_private.h"
> 
> /* This always return false, there's no "default value" for hw cap */
> -static inline int libxl__hwcap_is_default(libxl_hwcap *hwcap)
> +static inline int libxl__hwcap_is_default(const libxl_hwcap *hwcap)
> {
>     return 0;
> }
> 
> -static inline int libxl__string_list_is_empty(libxl_string_list *psl)
> +static inline int libxl__string_list_is_empty(const libxl_string_list *psl)
> {
>     return !libxl_string_list_length(psl);
> }
> 
> -static inline int libxl__key_value_list_is_empty(libxl_key_value_list *pkvl)
> +static inline int libxl__key_value_list_is_empty(const libxl_key_value_list *pkvl)
> {
>     return !libxl_key_value_list_length(pkvl);
> }
> 
> -int libxl__cpuid_policy_is_empty(libxl_cpuid_policy_list *pl);
> +int libxl__cpuid_policy_is_empty(const libxl_cpuid_policy_list *pl);
> 
> /* Portability note: a proper flock(2) implementation is required */
> typedef struct {
> @@ -4821,12 +4821,12 @@ void* libxl__device_list(libxl__gc *gc, const libxl__device_type *dt,
> void libxl__device_list_free(const libxl__device_type *dt,
>                              void *list, int num);
> 
> -static inline bool libxl__timer_mode_is_default(libxl_timer_mode *tm)
> +static inline bool libxl__timer_mode_is_default(const libxl_timer_mode *tm)
> {
>     return *tm == LIBXL_TIMER_MODE_DEFAULT;
> }
> 
> -static inline bool libxl__string_is_default(char **s)
> +static inline bool libxl__string_is_default(char *const *s)
> {
>     return *s == NULL;
> }
> diff --git a/tools/libs/light/libxl_json.c b/tools/libs/light/libxl_json.c
> index 9b8ef2cab9..88e81f9905 100644
> --- a/tools/libs/light/libxl_json.c
> +++ b/tools/libs/light/libxl_json.c
> @@ -95,7 +95,7 @@ yajl_gen_status libxl__yajl_gen_enum(yajl_gen hand, const char *str)
>  * YAJL generators for builtin libxl types.
>  */
> yajl_gen_status libxl_defbool_gen_json(yajl_gen hand,
> -                                       libxl_defbool *db)
> +                                       const libxl_defbool *db)
> {
>     return libxl__yajl_gen_asciiz(hand, libxl_defbool_to_string(*db));
> }
> @@ -137,7 +137,7 @@ int libxl__bool_parse_json(libxl__gc *gc, const libxl__json_object *o,
> }
> 
> yajl_gen_status libxl_uuid_gen_json(yajl_gen hand,
> -                                    libxl_uuid *uuid)
> +                                    const libxl_uuid *uuid)
> {
>     char buf[LIBXL_UUID_FMTLEN+1];
>     snprintf(buf, sizeof(buf), LIBXL_UUID_FMT, LIBXL_UUID_BYTES((*uuid)));
> @@ -154,7 +154,7 @@ int libxl__uuid_parse_json(libxl__gc *gc, const libxl__json_object *o,
> }
> 
> yajl_gen_status libxl_bitmap_gen_json(yajl_gen hand,
> -                                      libxl_bitmap *bitmap)
> +                                      const libxl_bitmap *bitmap)
> {
>     yajl_gen_status s;
>     int i;
> @@ -208,7 +208,7 @@ int libxl__bitmap_parse_json(libxl__gc *gc, const libxl__json_object *o,
> }
> 
> yajl_gen_status libxl_key_value_list_gen_json(yajl_gen hand,
> -                                              libxl_key_value_list *pkvl)
> +                                              const libxl_key_value_list *pkvl)
> {
>     libxl_key_value_list kvl = *pkvl;
>     yajl_gen_status s;
> @@ -269,7 +269,8 @@ int libxl__key_value_list_parse_json(libxl__gc *gc, const libxl__json_object *o,
>     return 0;
> }
> 
> -yajl_gen_status libxl_string_list_gen_json(yajl_gen hand, libxl_string_list *pl)
> +yajl_gen_status libxl_string_list_gen_json(yajl_gen hand,
> +                                           const libxl_string_list *pl)
> {
>     libxl_string_list l = *pl;
>     yajl_gen_status s;
> @@ -322,7 +323,7 @@ int libxl__string_list_parse_json(libxl__gc *gc, const libxl__json_object *o,
>     return 0;
> }
> 
> -yajl_gen_status libxl_mac_gen_json(yajl_gen hand, libxl_mac *mac)
> +yajl_gen_status libxl_mac_gen_json(yajl_gen hand, const libxl_mac *mac)
> {
>     char buf[LIBXL_MAC_FMTLEN+1];
>     snprintf(buf, sizeof(buf), LIBXL_MAC_FMT, LIBXL_MAC_BYTES((*mac)));
> @@ -339,7 +340,7 @@ int libxl__mac_parse_json(libxl__gc *gc, const libxl__json_object *o,
> }
> 
> yajl_gen_status libxl_hwcap_gen_json(yajl_gen hand,
> -                                     libxl_hwcap *p)
> +                                     const libxl_hwcap *p)
> {
>     yajl_gen_status s;
>     int i;
> @@ -377,7 +378,8 @@ int libxl__hwcap_parse_json(libxl__gc *gc, const libxl__json_object *o,
>     return 0;
> }
> 
> -yajl_gen_status libxl_ms_vm_genid_gen_json(yajl_gen hand, libxl_ms_vm_genid *p)
> +yajl_gen_status libxl_ms_vm_genid_gen_json(yajl_gen hand,
> +                                           const libxl_ms_vm_genid *p)
> {
>     yajl_gen_status s;
>     int i;
> diff --git a/tools/libs/light/libxl_nocpuid.c b/tools/libs/light/libxl_nocpuid.c
> index 0630959e76..f40a004e95 100644
> --- a/tools/libs/light/libxl_nocpuid.c
> +++ b/tools/libs/light/libxl_nocpuid.c
> @@ -14,7 +14,7 @@
> 
> #include "libxl_internal.h"
> 
> -int libxl__cpuid_policy_is_empty(libxl_cpuid_policy_list *pl)
> +int libxl__cpuid_policy_is_empty(const libxl_cpuid_policy_list *pl)
> {
>     return 1;
> }
> @@ -41,7 +41,7 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
> }
> 
> yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
> -                                libxl_cpuid_policy_list *pcpuid)
> +                                const libxl_cpuid_policy_list *pcpuid)
> {
>     return 0;
> }
> -- 
> 2.30.2
> 
> 

Re: [PATCH 2/5] tools/xl: Mark libxl_domain_config * arg of printf_info_*() const

[Luca Fancellu]

> On 18 Dec 2020, at 21:32, Elliott Mitchell <ehem+xen@m5p.com> wrote:
> 
> With libxl having gotten a lot more constant, now printf_info_sexp() and
> printf_info_one_json() can add consts.  May not be particularly
> important, but it is best to mark things constant when they are known to
> be so.

Looks ok to me
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> 
> Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
> ---
> tools/xl/xl.h      | 2 +-
> tools/xl/xl_info.c | 2 +-
> tools/xl/xl_sxp.c  | 6 +++---
> 3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/xl/xl.h b/tools/xl/xl.h
> index c5c4bedbdd..720adb0048 100644
> --- a/tools/xl/xl.h
> +++ b/tools/xl/xl.h
> @@ -300,7 +300,7 @@ typedef enum {
>     DOMAIN_RESTART_SOFT_RESET,   /* Soft reset should be performed */
> } domain_restart_type;
> 
> -extern void printf_info_sexp(int domid, libxl_domain_config *d_config, FILE *fh);
> +extern void printf_info_sexp(int domid, const libxl_domain_config *d_config, FILE *fh);
> extern void apply_global_affinity_masks(libxl_domain_type type,
>                                         libxl_bitmap *vcpu_affinity_array,
>                                         unsigned int size);
> diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
> index 712b7638b0..23d82ce2a2 100644
> --- a/tools/xl/xl_info.c
> +++ b/tools/xl/xl_info.c
> @@ -59,7 +59,7 @@ static int maybe_printf(const char *fmt, ...)
> }
> 
> static yajl_gen_status printf_info_one_json(yajl_gen hand, int domid,
> -                                            libxl_domain_config *d_config)
> +                                            const libxl_domain_config *d_config)
> {
>     yajl_gen_status s;
> 
> diff --git a/tools/xl/xl_sxp.c b/tools/xl/xl_sxp.c
> index 359a001570..d5b9051dfc 100644
> --- a/tools/xl/xl_sxp.c
> +++ b/tools/xl/xl_sxp.c
> @@ -26,13 +26,13 @@
> /* In general you should not add new output to this function since it
>  * is intended only for legacy use.
>  */
> -void printf_info_sexp(int domid, libxl_domain_config *d_config, FILE *fh)
> +void printf_info_sexp(int domid, const libxl_domain_config *d_config, FILE *fh)
> {
>     int i;
>     libxl_dominfo info;
> 
> -    libxl_domain_create_info *c_info = &d_config->c_info;
> -    libxl_domain_build_info *b_info = &d_config->b_info;
> +    const libxl_domain_create_info *c_info = &d_config->c_info;
> +    const libxl_domain_build_info *b_info = &d_config->b_info;
> 
>     fprintf(fh, "(domain\n\t(domid %d)\n", domid);
>     fprintf(fh, "\t(create_info)\n");
> -- 
> 2.30.2
> 
> 

Re: [PATCH 3/5] tools/xl: Rename printf_info()/list_domains_details() to dump_by_...()

[Luca Fancellu]

> On 18 Dec 2020, at 01:42, Elliott Mitchell <ehem+xen@m5p.com> wrote:
> 
> printf_info()/list_domains_details() had been serving fairly similar
> purposes.  Increase their consistency (add file-handle and output_format
> arguments to list_domains_details(), reorder arguments) and then rename
> to better reflect their functionality.
> 
> Both were simply outputting full domain information.  As this is more of
> a dump operation, "dump" is a better name.
> 

Looks ok to me
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

> Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
> ---
> tools/xl/xl.h      |  8 ++++++++
> tools/xl/xl_info.c | 30 ++++++++++++++++--------------
> tools/xl/xl_misc.c |  5 +----
> 3 files changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/xl/xl.h b/tools/xl/xl.h
> index 720adb0048..be5f4e11fe 100644
> --- a/tools/xl/xl.h
> +++ b/tools/xl/xl.h
> @@ -300,6 +300,14 @@ typedef enum {
>     DOMAIN_RESTART_SOFT_RESET,   /* Soft reset should be performed */
> } domain_restart_type;
> 
> +extern void dump_by_config(enum output_format output_format,
> +                           FILE *fh,
> +                           const libxl_domain_config *d_config,
> +                           int domid);
> +extern void dump_by_dominfo_list(enum output_format output_format,
> +                                 FILE *fh,
> +                                 const libxl_dominfo info[],
> +                                 int nb_domain);
> extern void printf_info_sexp(int domid, const libxl_domain_config *d_config, FILE *fh);
> extern void apply_global_affinity_masks(libxl_domain_type type,
>                                         libxl_bitmap *vcpu_affinity_array,
> diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
> index 23d82ce2a2..3647468420 100644
> --- a/tools/xl/xl_info.c
> +++ b/tools/xl/xl_info.c
> @@ -94,12 +94,10 @@ out:
>     return s;
> }
> 
> -void printf_info(enum output_format output_format,
> -                 int domid,
> -                 libxl_domain_config *d_config, FILE *fh);
> -void printf_info(enum output_format output_format,
> -                 int domid,
> -                 libxl_domain_config *d_config, FILE *fh)
> +void dump_by_config(enum output_format output_format,
> +                    FILE *fh,
> +                    const libxl_domain_config *const d_config,
> +                    int domid)
> {
>     if (output_format == OUTPUT_FORMAT_SXP)
>         return printf_info_sexp(domid, d_config, fh);
> @@ -442,7 +440,10 @@ static void list_domains(bool verbose, bool context, bool claim, bool numa,
>     libxl_physinfo_dispose(&physinfo);
> }
> 
> -static void list_domains_details(const libxl_dominfo *info, int nb_domain)
> +void dump_by_dominfo_list(enum output_format output_format,
> +                          FILE *fh,
> +                          const libxl_dominfo info[],
> +                          int nb_domain)
> {
>     libxl_domain_config d_config;
> 
> @@ -453,7 +454,7 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
>     const char *buf;
>     libxl_yajl_length yajl_len = 0;
> 
> -    if (default_output_format == OUTPUT_FORMAT_JSON) {
> +    if (output_format == OUTPUT_FORMAT_JSON) {
>         hand = libxl_yajl_gen_alloc(NULL);
>         if (!hand) {
>             fprintf(stderr, "unable to allocate JSON generator\n");
> @@ -472,16 +473,16 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
>                                                  &d_config, NULL);
>         if (rc)
>             continue;
> -        if (default_output_format == OUTPUT_FORMAT_JSON)
> +        if (output_format == OUTPUT_FORMAT_JSON)
>             s = printf_info_one_json(hand, info[i].domid, &d_config);
>         else
> -            printf_info_sexp(info[i].domid, &d_config, stdout);
> +            printf_info_sexp(info[i].domid, &d_config, fh);
>         libxl_domain_config_dispose(&d_config);
>         if (s != yajl_gen_status_ok)
>             goto out;
>     }
> 
> -    if (default_output_format == OUTPUT_FORMAT_JSON) {
> +    if (output_format == OUTPUT_FORMAT_JSON) {
>         s = yajl_gen_array_close(hand);
>         if (s != yajl_gen_status_ok)
>             goto out;
> @@ -490,11 +491,12 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
>         if (s != yajl_gen_status_ok)
>             goto out;
> 
> -        puts(buf);
> +        fputs(buf, fh);
> +        fputc('\n', fh);
>     }
> 
> out:
> -    if (default_output_format == OUTPUT_FORMAT_JSON) {
> +    if (output_format == OUTPUT_FORMAT_JSON) {
>         yajl_gen_free(hand);
>         if (s != yajl_gen_status_ok)
>             fprintf(stderr,
> @@ -571,7 +573,7 @@ int main_list(int argc, char **argv)
>     }
> 
>     if (details)
> -        list_domains_details(info, nb_domain);
> +        dump_by_dominfo_list(default_output_format, stdout, info, nb_domain);
>     else
>         list_domains(verbose, context, false /* claim */, numa, cpupool,
>                      info, nb_domain);
> diff --git a/tools/xl/xl_misc.c b/tools/xl/xl_misc.c
> index 08f0fb6dc9..bcf178762b 100644
> --- a/tools/xl/xl_misc.c
> +++ b/tools/xl/xl_misc.c
> @@ -256,9 +256,6 @@ int main_dump_core(int argc, char **argv)
>     return EXIT_SUCCESS;
> }
> 
> -extern void printf_info(enum output_format output_format,
> -                        int domid,
> -                        libxl_domain_config *d_config, FILE *fh);
> int main_config_update(int argc, char **argv)
> {
>     uint32_t domid;
> @@ -344,7 +341,7 @@ int main_config_update(int argc, char **argv)
>     parse_config_data(filename, config_data, config_len, &d_config);
> 
>     if (debug || dryrun_only)
> -        printf_info(default_output_format, -1, &d_config, stdout);
> +        dump_by_config(default_output_format, stdout, &d_config, -1);
> 
>     if (!dryrun_only) {
>         fprintf(stderr, "setting dom%u configuration\n", domid);
> -- 
> 2.30.2
> 
> 

Re: [PATCH 4/5] tools/xl: Merge down debug/dry-run section of create_domain()

[Luca Fancellu]

> On 18 Dec 2020, at 01:42, Elliott Mitchell <ehem+xen@m5p.com> wrote:
> 
> create_domain()'s use of printf_info_sexp() could be merged down to a
> single dump_by_config(), do so.  This results in an extra JSON dictionary
> in output, but I doubt that is an issue for dry-run or debugging output.
> 

Don’t know if the extra output is a problem, but for me looks ok
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

> Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
> ---
> tools/xl/xl_vmcontrol.c | 14 +-------------
> 1 file changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
> index 435155a033..4b95e7e463 100644
> --- a/tools/xl/xl_vmcontrol.c
> +++ b/tools/xl/xl_vmcontrol.c
> @@ -856,19 +856,7 @@ int create_domain(struct domain_create *dom_info)
> 
>     if (debug || dom_info->dryrun) {
>         FILE *cfg_print_fh = (debug && !dom_info->dryrun) ? stderr : stdout;
> -        if (default_output_format == OUTPUT_FORMAT_SXP) {
> -            printf_info_sexp(-1, &d_config, cfg_print_fh);
> -        } else {
> -            char *json = libxl_domain_config_to_json(ctx, &d_config);
> -            if (!json) {
> -                fprintf(stderr,
> -                        "Failed to convert domain configuration to JSON\n");
> -                exit(1);
> -            }
> -            fputs(json, cfg_print_fh);
> -            free(json);
> -            flush_stream(cfg_print_fh);
> -        }
> +        dump_by_config(default_output_format, cfg_print_fh, &d_config, -1);
>     }
> 
> 
> -- 
> 2.30.2
> 
> 

Re: [PATCH 5/5] tools/xl: Fix potential deallocation bug

[Luca Fancellu]

> On 10 Dec 2020, at 23:09, Elliott Mitchell <ehem+xen@m5p.com> wrote:
> 
> There is potential for the info and info_free variable's purposes to
> diverge.  If info was overwritten with a distinct value, yet info_free
> still needed deallocation a bug would occur on this line.  Preemptively
> address this issue (making use of divergent info/info_free values is
> under consideration).
> 

Looks ok to me
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

> Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
> ---
> tools/xl/xl_info.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
> index 3647468420..938f06f1a8 100644
> --- a/tools/xl/xl_info.c
> +++ b/tools/xl/xl_info.c
> @@ -579,7 +579,7 @@ int main_list(int argc, char **argv)
>                      info, nb_domain);
> 
>     if (info_free)
> -        libxl_dominfo_list_free(info, nb_domain);
> +        libxl_dominfo_list_free(info_free, nb_domain);
> 
>     libxl_dominfo_dispose(&info_buf);
> 
> -- 
> 2.30.2
> 
> 

Re: [PATCH 1/5] tools/libxl: Mark pointer args of many functions constant

[Anthony PERARD]

On Fri, Dec 18, 2020 at 01:37:44PM -0800, Elliott Mitchell wrote:
> Anything *_is_empty(), *_is_default(), or *_gen_json() is going to be
> examining the pointed to thing, not modifying it.  This potentially
> results in higher-performance output.  This also allows spreading
> constants further, allowing more checking and security.
> 
> Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>

This patch doesn't build.

    libxl_cpuid.c:510:17: error: conflicting types for ‘libxl_cpuid_policy_list_gen_json’
      510 | yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
          |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    In file included from libxl_internal.h:89,
                     from libxl_cpuid.c:15:
    /home/sheep/work/xen/tools/libs/light/../../../tools/include/libxl_json.h:30:17: note: previous declaration of ‘libxl_cpuid_policy_list_gen_json’ was here
       30 | yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
          |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Also we talked about this patch before, in
    https://lore.kernel.org/xen-devel/YImXfc4oaPgWzkeT@perard/

Cheers,

-- 
Anthony PERARD

Re: [PATCH 1/5] tools/libxl: Mark pointer args of many functions constant

[Luca Fancellu]

> On 5 Jan 2022, at 10:09, Anthony PERARD <anthony.perard@citrix.com> wrote:
> 
> On Fri, Dec 18, 2020 at 01:37:44PM -0800, Elliott Mitchell wrote:
>> Anything *_is_empty(), *_is_default(), or *_gen_json() is going to be
>> examining the pointed to thing, not modifying it.  This potentially
>> results in higher-performance output.  This also allows spreading
>> constants further, allowing more checking and security.
>> 
>> Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
> 
> This patch doesn't build.
> 
>    libxl_cpuid.c:510:17: error: conflicting types for ‘libxl_cpuid_policy_list_gen_json’
>      510 | yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
>          |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    In file included from libxl_internal.h:89,
>                     from libxl_cpuid.c:15:
>    /home/sheep/work/xen/tools/libs/light/../../../tools/include/libxl_json.h:30:17: note: previous declaration of ‘libxl_cpuid_policy_list_gen_json’ was here
>       30 | yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
>          |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Also we talked about this patch before, in
>    https://lore.kernel.org/xen-devel/YImXfc4oaPgWzkeT@perard/
> 
> Cheers,

Sorry about that, when I’ve put my R-by I thought it was building, I’ve checked again and it’s not building for x86_64, arm32 and arm64 looks fine so far.

Cheers,
Luca

> 
> -- 
> Anthony PERARD
> 

Re: [PATCH 5/5] tools/xl: Fix potential deallocation bug

[Anthony PERARD]

On Thu, Dec 10, 2020 at 03:09:06PM -0800, Elliott Mitchell wrote:
> There is potential for the info and info_free variable's purposes to
> diverge.  If info was overwritten with a distinct value, yet info_free
> still needed deallocation a bug would occur on this line.  Preemptively
> address this issue (making use of divergent info/info_free values is
> under consideration).
> 
> Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
> ---
>  tools/xl/xl_info.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
> index 3647468420..938f06f1a8 100644
> --- a/tools/xl/xl_info.c
> +++ b/tools/xl/xl_info.c
> @@ -579,7 +579,7 @@ int main_list(int argc, char **argv)
>                       info, nb_domain);
>  
>      if (info_free)
> -        libxl_dominfo_list_free(info, nb_domain);
> +        libxl_dominfo_list_free(info_free, nb_domain);
>  
>      libxl_dominfo_dispose(&info_buf);
>  

I don't think this is the right thing to do with this patch.
libxl_dominfo_list_free() should use the same variable that is used by
libxl_list_domain(). What we want to free is the allocation made by
libxl_list_domain().

"info_free" in the function seems to be used as a boolean which tell
if "info" have been allocated or not. Actually, it probably say if
"info" is a list of "libxl_dominfo" or not.

So instead of just replacing "info" by "info_free" here, we should
instead store the result from libxl_list_domain() into a different
variable and free that, like it is done with "info_buf".

I hope that makes sense?

Thanks,

-- 
Anthony PERARD

Re: [PATCH 5/5] tools/xl: Fix potential deallocation bug

[Elliott Mitchell]

Huh, never got around to replying to this.  Too many things going on, too
many distractions...

On Wed, Jan 05, 2022 at 03:05:23PM +0000, Anthony PERARD wrote:
> On Thu, Dec 10, 2020 at 03:09:06PM -0800, Elliott Mitchell wrote:
> > There is potential for the info and info_free variable's purposes to
> > diverge.  If info was overwritten with a distinct value, yet info_free
> > still needed deallocation a bug would occur on this line.  Preemptively
> > address this issue (making use of divergent info/info_free values is
> > under consideration).
> > 
> > Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
> > ---
> >  tools/xl/xl_info.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
> > index 3647468420..938f06f1a8 100644
> > --- a/tools/xl/xl_info.c
> > +++ b/tools/xl/xl_info.c
> > @@ -579,7 +579,7 @@ int main_list(int argc, char **argv)
> >                       info, nb_domain);
> >  
> >      if (info_free)
> > -        libxl_dominfo_list_free(info, nb_domain);
> > +        libxl_dominfo_list_free(info_free, nb_domain);
> >  
> >      libxl_dominfo_dispose(&info_buf);
> >  
> 
> I don't think this is the right thing to do with this patch.

I disagree with this statement.

> libxl_dominfo_list_free() should use the same variable that is used by
> libxl_list_domain(). What we want to free is the allocation made by
> libxl_list_domain().

I agree with this statement.

> "info_free" in the function seems to be used as a boolean which tell
> if "info" have been allocated or not. Actually, it probably say if
> "info" is a list of "libxl_dominfo" or not.

That may be what the author was thinking when they wrote lines 579 & 580.
Problem is info_free is a pointer to libxl_dominfo, *not* a boolean.

> So instead of just replacing "info" by "info_free" here, we should
> instead store the result from libxl_list_domain() into a different
> variable and free that, like it is done with "info_buf".
> 
> I hope that makes sense?

What you're describing seems to be precisely what the patch does.
Perhaps you got the roles of "info" and "info_free" reversed?

This actually points to an issue on lines 548 & 553.  Instead of storing
the return from libxl_list_domain() into "info" then copying to
"info_free" both should be set at the same time.

I had noticed this (and cringed), but didn't feel it was currently
worthwhile to go after lines 548 & 553.  If you want this additional
change to accept the patch, I'm up for that.


-- 
(\___(\___(\______          --=> 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