diff mbox series

[RESEND,kvmtool,1/4] util: Make pr_err() return void

Message ID 20230630133134.65284-2-alexandru.elisei@arm.com (mailing list archive)
State New, archived
Headers show
Series Add --loglevel argument | expand

Commit Message

Alexandru Elisei June 30, 2023, 1:31 p.m. UTC 
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@arm.com>
---
 include/kvm/util.h   |  2 +-
 util/parse-options.c | 28 ++++++++++++++++------------
 util/util.c          |  3 +--
 3 files changed, 18 insertions(+), 15 deletions(-)

Comments

Jean-Philippe Brucker July 4, 2023, 9:37 a.m. UTC | #1 
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@arm.com>

One small nit below, otherwise 

Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

> ---
>  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 " "

> +			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
>
Alexandru Elisei July 7, 2023, 1:10 p.m. UTC | #2 
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@arm.com>
> 
> One small nit below, otherwise 
> 
> Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

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
> >
diff mbox series

Patch

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)",
+			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, ...)