Fix usage of error_append_hint()

[Qemu-devel] [PATCH 17/17] checkpatch: Warn when errp is passed to error_append_hint()

Posted by Greg Kurz 6 years, 1 month ago

Passing errp from the argument list to error_append_hint()
isn't recommended because it may cause unwanted behaviours
when errp is equal to &error_fatal or &error_abort.

First, error_append_hint() aborts QEMU when passed either of
those.

Second, consider the following:

    void foo(Error **errp)
    {
         error_setg(errp, "foo");
         error_append_hint(errp, "Try bar\n");
    }

error_setg() causes QEMU to exit or abort, and hints aren't
added.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 scripts/checkpatch.pl |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index aa9a354a0e7e..17ce026282a6 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2902,6 +2902,10 @@ sub process {
 		}
 	}
 
+		if ($line =~ /error_append_hint\(errp/) {
+		    WARN("suspicious errp passed to error_append_hint()\n" .
+			 $herecurr);
+		}
 # check for non-portable libc calls that have portable alternatives in QEMU
 		if ($line =~ /\bffs\(/) {
 			ERROR("use ctz32() instead of ffs()\n" . $herecurr);

Re: [Qemu-devel] [PATCH 17/17] checkpatch: Warn when errp is passed to error_append_hint()

Posted by Philippe Mathieu-Daudé 6 years, 1 month ago

On 9/17/19 12:22 PM, Greg Kurz wrote:
> Passing errp from the argument list to error_append_hint()
> isn't recommended because it may cause unwanted behaviours
> when errp is equal to &error_fatal or &error_abort.
> 
> First, error_append_hint() aborts QEMU when passed either of
> those.
> 
> Second, consider the following:
> 
>     void foo(Error **errp)
>     {
>          error_setg(errp, "foo");
>          error_append_hint(errp, "Try bar\n");
>     }
> 
> error_setg() causes QEMU to exit or abort, and hints aren't
> added.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  scripts/checkpatch.pl |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index aa9a354a0e7e..17ce026282a6 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2902,6 +2902,10 @@ sub process {
>  		}
>  	}
>  
> +		if ($line =~ /error_append_hint\(errp/) {

Checking for 'errp' variable name seems fragile, but all the codebase
uses exactly this name, so it is sufficient.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +		    WARN("suspicious errp passed to error_append_hint()\n" .
> +			 $herecurr);
> +		}
>  # check for non-portable libc calls that have portable alternatives in QEMU
>  		if ($line =~ /\bffs\(/) {
>  			ERROR("use ctz32() instead of ffs()\n" . $herecurr);
> 
>

Re: [Qemu-devel] [PATCH 17/17] checkpatch: Warn when errp is passed to error_append_hint()

Posted by Cornelia Huck 6 years, 1 month ago

On Tue, 17 Sep 2019 12:22:18 +0200
Greg Kurz <groug@kaod.org> wrote:

> Passing errp from the argument list to error_append_hint()
> isn't recommended because it may cause unwanted behaviours
> when errp is equal to &error_fatal or &error_abort.
> 
> First, error_append_hint() aborts QEMU when passed either of
> those.
> 
> Second, consider the following:
> 
>     void foo(Error **errp)
>     {
>          error_setg(errp, "foo");
>          error_append_hint(errp, "Try bar\n");
>     }
> 
> error_setg() causes QEMU to exit or abort, and hints aren't
> added.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  scripts/checkpatch.pl |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index aa9a354a0e7e..17ce026282a6 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2902,6 +2902,10 @@ sub process {
>  		}
>  	}
>  

Maybe add a comment here?

# using errp is common practice, so that check should hopefully be enough

> +		if ($line =~ /error_append_hint\(errp/) {
> +		    WARN("suspicious errp passed to error_append_hint()\n" .

Add "(use a local error object)"?

> +			 $herecurr);
> +		}
>  # check for non-portable libc calls that have portable alternatives in QEMU
>  		if ($line =~ /\bffs\(/) {
>  			ERROR("use ctz32() instead of ffs()\n" . $herecurr);
>

Re: [Qemu-devel] [PATCH 17/17] checkpatch: Warn when errp is passed to error_append_hint()

Posted by Greg Kurz 6 years, 1 month ago

On Tue, 17 Sep 2019 13:29:36 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 17 Sep 2019 12:22:18 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > Passing errp from the argument list to error_append_hint()
> > isn't recommended because it may cause unwanted behaviours
> > when errp is equal to &error_fatal or &error_abort.
> > 
> > First, error_append_hint() aborts QEMU when passed either of
> > those.
> > 
> > Second, consider the following:
> > 
> >     void foo(Error **errp)
> >     {
> >          error_setg(errp, "foo");
> >          error_append_hint(errp, "Try bar\n");
> >     }
> > 
> > error_setg() causes QEMU to exit or abort, and hints aren't
> > added.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  scripts/checkpatch.pl |    4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index aa9a354a0e7e..17ce026282a6 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -2902,6 +2902,10 @@ sub process {
> >  		}
> >  	}
> >  
> 
> Maybe add a comment here?
> 
> # using errp is common practice, so that check should hopefully be enough
> 
> > +		if ($line =~ /error_append_hint\(errp/) {
> > +		    WARN("suspicious errp passed to error_append_hint()\n" .
> 
> Add "(use a local error object)"?
> 

Sure. I'll add both.

> > +			 $herecurr);
> > +		}
> >  # check for non-portable libc calls that have portable alternatives in QEMU
> >  		if ($line =~ /\bffs\(/) {
> >  			ERROR("use ctz32() instead of ffs()\n" . $herecurr);
> > 
>