| Message ID | 20231003183058.1639121-10-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | TCG code quality tracking | expand |
Richard Henderson <richard.henderson@linaro.org> writes: > Do not rely on return value of 0 to indicate error, > pass along an Error pointer to be set. Not wrong, but goes against error.h's recommendation * - Whenever practical, also return a value that indicates success / * failure. This can make the error checking more concise, and can * avoid useless error object creation and destruction. Note that * we still have many functions returning void. We recommend * • bool-valued functions return true on success / false on failure, * • pointer-valued functions return non-null / null pointer, and * • integer-valued functions return non-negative / negative. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > include/qemu/log.h | 2 +- > bsd-user/main.c | 6 ++++-- > linux-user/main.c | 7 +++++-- > monitor/hmp-cmds.c | 5 +++-- > softmmu/vl.c | 7 +++++-- > util/log.c | 5 +++-- > 6 files changed, 21 insertions(+), 11 deletions(-) > > diff --git a/include/qemu/log.h b/include/qemu/log.h > index df59bfabcd..9b92d2663e 100644 > --- a/include/qemu/log.h > +++ b/include/qemu/log.h > @@ -87,7 +87,7 @@ bool qemu_set_log_filename(const char *filename, Error **errp); > bool qemu_set_log_filename_flags(const char *name, int flags, Error **errp); > void qemu_set_dfilter_ranges(const char *ranges, Error **errp); > bool qemu_log_in_addr_range(uint64_t addr); > -int qemu_str_to_log_mask(const char *str); > +int qemu_str_to_log_mask(const char *str, Error **errp); > > /* Print a usage message listing all the valid logging categories > * to the specified FILE*. > diff --git a/bsd-user/main.c b/bsd-user/main.c > index 703f3e2c41..a981239a0b 100644 > --- a/bsd-user/main.c > +++ b/bsd-user/main.c > @@ -411,8 +411,10 @@ int main(int argc, char **argv) > { > int mask = 0; > if (log_mask) { > - mask = qemu_str_to_log_mask(log_mask); > - if (!mask) { > + Error *err = NULL; > + mask = qemu_str_to_log_mask(log_mask, &err); > + if (err) { > + error_report_err(err); > qemu_print_log_usage(stdout); > exit(1); > } > diff --git a/linux-user/main.c b/linux-user/main.c > index 0c23584a96..d0464736cc 100644 > --- a/linux-user/main.c > +++ b/linux-user/main.c > @@ -264,8 +264,11 @@ static void handle_arg_help(const char *arg) > > static void handle_arg_log(const char *arg) > { > - last_log_mask = qemu_str_to_log_mask(arg); > - if (!last_log_mask) { > + Error *err = NULL; > + > + last_log_mask = qemu_str_to_log_mask(arg, &err); > + if (err) { > + error_report_err(err); > qemu_print_log_usage(stdout); > exit(EXIT_FAILURE); > } > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > index 6c559b48c8..c4bd97d467 100644 > --- a/monitor/hmp-cmds.c > +++ b/monitor/hmp-cmds.c > @@ -291,8 +291,9 @@ void hmp_log(Monitor *mon, const QDict *qdict) > if (!strcmp(items, "none")) { > mask = 0; > } else { > - mask = qemu_str_to_log_mask(items); > - if (!mask) { > + mask = qemu_str_to_log_mask(items, &err); > + if (err) { > + error_free(err); > hmp_help_cmd(mon, "log"); > return; > } > diff --git a/softmmu/vl.c b/softmmu/vl.c > index 98e071e63b..02193696b9 100644 > --- a/softmmu/vl.c > +++ b/softmmu/vl.c > @@ -2486,8 +2486,11 @@ static void qemu_process_early_options(void) > { > int mask = 0; > if (log_mask) { > - mask = qemu_str_to_log_mask(log_mask); > - if (!mask) { > + Error *err = NULL; > + > + mask = qemu_str_to_log_mask(log_mask, &err); > + if (err) { > + error_report_err(err); > qemu_print_log_usage(stdout); > exit(1); > } > diff --git a/util/log.c b/util/log.c > index def88a9402..b5f08db202 100644 > --- a/util/log.c > +++ b/util/log.c > @@ -500,8 +500,8 @@ const QEMULogItem qemu_log_items[] = { > { 0, NULL, NULL }, > }; > > -/* takes a comma separated list of log masks. Return 0 if error. */ > -int qemu_str_to_log_mask(const char *str) > +/* Takes a comma separated list of log masks. */ > +int qemu_str_to_log_mask(const char *str, Error **errp) > { > const QEMULogItem *item; > int mask = 0; > @@ -524,6 +524,7 @@ int qemu_str_to_log_mask(const char *str) char **parts = g_strsplit(str, ",", 0); char **tmp; When @parts is null or "", the loop runs zero times, and ... for (tmp = parts; tmp && *tmp; tmp++) { if (g_str_equal(*tmp, "all")) { for (item = qemu_log_items; item->mask != 0; item++) { mask |= item->mask; } #ifdef CONFIG_TRACE_LOG } else if (g_str_has_prefix(*tmp, "trace:") && (*tmp)[6] != '\0') { trace_enable_events((*tmp) + 6); mask |= LOG_TRACE; #endif } else { for (item = qemu_log_items; item->mask != 0; item++) { if (g_str_equal(*tmp, item->name)) { > goto found; > } > } > + error_setg(errp, "Invalid -d option \"%s\"", *tmp); > goto error; > found: > mask |= item->mask; } } ... we end up here with mask zero and no error set. g_strfreev(parts); return mask; Before the patch, this counted as error. Afterwards, it doesn't. Impact? Worth mentioning in the commit message? error: g_strfreev(parts); return 0; Returning -1 here would stick to error.h's recommendation. }
On 10/10/23 05:55, Markus Armbruster wrote: > Richard Henderson <richard.henderson@linaro.org> writes: > >> Do not rely on return value of 0 to indicate error, >> pass along an Error pointer to be set. > > Not wrong, but goes against error.h's recommendation > > * - Whenever practical, also return a value that indicates success / > * failure. This can make the error checking more concise, and can > * avoid useless error object creation and destruction. Note that > * we still have many functions returning void. We recommend > * • bool-valued functions return true on success / false on failure, > * • pointer-valued functions return non-null / null pointer, and > * • integer-valued functions return non-negative / negative. > ... > Returning -1 here would stick to error.h's recommendation. The problem had been that 0 would become a valid result in the next patch. But using -1 would work, particularly with bit 31 of the log mask as yet unused. r~
diff --git a/include/qemu/log.h b/include/qemu/log.h index df59bfabcd..9b92d2663e 100644 --- a/include/qemu/log.h +++ b/include/qemu/log.h @@ -87,7 +87,7 @@ bool qemu_set_log_filename(const char *filename, Error **errp); bool qemu_set_log_filename_flags(const char *name, int flags, Error **errp); void qemu_set_dfilter_ranges(const char *ranges, Error **errp); bool qemu_log_in_addr_range(uint64_t addr); -int qemu_str_to_log_mask(const char *str); +int qemu_str_to_log_mask(const char *str, Error **errp); /* Print a usage message listing all the valid logging categories * to the specified FILE*. diff --git a/bsd-user/main.c b/bsd-user/main.c index 703f3e2c41..a981239a0b 100644 --- a/bsd-user/main.c +++ b/bsd-user/main.c @@ -411,8 +411,10 @@ int main(int argc, char **argv) { int mask = 0; if (log_mask) { - mask = qemu_str_to_log_mask(log_mask); - if (!mask) { + Error *err = NULL; + mask = qemu_str_to_log_mask(log_mask, &err); + if (err) { + error_report_err(err); qemu_print_log_usage(stdout); exit(1); } diff --git a/linux-user/main.c b/linux-user/main.c index 0c23584a96..d0464736cc 100644 --- a/linux-user/main.c +++ b/linux-user/main.c @@ -264,8 +264,11 @@ static void handle_arg_help(const char *arg) static void handle_arg_log(const char *arg) { - last_log_mask = qemu_str_to_log_mask(arg); - if (!last_log_mask) { + Error *err = NULL; + + last_log_mask = qemu_str_to_log_mask(arg, &err); + if (err) { + error_report_err(err); qemu_print_log_usage(stdout); exit(EXIT_FAILURE); } diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 6c559b48c8..c4bd97d467 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -291,8 +291,9 @@ void hmp_log(Monitor *mon, const QDict *qdict) if (!strcmp(items, "none")) { mask = 0; } else { - mask = qemu_str_to_log_mask(items); - if (!mask) { + mask = qemu_str_to_log_mask(items, &err); + if (err) { + error_free(err); hmp_help_cmd(mon, "log"); return; } diff --git a/softmmu/vl.c b/softmmu/vl.c index 98e071e63b..02193696b9 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2486,8 +2486,11 @@ static void qemu_process_early_options(void) { int mask = 0; if (log_mask) { - mask = qemu_str_to_log_mask(log_mask); - if (!mask) { + Error *err = NULL; + + mask = qemu_str_to_log_mask(log_mask, &err); + if (err) { + error_report_err(err); qemu_print_log_usage(stdout); exit(1); } diff --git a/util/log.c b/util/log.c index def88a9402..b5f08db202 100644 --- a/util/log.c +++ b/util/log.c @@ -500,8 +500,8 @@ const QEMULogItem qemu_log_items[] = { { 0, NULL, NULL }, }; -/* takes a comma separated list of log masks. Return 0 if error. */ -int qemu_str_to_log_mask(const char *str) +/* Takes a comma separated list of log masks. */ +int qemu_str_to_log_mask(const char *str, Error **errp) { const QEMULogItem *item; int mask = 0; @@ -524,6 +524,7 @@ int qemu_str_to_log_mask(const char *str) goto found; } } + error_setg(errp, "Invalid -d option \"%s\"", *tmp); goto error; found: mask |= item->mask;
Do not rely on return value of 0 to indicate error, pass along an Error pointer to be set. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- include/qemu/log.h | 2 +- bsd-user/main.c | 6 ++++-- linux-user/main.c | 7 +++++-- monitor/hmp-cmds.c | 5 +++-- softmmu/vl.c | 7 +++++-- util/log.c | 5 +++-- 6 files changed, 21 insertions(+), 11 deletions(-)