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

[Markus Armbruster]

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

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;
     }

> +
> +             if ($rawlines[$i] =~ /\b(?:$qemu_error_funcs)\s*\(/) {
> +                     WARN("Error function text should not contain 
> newlines\n" . $herecurr);
> +             }
> +     }
> +
>  # check for non-portable ffs() calls that have portable alternatives in QEMU
>               if ($line =~ /\bffs\(/) {
>                       ERROR("use ctz32() instead of ffs()\n" . $herecurr);

Even in its current state, better than nothing.