From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41875) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8UOC-0001Q1-PW for qemu-devel@nongnu.org; Mon, 14 Dec 2015 09:46:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a8UO8-00010K-IL for qemu-devel@nongnu.org; Mon, 14 Dec 2015 09:46:12 -0500 Received: from e34.co.us.ibm.com ([32.97.110.152]:51786) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8UO8-0000zu-By for qemu-devel@nongnu.org; Mon, 14 Dec 2015 09:46:08 -0500 Received: from localhost by e34.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 14 Dec 2015 07:46:05 -0700 Received: from b03cxnp07028.gho.boulder.ibm.com (b03cxnp07028.gho.boulder.ibm.com [9.17.130.15]) by d03dlp01.boulder.ibm.com (Postfix) with ESMTP id 8F0981FF0043 for ; Mon, 14 Dec 2015 07:34:13 -0700 (MST) Received: from d03av04.boulder.ibm.com (d03av04.boulder.ibm.com [9.17.195.170]) by b03cxnp07028.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id tBEEk2ke27394220 for ; Mon, 14 Dec 2015 07:46:02 -0700 Received: from d03av04.boulder.ibm.com (loopback [127.0.0.1]) by d03av04.boulder.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id tBEEk1ZD020836 for ; Mon, 14 Dec 2015 07:46:02 -0700 References: <1449858642-24267-1-git-send-email-jjherne@linux.vnet.ibm.com> <87poy9rybu.fsf@blackfin.pond.sub.org> From: "Jason J. Herne" Message-ID: <566ED627.2030706@linux.vnet.ibm.com> Date: Mon, 14 Dec 2015 09:45:59 -0500 MIME-Version: 1.0 In-Reply-To: <87poy9rybu.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] checkpatch: Detect newlines in error_report and other error functions Reply-To: jjherne@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: blauwirbel@gmail.com, cornelia.huck@de.ibm.com, qemu-devel@nongnu.org On 12/14/2015 07:47 AM, Markus Armbruster wrote: > "Jason J. Herne" 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 > > 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)