Re: [Qemu-devel] [PATCH v2] checkpatch: Detect newlines in error_report and other error functions

[Jason J. Herne]

On 12/14/2015 07:47 AM, Markus Armbruster wrote:
> "Jason J. Herne" <address@hidden> writes:
>
> > We don't want newlines embedded in error messages. This seems to be a common
> > problem with new code so let's try to catch it with checkpatch.
> >
> > This will not catch cases where newlines are inserted into the middle of an
> > existing multi-line statement. But those cases should be rare.
> >
> > Signed-off-by: Jason J. Herne <address@hidden>
>
> Awesome!
>
> Ironically, checkpatch complains a lot about this patch: 31 "code indent
> should never use tabs" and four "line over 80 characters".  Since the
> script uses tabs pretty consistently, I guess we'll want to ignore the
> former.  Long lines are also frequent, but three of the four new ones
> are comments that ought to be wrapped.  Could be done on commit.

Yep, the whole file uses tabs. I think we should convert it to spaces to be
consistent with Qemu coding guidelines. I can work up that patch if it would
be accepted.

I'll fix the comments.

> To test this patch, I fed it a revert of my series.  Score:
>
> * Revert "error: Clean up errors with embedded newlines (again), part 2"
>
>    1/6
>
>    Pretty difficult cases.  The last one is flagged, perhaps because the
>    string is split right after an embedded newline.
>
> * Revert "error: Clean up errors with embedded newlines (again), part 1"
>
>    2/2
>
> * Revert "error: Strip trailing '\n' from error string arguments (again)"
>
>    10/23
>
>    Could you look into catching a few more of these?
>
> > ---
> >   scripts/checkpatch.pl | 39 +++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 39 insertions(+)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index b0f6e11..51ea667 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -2511,6 +2511,45 @@ sub process {
> >                         WARN("use QEMU instead of Qemu or QEmu\n" . $herecurr);
> >                 }
> >
> > +# Qemu error function tests
> > +
> > +       # Find newlines in error function text
> > +       my $qemu_error_funcs = qr{error_setg|
> > +                               error_setg_errno|
> > +                               error_setg_win32|
> > +                               error_set|
> > +                               error_vprintf|
> > +                               error_printf|
> > +                               error_printf_unless_qmp|
> > +                               error_vreport|
> > +                               error_report}x;
> > +
> > +       if ($rawline =~ /\b(?:$qemu_error_funcs)\s*\(\s*\".*\\n/) {
> > +               WARN("Error function text should not contain newlines\n" . 
> > $herecurr);
> > +       }
> > +
> > +       # Continue checking for error function text that contains newlines. This
> > +       # check handles cases where string literals are spread over multiple 
> > lines.
> > +       # Example:
> > +       # error_report("Error msg line #1"
> > +       #              "Error msg line #2\n");
> > +       my $quoted_newline_regex = qr{\+\s*\".*\\n.*\"};
> > +       my $continued_str_literal = qr{\+\s*\".*\"};
> > +
> > +       if ($rawline =~ /$quoted_newline_regex/) {
> > +               # Backtrack to first line that does not contain only a quoted 
> > literal
> > +               # and assume that it is the start of the statement.
> > +               my $i = $linenr - 2;
> > +
> > +               while (($i >= 0) & $rawlines[$i] =~ /$continued_str_literal/) {
> > +                       $i--;
> > +               }
>
> I guess this fails to backtrack over lines that consisting of string
> literals and macros (that expand into string literals), such as in this
> example from my Revert "error: Strip trailing '\n' from error string
> arguments (again)":
>
>       if (sector_num > bs->total_sectors) {
>           error_report("Wrong offset: sector_num=0x%" PRIx64
> -                     " total_sectors=0x%" PRIx64,
> -                     sector_num, bs->total_sectors);
> +                " total_sectors=0x%" PRIx64 "\n",
> +                sector_num, bs->total_sectors);
>           return -EIO;
>       }
>
> > +

The problem here has more to do with how patches are structured. In 
particular, the - lines. I could try to add code to ignore the - lines. 
Really though, this is a limitation of checkpatch. We do not currently 
(not that I could see) have a good method of isolating a single 
multiline statement. But that is a problem for a different day I think :)

Ultimately, we'll never catch all of them with checkpatch. For example:
     "line 5"
     "line 6"
     "line 7"
+    "line 8\n"
    "line 7"
    "line 8"
    "line 9"
...
The patch only contains so much context info so in this case, because we 
do not have the call to error_report in the context we will never catch 
the error.

But I will take a look at this series and see if we can do better :).

--
-- Jason J. Herne (address@hidden)