From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751486AbdK0JZq (ORCPT ); Mon, 27 Nov 2017 04:25:46 -0500 Received: from smtprelay0013.hostedemail.com ([216.40.44.13]:50261 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751277AbdK0JZm (ORCPT ); Mon, 27 Nov 2017 04:25:42 -0500 X-Session-Marker: 6A6F6540706572636865732E636F6D X-Spam-Summary: 2,0,0,,d41d8cd98f00b204,joe@perches.com,:::::::::,RULES_HIT:41:355:379:541:599:973:982:988:989:1260:1277:1311:1313:1314:1345:1359:1373:1437:1515:1516:1518:1535:1543:1593:1594:1605:1711:1730:1747:1777:1792:2393:2553:2559:2562:2693:2828:3138:3139:3140:3141:3142:3308:3622:3743:3865:3866:3867:3868:3870:3871:3872:3873:3874:4321:4384:4605:5007:6119:6691:7808:7903:8700:10004:10400:10848:11026:11232:11657:11658:11914:12043:12257:12295:12296:12438:12663:12679:12740:12760:12895:13255:13439:13618:14181:14659:14721:21063:21080:21212:21324:21451:21627:30054:30090:30091,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:,MSBL:0,DNSBL:none,Custom_rules:0:0:0,LFtime:2,LUA_SUMMARY:none X-HE-Tag: game09_6bb133eb9e209 X-Filterd-Recvd-Size: 5012 Message-ID: <1511774737.32426.29.camel@perches.com> Subject: Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line From: Joe Perches To: Julia Lawall Cc: Logan Gunthorpe , linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org, Andy Whitcroft Date: Mon, 27 Nov 2017 01:25:37 -0800 In-Reply-To: References: <20171126054037.9743-1-logang@deltatee.com> <1511676085.20482.18.camel@perches.com> <5c0a2778-8e8f-9fbb-b13f-1d880acb949b@deltatee.com> <1511735382.20482.27.camel@perches.com> <1511745165.20482.34.camel@perches.com> Content-Type: text/plain; charset="ISO-8859-1" X-Mailer: Evolution 3.26.1-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 2017-11-27 at 07:08 +0100, Julia Lawall wrote: > > On Sun, 26 Nov 2017, Joe Perches wrote: > > > On Sun, 2017-11-26 at 23:44 +0100, Julia Lawall wrote: > > > My semantic patch and results are below. The semantic patch has some > > > features that may or may not be desired: > > > > > > 1. It goes beyond printk, pr_xxx, dev_xxx, and netdev_xxx, by finding > > > functions that are sometimes used with a format string ending with a > > > newline. To reduce false positives, such a function is ignored if it is > > > sometimes used with a string that ends in a space. This could lead to > > > false positives where actually one of the calls has a \n that it should > > > not have. > > > > > > 2. Coccinelle puts multipart strings on a single line. So the rule goes > > > a little further and eliminates the multipartness. Basically "xxx " "yyy" > > > becomes "xxx yyy" regardless of the length of the result. > > > > What about the semi-common string concatenation "foo" #var "bar" ? > > I don't think this is an issue. There is no " " pattern in this. It's > true that if the pieces were on separate lines, Coccinelle will now put > them on a single line. I'm not sure I want to bother with this. > > > > 3. Some prints appear not to end with a newline because they end with \n. > > > where .\n was likely intended. Instead of creating \n.\n, the semantic > > > patch just moves the .to the left of the . And if there was .\n. it just > > > drops the final period. > > > > That may be a problem if the sentence is "something...\n" > > I think I was not clear. The sentence ends in ".\n.". > > > There seem to be many false positives in here too. > > Could you point to something specifically? I saw a lot of cases with > prints followed by returns and gotos. I guess those are not likely false > positives. random entries, as your original post is 2.6M (and didn't get to lkml) and I only sampled it at a few places. [] diff -u -p a/lib/locking-selftest.c b/lib/locking-selftest.c --- a/lib/locking-selftest.c +++ b/lib/locking-selftest.c @@ -1186,7 +1186,7 @@ static void dotest(void (*testcase_fn)(v static inline void print_testname(const char *testname) { - printk("%33s:", testname); + printk("%33s:\n", testname); } [] diff -u -p a/lib/dynamic_debug.c b/lib/dynamic_debug.c --- a/lib/dynamic_debug.c +++ b/lib/dynamic_debug.c @@ -562,7 +562,8 @@ void __dynamic_pr_debug(struct _ddebug * vaf.fmt = fmt; vaf.va = &args; - printk(KERN_DEBUG "%s%pV", dynamic_emit_prefix(descriptor, buf), &vaf); + printk(KERN_DEBUG "%s%pV\n", dynamic_emit_prefix(descriptor, buf), + &vaf); va_end(args); } [] diff -u -p a/drivers/tty/serial/ioc4_serial.c b/drivers/tty/serial/ioc4_serial.c --- a/drivers/tty/serial/ioc4_serial.c +++ b/drivers/tty/serial/ioc4_serial.c @@ -2858,8 +2858,7 @@ ioc4_serial_attach_one(struct ioc4_drive "sgi-ioc4serial", soft)) { control->ic_irq = idd->idd_pdev->irq; } else { - printk(KERN_WARNING - "%s : request_irq fails for IRQ 0x%x\n ", + printk(KERN_WARNING "%s : request_irq fails for IRQ 0x%x\n \n", __func__, idd->idd_pdev->irq); } ret = ioc4_attach_local(idd); [] below: the gig_dbg macro and _many_ other append a newline to a format diff -u -p a/drivers/isdn/gigaset/ev-layer.c b/drivers/isdn/gigaset/ev-layer.c --- a/drivers/isdn/gigaset/ev-layer.c +++ b/drivers/isdn/gigaset/ev-layer.c @@ -411,7 +411,7 @@ static void add_cid_event(struct cardsta unsigned next, tail; struct event_t *event; - gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d", type, cid); + gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d\n", type, cid); spin_lock_irqsave(&cs->ev_lock, flags); etc... From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joe Perches Date: Mon, 27 Nov 2017 09:25:37 +0000 Subject: Re: [PATCH v2] checkpatch: Add a warning for log messages that don't end in a new line Message-Id: <1511774737.32426.29.camel@perches.com> List-Id: References: <20171126054037.9743-1-logang@deltatee.com> <1511676085.20482.18.camel@perches.com> <5c0a2778-8e8f-9fbb-b13f-1d880acb949b@deltatee.com> <1511735382.20482.27.camel@perches.com> <1511745165.20482.34.camel@perches.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Julia Lawall Cc: Logan Gunthorpe , linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org, Andy Whitcroft On Mon, 2017-11-27 at 07:08 +0100, Julia Lawall wrote: > > On Sun, 26 Nov 2017, Joe Perches wrote: > > > On Sun, 2017-11-26 at 23:44 +0100, Julia Lawall wrote: > > > My semantic patch and results are below. The semantic patch has some > > > features that may or may not be desired: > > > > > > 1. It goes beyond printk, pr_xxx, dev_xxx, and netdev_xxx, by finding > > > functions that are sometimes used with a format string ending with a > > > newline. To reduce false positives, such a function is ignored if it is > > > sometimes used with a string that ends in a space. This could lead to > > > false positives where actually one of the calls has a \n that it should > > > not have. > > > > > > 2. Coccinelle puts multipart strings on a single line. So the rule goes > > > a little further and eliminates the multipartness. Basically "xxx " "yyy" > > > becomes "xxx yyy" regardless of the length of the result. > > > > What about the semi-common string concatenation "foo" #var "bar" ? > > I don't think this is an issue. There is no " " pattern in this. It's > true that if the pieces were on separate lines, Coccinelle will now put > them on a single line. I'm not sure I want to bother with this. > > > > 3. Some prints appear not to end with a newline because they end with \n. > > > where .\n was likely intended. Instead of creating \n.\n, the semantic > > > patch just moves the .to the left of the . And if there was .\n. it just > > > drops the final period. > > > > That may be a problem if the sentence is "something...\n" > > I think I was not clear. The sentence ends in ".\n.". > > > There seem to be many false positives in here too. > > Could you point to something specifically? I saw a lot of cases with > prints followed by returns and gotos. I guess those are not likely false > positives. random entries, as your original post is 2.6M (and didn't get to lkml) and I only sampled it at a few places. [] diff -u -p a/lib/locking-selftest.c b/lib/locking-selftest.c --- a/lib/locking-selftest.c +++ b/lib/locking-selftest.c @@ -1186,7 +1186,7 @@ static void dotest(void (*testcase_fn)(v static inline void print_testname(const char *testname) { - printk("%33s:", testname); + printk("%33s:\n", testname); } [] diff -u -p a/lib/dynamic_debug.c b/lib/dynamic_debug.c --- a/lib/dynamic_debug.c +++ b/lib/dynamic_debug.c @@ -562,7 +562,8 @@ void __dynamic_pr_debug(struct _ddebug * vaf.fmt = fmt; vaf.va = &args; - printk(KERN_DEBUG "%s%pV", dynamic_emit_prefix(descriptor, buf), &vaf); + printk(KERN_DEBUG "%s%pV\n", dynamic_emit_prefix(descriptor, buf), + &vaf); va_end(args); } [] diff -u -p a/drivers/tty/serial/ioc4_serial.c b/drivers/tty/serial/ioc4_serial.c --- a/drivers/tty/serial/ioc4_serial.c +++ b/drivers/tty/serial/ioc4_serial.c @@ -2858,8 +2858,7 @@ ioc4_serial_attach_one(struct ioc4_drive "sgi-ioc4serial", soft)) { control->ic_irq = idd->idd_pdev->irq; } else { - printk(KERN_WARNING - "%s : request_irq fails for IRQ 0x%x\n ", + printk(KERN_WARNING "%s : request_irq fails for IRQ 0x%x\n \n", __func__, idd->idd_pdev->irq); } ret = ioc4_attach_local(idd); [] below: the gig_dbg macro and _many_ other append a newline to a format diff -u -p a/drivers/isdn/gigaset/ev-layer.c b/drivers/isdn/gigaset/ev-layer.c --- a/drivers/isdn/gigaset/ev-layer.c +++ b/drivers/isdn/gigaset/ev-layer.c @@ -411,7 +411,7 @@ static void add_cid_event(struct cardsta unsigned next, tail; struct event_t *event; - gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d", type, cid); + gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d\n", type, cid); spin_lock_irqsave(&cs->ev_lock, flags); etc...