linux-kernel - Re: [PATCH 001/001] drivers/staging/vt6656/main

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] [day] [month] [year] [list]

Date:   Fri, 31 Mar 2017 21:33:22 -0700
From:   Chewie Lin <linsh@...gonstate.edu>
To:     Al Viro <viro@...IV.linux.org.uk>
Cc:     greg@...ah.com, forest@...ttletooquiet.net,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch
 warning

On Sat, Apr 01, 2017 at 04:32:39AM +0100, Al Viro wrote:
> On Fri, Mar 31, 2017 at 06:59:19PM -0700, Chewie Lin wrote:
> > Replace string with formatted arguments in the dev_warn() call. It removes
> > the checkpatch warning:
> > 
> > 	WARNING: Prefer using "%s", __func__ to embedded function names
> > 	#417: FILE: main_usb.c:417:
> > 	+			 "usb_device_reset fail status=%d\n", status);
> > 
> > 	total: 0 errors, 1 warnings, 1058 lines checked
> > 
> > And after fix:
> > 
> > 	main_usb.c has no obvious style problems and is ready for submission.
> 
> 
> > -			 "usb_device_reset fail status=%d\n", status);
> > +			 "%s=%d\n", "usb_device_reset fail status", status);
> 
> Would you mind explaining the meaning of that warning?  Incidentally, why not
> 			  "%se_reset fail status=%d\n", "usb_devic", status);
> - it would produce the identical output and silences checkpatch even more
> reliably.  Discuss.
> 
> Sarcasm aside, when you are proposing a change, there are several questions you
> really must ask yourself and be able to answer.  First and foremost, of course,
> is
> 	* Why is it an improvement?
> If the answer to that was "it makes a warning go away", the next questions are
> 	* What are we warned about?
> 	* What is problematic in the original variant?
> 	* Is the replacement free from the problem we had in the original?
> and if the answer to any of that is "I don't know", the next step is _not_
> "send it anyway".  "Try to figure out" might be a good idea, with usual
> heuristics regarding the reading comprehension ("if a sentence remains
> a mystery, see if there are any unfamiliar words skipped at the first reading
> and find what they mean", etc.) applying.
> 
> In this particular case the part you've apparently skipped was, indeed,
> critical - "__func__".  I am not trying to defend the quality of checkpatch -
> the warning is badly worded, the tool is badly written and more often than
> not its suggestions reflect nothing but authors' arbitrary preferences.
> 
> This one, however, does have some rationale behind it.  Namely, if some
> debugging output contains the name of a function that has produced it,
> having that name spelled out in format string is not a good idea.  If
> that code gets moved elsewhere one would have to replace the name in format
> string or face confusing messages refering to the function where that
> code used to be.  Since __func__ is interpreted as a constant string
> initialized with the name of the function it's used in, it is possible
> to avoid spelling the function name out - instead of
> "my_function: pink elephants; reduce the daily intake to %d glasses\n", n
> one could use
> "%s: pink elephants; reduce the daily intake to %d glasses\n", __func__, n
> producing the same output and avoiding the need to adjust anything when
> the whole thing is moved to another function.  It's not particularly big
> deal, but it's a more or less reasonable suggestion.
> 
> Your change, of course, has achieved only one thing - it has moved the
> explicitly spelled out function name out of format string.  It's still
> just as explicitly spelled out, still would need to be adjusted if moved
> to another function and the whole thing has become harder to read and
> understand since you've buried other parts of message in the place where
> it's harder to follow.
> 
> Again, checkpatch warning is badly written, but the main problem with
> your posting is not that you had been confused by checkpatch - it's
> that you have posted it based on an incomprehensible message with no better
> rationale than "it shuts checkpatch up, dunno why and what about".

Al, brilliant, that's exactly what I was trying to do on my first try. 
The checkpatch *is* confusing. It's fine with a simple string but doesn't 
like it when it's formatted string. From what you said, I 
think this may work better and more portable: 

"%s: fail status = %d\n", "usb_device_reset", status)