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

["Jason J. Herne" <jjherne@linux.vnet.ibm.com>]

On 12/14/2015 07:47 AM, Markus Armbruster wrote:
> "Jason J. Herne" <jjherne@linux.vnet.ibm.com> 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 <jjherne@linux.vnet.ibm.com>
>
> 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 (jjherne@linux.vnet.ibm.com)