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

[Philippe Mathieu-Daudé]

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 <address@hidden>
> ---
>  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é <address@hidden>

> +                 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);
> 
>