* [PATCH 0/5] Some misc from my tree @ 2021-12-27 7:39 Elliott Mitchell 2020-12-10 23:09 ` [PATCH 5/5] tools/xl: Fix potential deallocation bug Elliott Mitchell ` (4 more replies) 0 siblings, 5 replies; 15+ messages in thread From: Elliott Mitchell @ 2021-12-27 7:39 UTC (permalink / raw) To: xen-devel; +Cc: Wei Liu, Anthony PERARD, Juergen Gross 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 5/5] tools/xl: Fix potential deallocation bug 2021-12-27 7:39 [PATCH 0/5] Some misc from my tree Elliott Mitchell @ 2020-12-10 23:09 ` Elliott Mitchell 2021-12-29 17:20 ` Luca Fancellu 2022-01-05 15:05 ` Anthony PERARD 2020-12-18 1:42 ` [PATCH 4/5] tools/xl: Merge down debug/dry-run section of create_domain() Elliott Mitchell ` (3 subsequent siblings) 4 siblings, 2 replies; 15+ messages in thread From: Elliott Mitchell @ 2020-12-10 23:09 UTC (permalink / raw) To: xen-devel; +Cc: Wei Liu, Anthony PERARD 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 5/5] tools/xl: Fix potential deallocation bug 2020-12-10 23:09 ` [PATCH 5/5] tools/xl: Fix potential deallocation bug Elliott Mitchell @ 2021-12-29 17:20 ` Luca Fancellu 2022-01-05 15:05 ` Anthony PERARD 1 sibling, 0 replies; 15+ messages in thread From: Luca Fancellu @ 2021-12-29 17:20 UTC (permalink / raw) To: Elliott Mitchell; +Cc: Xen-devel, Wei Liu, Anthony PERARD > 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 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 5/5] tools/xl: Fix potential deallocation bug 2020-12-10 23:09 ` [PATCH 5/5] tools/xl: Fix potential deallocation bug Elliott Mitchell 2021-12-29 17:20 ` Luca Fancellu @ 2022-01-05 15:05 ` Anthony PERARD 2022-04-22 22:58 ` Elliott Mitchell 1 sibling, 1 reply; 15+ messages in thread From: Anthony PERARD @ 2022-01-05 15:05 UTC (permalink / raw) To: Elliott Mitchell; +Cc: xen-devel, Wei Liu 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 5/5] tools/xl: Fix potential deallocation bug 2022-01-05 15:05 ` Anthony PERARD @ 2022-04-22 22:58 ` Elliott Mitchell 0 siblings, 0 replies; 15+ messages in thread From: Elliott Mitchell @ 2022-04-22 22:58 UTC (permalink / raw) To: Anthony PERARD; +Cc: xen-devel, Wei Liu 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/5] tools/xl: Merge down debug/dry-run section of create_domain() 2021-12-27 7:39 [PATCH 0/5] Some misc from my tree Elliott Mitchell 2020-12-10 23:09 ` [PATCH 5/5] tools/xl: Fix potential deallocation bug Elliott Mitchell @ 2020-12-18 1:42 ` Elliott Mitchell 2021-12-29 17:19 ` Luca Fancellu 2020-12-18 1:42 ` [PATCH 3/5] tools/xl: Rename printf_info()/list_domains_details() to dump_by_...() Elliott Mitchell ` (2 subsequent siblings) 4 siblings, 1 reply; 15+ messages in thread From: Elliott Mitchell @ 2020-12-18 1:42 UTC (permalink / raw) To: xen-devel; +Cc: Wei Liu, Anthony PERARD 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 4/5] tools/xl: Merge down debug/dry-run section of create_domain() 2020-12-18 1:42 ` [PATCH 4/5] tools/xl: Merge down debug/dry-run section of create_domain() Elliott Mitchell @ 2021-12-29 17:19 ` Luca Fancellu 0 siblings, 0 replies; 15+ messages in thread From: Luca Fancellu @ 2021-12-29 17:19 UTC (permalink / raw) To: Elliott Mitchell; +Cc: Xen-devel, Wei Liu, Anthony PERARD > 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 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/5] tools/xl: Rename printf_info()/list_domains_details() to dump_by_...() 2021-12-27 7:39 [PATCH 0/5] Some misc from my tree Elliott Mitchell 2020-12-10 23:09 ` [PATCH 5/5] tools/xl: Fix potential deallocation bug Elliott Mitchell 2020-12-18 1:42 ` [PATCH 4/5] tools/xl: Merge down debug/dry-run section of create_domain() Elliott Mitchell @ 2020-12-18 1:42 ` Elliott Mitchell 2021-12-29 17:18 ` Luca Fancellu 2020-12-18 21:32 ` [PATCH 2/5] tools/xl: Mark libxl_domain_config * arg of printf_info_*() const Elliott Mitchell 2020-12-18 21:37 ` [PATCH 1/5] tools/libxl: Mark pointer args of many functions constant Elliott Mitchell 4 siblings, 1 reply; 15+ messages in thread From: Elliott Mitchell @ 2020-12-18 1:42 UTC (permalink / raw) To: xen-devel; +Cc: Wei Liu, Anthony PERARD 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 3/5] tools/xl: Rename printf_info()/list_domains_details() to dump_by_...() 2020-12-18 1:42 ` [PATCH 3/5] tools/xl: Rename printf_info()/list_domains_details() to dump_by_...() Elliott Mitchell @ 2021-12-29 17:18 ` Luca Fancellu 0 siblings, 0 replies; 15+ messages in thread From: Luca Fancellu @ 2021-12-29 17:18 UTC (permalink / raw) To: Elliott Mitchell; +Cc: Xen-devel, Wei Liu, Anthony PERARD > 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 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/5] tools/xl: Mark libxl_domain_config * arg of printf_info_*() const 2021-12-27 7:39 [PATCH 0/5] Some misc from my tree Elliott Mitchell ` (2 preceding siblings ...) 2020-12-18 1:42 ` [PATCH 3/5] tools/xl: Rename printf_info()/list_domains_details() to dump_by_...() Elliott Mitchell @ 2020-12-18 21:32 ` Elliott Mitchell 2021-12-29 17:18 ` Luca Fancellu 2020-12-18 21:37 ` [PATCH 1/5] tools/libxl: Mark pointer args of many functions constant Elliott Mitchell 4 siblings, 1 reply; 15+ messages in thread From: Elliott Mitchell @ 2020-12-18 21:32 UTC (permalink / raw) To: xen-devel; +Cc: Wei Liu, Anthony PERARD 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/5] tools/xl: Mark libxl_domain_config * arg of printf_info_*() const 2020-12-18 21:32 ` [PATCH 2/5] tools/xl: Mark libxl_domain_config * arg of printf_info_*() const Elliott Mitchell @ 2021-12-29 17:18 ` Luca Fancellu 0 siblings, 0 replies; 15+ messages in thread From: Luca Fancellu @ 2021-12-29 17:18 UTC (permalink / raw) To: Elliott Mitchell; +Cc: Xen-devel, Wei Liu, Anthony PERARD > 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 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/5] tools/libxl: Mark pointer args of many functions constant 2021-12-27 7:39 [PATCH 0/5] Some misc from my tree Elliott Mitchell ` (3 preceding siblings ...) 2020-12-18 21:32 ` [PATCH 2/5] tools/xl: Mark libxl_domain_config * arg of printf_info_*() const Elliott Mitchell @ 2020-12-18 21:37 ` Elliott Mitchell 2021-12-29 17:15 ` Luca Fancellu 2022-01-05 10:09 ` Anthony PERARD 4 siblings, 2 replies; 15+ messages in thread From: Elliott Mitchell @ 2020-12-18 21:37 UTC (permalink / raw) To: xen-devel; +Cc: Wei Liu, Anthony PERARD, Juergen Gross 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/5] tools/libxl: Mark pointer args of many functions constant 2020-12-18 21:37 ` [PATCH 1/5] tools/libxl: Mark pointer args of many functions constant Elliott Mitchell @ 2021-12-29 17:15 ` Luca Fancellu 2022-01-05 10:09 ` Anthony PERARD 1 sibling, 0 replies; 15+ messages in thread From: Luca Fancellu @ 2021-12-29 17:15 UTC (permalink / raw) To: Elliott Mitchell; +Cc: Xen-devel, Wei Liu, Anthony PERARD, Juergen Gross > 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 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/5] tools/libxl: Mark pointer args of many functions constant 2020-12-18 21:37 ` [PATCH 1/5] tools/libxl: Mark pointer args of many functions constant Elliott Mitchell 2021-12-29 17:15 ` Luca Fancellu @ 2022-01-05 10:09 ` Anthony PERARD 2022-01-05 13:09 ` Luca Fancellu 1 sibling, 1 reply; 15+ messages in thread From: Anthony PERARD @ 2022-01-05 10:09 UTC (permalink / raw) To: Elliott Mitchell; +Cc: xen-devel, Wei Liu, Juergen Gross 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/5] tools/libxl: Mark pointer args of many functions constant 2022-01-05 10:09 ` Anthony PERARD @ 2022-01-05 13:09 ` Luca Fancellu 0 siblings, 0 replies; 15+ messages in thread From: Luca Fancellu @ 2022-01-05 13:09 UTC (permalink / raw) To: Anthony PERARD; +Cc: Elliott Mitchell, xen-devel, Wei Liu, Juergen Gross > 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-04-22 22:59 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-27 7:39 [PATCH 0/5] Some misc from my tree Elliott Mitchell 2020-12-10 23:09 ` [PATCH 5/5] tools/xl: Fix potential deallocation bug Elliott Mitchell 2021-12-29 17:20 ` Luca Fancellu 2022-01-05 15:05 ` Anthony PERARD 2022-04-22 22:58 ` Elliott Mitchell 2020-12-18 1:42 ` [PATCH 4/5] tools/xl: Merge down debug/dry-run section of create_domain() Elliott Mitchell 2021-12-29 17:19 ` Luca Fancellu 2020-12-18 1:42 ` [PATCH 3/5] tools/xl: Rename printf_info()/list_domains_details() to dump_by_...() Elliott Mitchell 2021-12-29 17:18 ` Luca Fancellu 2020-12-18 21:32 ` [PATCH 2/5] tools/xl: Mark libxl_domain_config * arg of printf_info_*() const Elliott Mitchell 2021-12-29 17:18 ` Luca Fancellu 2020-12-18 21:37 ` [PATCH 1/5] tools/libxl: Mark pointer args of many functions constant Elliott Mitchell 2021-12-29 17:15 ` Luca Fancellu 2022-01-05 10:09 ` Anthony PERARD 2022-01-05 13:09 ` Luca Fancellu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.