Hi Jean-Philippe,
On Tue, Jul 04, 2023 at 10:37:17AM +0100, Jean-Philippe Brucker wrote:
> On Fri, Jun 30, 2023 at 02:31:31PM +0100, Alexandru Elisei wrote:
> > Of all the pr_* functions, pr_err() is the only function that returns a
> > value, which is -1. The code in parse_options is the only code that relies
> > on pr_err() returning a value, and that value must be exactly -1, because
> > it is being treated differently than the other return values.
> >
> > This makes the code opaque, because it's not immediately obvious where that
> > value comes from, and fragile, as a change in the return value of pr_err
> > would break it.
> >
> > Make pr_err() more like the other functions and don't return a value.
> >
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
>
> One small nit below, otherwise
>
> Reviewed-by: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
Thank you for the review!
>
> > ---
> > include/kvm/util.h | 2 +-
> > util/parse-options.c | 28 ++++++++++++++++------------
> > util/util.c | 3 +--
> > 3 files changed, 18 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/kvm/util.h b/include/kvm/util.h
> > index b49454876a83..f51f370d2b37 100644
> > --- a/include/kvm/util.h
> > +++ b/include/kvm/util.h
> > @@ -39,7 +39,7 @@ extern bool do_debug_print;
> >
> > extern void die(const char *err, ...) NORETURN __attribute__((format (printf, 1, 2)));
> > extern void die_perror(const char *s) NORETURN;
> > -extern int pr_err(const char *err, ...) __attribute__((format (printf, 1, 2)));
> > +extern void pr_err(const char *err, ...) __attribute__((format (printf, 1, 2)));
> > extern void pr_warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
> > extern void pr_info(const char *err, ...) __attribute__((format (printf, 1, 2)));
> > extern void set_die_routine(void (*routine)(const char *err, va_list params) NORETURN);
> > diff --git a/util/parse-options.c b/util/parse-options.c
> > index 9a1bbee6c271..fb44b48d14c8 100644
> > --- a/util/parse-options.c
> > +++ b/util/parse-options.c
> > @@ -17,10 +17,13 @@
> > static int opterror(const struct option *opt, const char *reason, int flags)
> > {
> > if (flags & OPT_SHORT)
> > - return pr_err("switch `%c' %s", opt->short_name, reason);
> > - if (flags & OPT_UNSET)
> > - return pr_err("option `no-%s' %s", opt->long_name, reason);
> > - return pr_err("option `%s' %s", opt->long_name, reason);
> > + pr_err("switch `%c' %s", opt->short_name, reason);
> > + else if (flags & OPT_UNSET)
> > + pr_err("option `no-%s' %s", opt->long_name, reason);
> > + else
> > + pr_err("option `%s' %s", opt->long_name, reason);
> > +
> > + return -1;
> > }
> >
> > static int get_arg(struct parse_opt_ctx_t *p, const struct option *opt,
> > @@ -429,14 +432,15 @@ is_abbreviated:
> > return get_value(p, options, flags);
> > }
> >
> > - if (ambiguous_option)
> > - return pr_err("Ambiguous option: %s "
> > - "(could be --%s%s or --%s%s)",
> > - arg,
> > - (ambiguous_flags & OPT_UNSET) ? "no-" : "",
> > - ambiguous_option->long_name,
> > - (abbrev_flags & OPT_UNSET) ? "no-" : "",
> > - abbrev_option->long_name);
> > + if (ambiguous_option) {
> > + pr_err("Ambiguous option: %s " "(could be --%s%s or --%s%s)",
>
> drop " "
Done.
Thanks,
Alex
>
> > + arg,
> > + (ambiguous_flags & OPT_UNSET) ? "no-" : "",
> > + ambiguous_option->long_name,
> > + (abbrev_flags & OPT_UNSET) ? "no-" : "",
> > + abbrev_option->long_name);
> > + return -1;
> > + }
> > if (abbrev_option)
> > return get_value(p, abbrev_option, abbrev_flags);
> > return -2;
> > diff --git a/util/util.c b/util/util.c
> > index 1877105e3c08..f59f26e1581c 100644
> > --- a/util/util.c
> > +++ b/util/util.c
> > @@ -47,14 +47,13 @@ void die(const char *err, ...)
> > va_end(params);
> > }
> >
> > -int pr_err(const char *err, ...)
> > +void pr_err(const char *err, ...)
> > {
> > va_list params;
> >
> > va_start(params, err);
> > error_builtin(err, params);
> > va_end(params);
> > - return -1;
> > }
> >
> > void pr_warning(const char *warn, ...)
> > --
> > 2.41.0
> >
