netdev - Re: [PATCH] wcn36xx: Fix logging macro with unnecessary semicolon

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 07 Nov 2013 11:40:21 -0800
From:	Joe Perches <joe@...ches.com>
To:	Eugene Krasnikov <k.eugene.e@...il.com>
Cc:	"Luis R. Rodriguez" <mcgrof@....qualcomm.com>,
	"John W. Linville" <linville@...driver.com>,
	wcn36xx <wcn36xx@...ts.infradead.org>,
	linux-wireless <linux-wireless@...r.kernel.org>,
	netdev@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	ath5k-devel@...ts.ath5k.org,
	ath9k-devel <ath9k-devel@...ts.ath9k.org>,
	ath10k@...ts.infradead.org, Kalle Valo <kvalo@....qualcomm.com>,
	wil6210@....qualcomm.com
Subject: Re: [PATCH] wcn36xx: Fix logging macro with unnecessary semicolon

On Thu, 2013-11-07 at 07:32 +0000, Eugene Krasnikov wrote:
> Hi Joe,

Hi Eugene.

> I personally like the idea of making some kind of framework on top of
> printing because all ath drivers are using the same printing approach
> and combining all that code in one place will reduce amount of code in
> each particular driver. As a true engineer i like when it's less code
> = less work to do = less bugs:)
> 
> Suggestion is to send a patch with semicolon fix only

It's a one line fix, and pretty trivial to do yourself if you
want it.

I do think though the slightly odd wcn36xx_<level> uses of:

-#define wcn36xx_err(fmt, arg...)                               \
-       printk(KERN_ERR pr_fmt("ERROR " fmt), ##arg);
-
-#define wcn36xx_warn(fmt, arg...)                              \
-       printk(KERN_WARNING pr_fmt("WARNING " fmt), ##arg)
-
-#define wcn36xx_info(fmt, arg...)              \
-       printk(KERN_INFO pr_fmt(fmt), ##arg)
-
[]
+#define wcn36xx_err(fmt, ...)                                  \
+       pr_err("ERROR " fmt, ##__VA_ARGS__)
+#define wcn36xx_warn(fmt, ...)                                 \
+       pr_warn("WARNING " fmt, ##__VA_ARGS__)
+#define wcn36xx_info(fmt, ...)                                 \
+       pr_info(fmt, ##__VA_ARGS__)

should still be converted to more normal pr_<level> like I did.

I don't believe that prefixing of "ERROR " and "WARNING " to
pr_err and pr_warn actually gain anything as the same level
information can be got already with dmesg -r via the "<3>" and
"<4>" prefixes or dmesg -l3 -l4

> have a
> second round of convincing ath guys to change printing code. How does
> that sound?

Luis?  Kalle?  Other Qualcomm/Atheros folk?

One of the nominal benefits of separating the ath_<level>
macros by subsystem was perf/tracing.  I believe some of the
ath subsystems have integrated the dmesg and tracing code.

I'm not sure how separable those may be here or if it's even
desirable to separate them.  I'd guess not.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html