Richard Henderson <richard.hender...@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.hender...@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.
}
