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

[Markus Armbruster <armbru@redhat.com>]

"Jason J. Herne" <jjherne@linux.vnet.ibm.com> writes:

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

We may want to keep the tabs to make continued stealing of code from the
kernel's checkpatch.pl easier.

> 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 :)

You're right.

> 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 :).

Thanks!  If we can't, then I'm for taking this imperfect patch, because
flagging some of these mistakes is better than flagging none.