| Message ID | 156871573833.196432.7906362695920387537.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
| Series | Fix usage of error_append_hint() | expand |
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); > >
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); >
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); > > >
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);
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(+)