* [PATCH] eudyptula challenge @ 2017-04-01 1:59 Chewie Lin 2017-04-01 1:59 ` [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning Chewie Lin 0 siblings, 1 reply; 17+ messages in thread From: Chewie Lin @ 2017-04-01 1:59 UTC (permalink / raw) To: greg, forest, devel, linux-kernel Hi all: Better and improved changelog version 3. I'm submitting this patch as part of Eudyptula challenge to fix a coding style problem. Thanks for taking time on this trivial patch. sl424 Chewie Lin (1): drivers/staging/vt6656/main_usb.c: checkpatch warning drivers/staging/vt6656/main_usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.10.0 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning 2017-04-01 1:59 [PATCH] eudyptula challenge Chewie Lin @ 2017-04-01 1:59 ` Chewie Lin 2017-04-01 2:15 ` Randy Dunlap ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Chewie Lin @ 2017-04-01 1:59 UTC (permalink / raw) To: greg, forest, devel, linux-kernel 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. Signed-off-by: Chewie Lin <linsh@oregonstate.edu> --- drivers/staging/vt6656/main_usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c index 9e074e9..2d9e7af 100644 --- a/drivers/staging/vt6656/main_usb.c +++ b/drivers/staging/vt6656/main_usb.c @@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv) status = usb_reset_device(priv->usb); if (status) dev_warn(&priv->usb->dev, - "usb_device_reset fail status=%d\n", status); + "%s=%d\n", "usb_device_reset fail status", status); } static void vnt_free_int_bufs(struct vnt_private *priv) -- 2.10.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning 2017-04-01 1:59 ` [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning Chewie Lin @ 2017-04-01 2:15 ` Randy Dunlap 2017-04-01 2:45 ` Joe Perches 2017-04-01 2:16 ` Joe Perches 2017-04-01 3:32 ` Al Viro 2 siblings, 1 reply; 17+ messages in thread From: Randy Dunlap @ 2017-04-01 2:15 UTC (permalink / raw) To: Chewie Lin, greg, forest, devel, linux-kernel On 03/31/17 18:59, 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. > > Signed-off-by: Chewie Lin <linsh@oregonstate.edu> > --- > drivers/staging/vt6656/main_usb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c > index 9e074e9..2d9e7af 100644 > --- a/drivers/staging/vt6656/main_usb.c > +++ b/drivers/staging/vt6656/main_usb.c > @@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv) > status = usb_reset_device(priv->usb); > if (status) > dev_warn(&priv->usb->dev, > - "usb_device_reset fail status=%d\n", status); > + "%s=%d\n", "usb_device_reset fail status", status); > } > > static void vnt_free_int_bufs(struct vnt_private *priv) > As other people have said: This function is usb_device_reset(). If that function name string is to be used in the message, it should be done so by using __func__. See http://marc.info/?l=linux-driver-devel&m=149095639202492&w=2 Or is the called (failing) function name is to be kept in the message (as it is currently), then the message should contain "usb_reset_device" instead of "usb_device_reset". See http://marc.info/?l=linux-driver-devel&m=149098680312723&w=2 -- ~Randy ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning 2017-04-01 2:15 ` Randy Dunlap @ 2017-04-01 2:45 ` Joe Perches 2017-04-01 3:18 ` Chewie Lin 0 siblings, 1 reply; 17+ messages in thread From: Joe Perches @ 2017-04-01 2:45 UTC (permalink / raw) To: Randy Dunlap, Chewie Lin, greg, forest, devel, linux-kernel On Fri, 2017-03-31 at 19:15 -0700, Randy Dunlap wrote: > On 03/31/17 18:59, 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. > > > > Signed-off-by: Chewie Lin <linsh@oregonstate.edu> > > --- > > drivers/staging/vt6656/main_usb.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c > > index 9e074e9..2d9e7af 100644 > > --- a/drivers/staging/vt6656/main_usb.c > > +++ b/drivers/staging/vt6656/main_usb.c > > @@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv) > > status = usb_reset_device(priv->usb); > > if (status) > > dev_warn(&priv->usb->dev, > > - "usb_device_reset fail status=%d\n", status); > > + "%s=%d\n", "usb_device_reset fail status", status); > > } > > > > static void vnt_free_int_bufs(struct vnt_private *priv) > > > > As other people have said: > > This function is usb_device_reset(). If that function name string is to be > used in the message, it should be done so by using __func__. > See http://marc.info/?l=linux-driver-devel&m=149095639202492&w=2 > > Or is the called (failing) function name is to be kept in the message (as it > is currently), then the message should contain "usb_reset_device" instead of > "usb_device_reset". See http://marc.info/?l=linux-driver-devel&m=149098680312723&w=2 If this is to be changed at all, I suggest just getting rid of the function. --- drivers/staging/vt6656/main_usb.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c index 9e074e9daf4e..799aeeb6812d 100644 --- a/drivers/staging/vt6656/main_usb.c +++ b/drivers/staging/vt6656/main_usb.c @@ -407,16 +407,6 @@ static void vnt_free_rx_bufs(struct vnt_private *priv) } } -static void usb_device_reset(struct vnt_private *priv) -{ - int status; - - status = usb_reset_device(priv->usb); - if (status) - dev_warn(&priv->usb->dev, - "usb_device_reset fail status=%d\n", status); -} - static void vnt_free_int_bufs(struct vnt_private *priv) { kfree(priv->int_buf.data_buf); @@ -995,7 +985,9 @@ vt6656_probe(struct usb_interface *intf, const struct usb_device_id *id) SET_IEEE80211_DEV(priv->hw, &intf->dev); - usb_device_reset(priv); + rc = usb_reset_device(priv->usb); + if (rc) + dev_warn(&priv->usb->dev, "usb_reset_device failed: %d\n", rc); clear_bit(DEVICE_FLAGS_DISCONNECTED, &priv->flags); vnt_reset_command_timer(priv); ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning 2017-04-01 2:45 ` Joe Perches @ 2017-04-01 3:18 ` Chewie Lin 2017-04-01 3:33 ` Joe Perches 0 siblings, 1 reply; 17+ messages in thread From: Chewie Lin @ 2017-04-01 3:18 UTC (permalink / raw) To: Joe Perches; +Cc: Randy Dunlap, greg, forest, devel, linux-kernel On Fri, Mar 31, 2017 at 07:45:09PM -0700, Joe Perches wrote: > On Fri, 2017-03-31 at 19:15 -0700, Randy Dunlap wrote: > > On 03/31/17 18:59, 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. > > > > > > Signed-off-by: Chewie Lin <linsh@oregonstate.edu> > > > --- > > > drivers/staging/vt6656/main_usb.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c > > > index 9e074e9..2d9e7af 100644 > > > --- a/drivers/staging/vt6656/main_usb.c > > > +++ b/drivers/staging/vt6656/main_usb.c > > > @@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv) > > > status = usb_reset_device(priv->usb); > > > if (status) > > > dev_warn(&priv->usb->dev, > > > - "usb_device_reset fail status=%d\n", status); > > > + "%s=%d\n", "usb_device_reset fail status", status); > > > } > > > > > > static void vnt_free_int_bufs(struct vnt_private *priv) > > > > > > > As other people have said: > > > > This function is usb_device_reset(). If that function name string is to be > > used in the message, it should be done so by using __func__. > > See http://marc.info/?l=linux-driver-devel&m=149095639202492&w=2 > > > > Or is the called (failing) function name is to be kept in the message (as it > > is currently), then the message should contain "usb_reset_device" instead of > > "usb_device_reset". See http://marc.info/?l=linux-driver-devel&m=149098680312723&w=2 > > If this is to be changed at all, I suggest > just getting rid of the function. These are good points, but any feedback on the dev_warn() call itself? I was trying to fix the checkpatch warning on my first try. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning 2017-04-01 3:18 ` Chewie Lin @ 2017-04-01 3:33 ` Joe Perches 0 siblings, 0 replies; 17+ messages in thread From: Joe Perches @ 2017-04-01 3:33 UTC (permalink / raw) To: Chewie Lin; +Cc: Randy Dunlap, greg, forest, devel, linux-kernel On Fri, 2017-03-31 at 20:18 -0700, Chewie Lin wrote: > These are good points, but any feedback on the dev_warn() call itself? > I was trying to fix the checkpatch warning on my first try. Using "%s" with a long string is generally inefficient. Compare the compiled object size of printf("%s is %d\n", "Long string", 1); to printf("Long string is %d\n", 1); ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning 2017-04-01 1:59 ` [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning Chewie Lin 2017-04-01 2:15 ` Randy Dunlap @ 2017-04-01 2:16 ` Joe Perches 2017-04-01 3:32 ` Al Viro 2 siblings, 0 replies; 17+ messages in thread From: Joe Perches @ 2017-04-01 2:16 UTC (permalink / raw) To: Chewie Lin, greg, forest, devel, linux-kernel On Fri, 2017-03-31 at 18:59 -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. > > Signed-off-by: Chewie Lin <linsh@oregonstate.edu> > --- > drivers/staging/vt6656/main_usb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c > index 9e074e9..2d9e7af 100644 > --- a/drivers/staging/vt6656/main_usb.c > +++ b/drivers/staging/vt6656/main_usb.c > @@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv) > status = usb_reset_device(priv->usb); > if (status) > dev_warn(&priv->usb->dev, > - "usb_device_reset fail status=%d\n", status); > + "%s=%d\n", "usb_device_reset fail status", status); So, what is failing? usb_reset_device or usb_device_reset? And this function is used exactly once, is trivial, and would likely be better as direct code in the one place it's used. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning 2017-04-01 1:59 ` [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning Chewie Lin 2017-04-01 2:15 ` Randy Dunlap 2017-04-01 2:16 ` Joe Perches @ 2017-04-01 3:32 ` Al Viro 2017-04-01 3:36 ` Joe Perches 2017-04-01 4:33 ` Chewie Lin 2 siblings, 2 replies; 17+ messages in thread From: Al Viro @ 2017-04-01 3:32 UTC (permalink / raw) To: Chewie Lin; +Cc: greg, forest, devel, linux-kernel 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". ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning 2017-04-01 3:32 ` Al Viro @ 2017-04-01 3:36 ` Joe Perches 2017-04-01 3:46 ` Al Viro 2017-04-01 4:33 ` Chewie Lin 1 sibling, 1 reply; 17+ messages in thread From: Joe Perches @ 2017-04-01 3:36 UTC (permalink / raw) To: Al Viro, Chewie Lin; +Cc: greg, forest, devel, linux-kernel On Sat, 2017-04-01 at 04:32 +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 [] > Again, checkpatch warning is badly written In your opinion, what wording would be better? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning 2017-04-01 3:36 ` Joe Perches @ 2017-04-01 3:46 ` Al Viro 2017-04-01 3:52 ` Joe Perches 0 siblings, 1 reply; 17+ messages in thread From: Al Viro @ 2017-04-01 3:46 UTC (permalink / raw) To: Joe Perches; +Cc: Chewie Lin, greg, forest, devel, linux-kernel On Fri, Mar 31, 2017 at 08:36:22PM -0700, Joe Perches wrote: > On Sat, 2017-04-01 at 04:32 +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 > [] > > Again, checkpatch warning is badly written > > In your opinion, what wording would be better? MILD SUGGESTION: don't spell the function name out in format strings; "this_function: foo is %d", n might be better off as "%s: foo is %d", __func__, n in case you ever move it to another function or rename your function. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning 2017-04-01 3:46 ` Al Viro @ 2017-04-01 3:52 ` Joe Perches 2017-04-01 4:08 ` Al Viro 0 siblings, 1 reply; 17+ messages in thread From: Joe Perches @ 2017-04-01 3:52 UTC (permalink / raw) To: Al Viro; +Cc: Chewie Lin, greg, forest, devel, linux-kernel On Sat, 2017-04-01 at 04:46 +0100, Al Viro wrote: > On Fri, Mar 31, 2017 at 08:36:22PM -0700, Joe Perches wrote: > > On Sat, 2017-04-01 at 04:32 +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 > > > > [] > > > Again, checkpatch warning is badly written > > > > In your opinion, what wording would be better? > > MILD SUGGESTION: don't spell the function name out in format strings; > "this_function: foo is %d", n > might be better off as > "%s: foo is %d", __func__, n > in case you ever move it to another function or rename your function. Thank you sir, may I have another. checkpatch messages are single line. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning 2017-04-01 3:52 ` Joe Perches @ 2017-04-01 4:08 ` Al Viro 2017-04-01 4:27 ` Joe Perches 0 siblings, 1 reply; 17+ messages in thread From: Al Viro @ 2017-04-01 4:08 UTC (permalink / raw) To: Joe Perches; +Cc: Chewie Lin, greg, forest, devel, linux-kernel On Fri, Mar 31, 2017 at 08:52:50PM -0700, Joe Perches wrote: > > MILD SUGGESTION: don't spell the function name out in format strings; > > "this_function: foo is %d", n > > might be better off as > > "%s: foo is %d", __func__, n > > in case you ever move it to another function or rename your function. > > Thank you sir, may I have another. > > checkpatch messages are single line. Too bad... Incidentally, being able to get more detailed explanation of a warning might be a serious improvement, especially if it contains the rationale. Hell, something like TeX handling of errors might be a good idea - warning printed, offered actions include 'give more help', 'continue', 'exit', 'from now on suppress this kind of warning', 'from now on just dump this kind of warning into log and keep going', 'from now on dump all warnings into log and keep going'. And yes, I'm serious about having something like "mild suggestion" as possible severity - people are using that thing to look for potential improvements to make and 'such and such change might be useful for such and such reasons' is a lot more useful than 'this needs to be thus because it must be thus or I'll keep warning'. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning 2017-04-01 4:08 ` Al Viro @ 2017-04-01 4:27 ` Joe Perches 0 siblings, 0 replies; 17+ messages in thread From: Joe Perches @ 2017-04-01 4:27 UTC (permalink / raw) To: Al Viro; +Cc: Chewie Lin, greg, forest, devel, linux-kernel On Sat, 2017-04-01 at 05:08 +0100, Al Viro wrote: > On Fri, Mar 31, 2017 at 08:52:50PM -0700, Joe Perches wrote: > > > > MILD SUGGESTION: don't spell the function name out in format strings; > > > "this_function: foo is %d", n > > > might be better off as > > > "%s: foo is %d", __func__, n > > > in case you ever move it to another function or rename your function. > > > > Thank you sir, may I have another. > > > > checkpatch messages are single line. > > Too bad... Incidentally, being able to get more detailed explanation of > a warning might be a serious improvement, especially if it contains > the rationale. Hell, something like TeX handling of errors might be > a good idea - warning printed, offered actions include 'give more help', > 'continue', 'exit', 'from now on suppress this kind of warning', 'from > now on just dump this kind of warning into log and keep going', 'from > now on dump all warnings into log and keep going'. Well, there is the possibility to have longer messages. It's just the --terse option has to be somewhat sensible. > And yes, I'm serious about having something like "mild suggestion" as > possible severity - people are using that thing to look for potential > improvements to make and 'such and such change might be useful for such > and such reasons' is a lot more useful than 'this needs to be thus because > it must be thus or I'll keep warning'. I agree about checkpatch and ERROR/WARNING/CHECK vs some other wording. https://lkml.org/lkml/2016/8/27/180 https://lkml.org/lkml/2015/7/16/568 The other thing that might help is for people to take the warnings the script produces less seriously. Maybe convert: ERROR -> defect WARNING -> unstylish CHECK -> nitpick or some such. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning 2017-04-01 3:32 ` Al Viro 2017-04-01 3:36 ` Joe Perches @ 2017-04-01 4:33 ` Chewie Lin 1 sibling, 0 replies; 17+ messages in thread From: Chewie Lin @ 2017-04-01 4:33 UTC (permalink / raw) To: Al Viro; +Cc: greg, forest, devel, linux-kernel 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) ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] eudyptula challenge @ 2017-03-31 20:58 Chewie Lin 0 siblings, 0 replies; 17+ messages in thread From: Chewie Lin @ 2017-03-31 20:58 UTC (permalink / raw) To: greg, forest, devel, linux-kernel Hi greg and forest: I hate to bother but one more, with better changelog. I'm submitting this patch as part of Eudyptula challenge to fix a coding style problem. Thanks for taking time on this trivial patch. sl424 Chewie Lin (1): drivers/staging/vt6656/main_usb.c: checkpatch warning drivers/staging/vt6656/main_usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.10.0 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] Eudyptula challenge @ 2017-03-31 10:01 Chewie Lin 0 siblings, 0 replies; 17+ messages in thread From: Chewie Lin @ 2017-03-31 10:01 UTC (permalink / raw) To: greg, forest, devel, linux-kernel Hi greg k-h and forest: Sorry about all the spam I've been sending earlier. One more try. I'm submitting this patch as part of Eudyptula challenge to fix a coding style problem. Thanks for taking time on this trivial patch. sl424 Chewie Lin (1): fixed a checkpatch warning drivers/staging/vt6656/main_usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.10.0 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] Eudyptula challenge @ 2017-03-31 8:40 Chewie Lin 0 siblings, 0 replies; 17+ messages in thread From: Chewie Lin @ 2017-03-31 8:40 UTC (permalink / raw) To: greg, forest, devel, linux-kernel Hi, I'm submitting this patch as part of Eudyptula challenge to fix a coding style problem. Thanks for taking time on this trivial patch. sl424 Chewie Lin (1): fixed a checkpatch warning drivers/staging/vt6656/main_usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.10.0 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-04-01 4:32 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-01 1:59 [PATCH] eudyptula challenge Chewie Lin 2017-04-01 1:59 ` [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning Chewie Lin 2017-04-01 2:15 ` Randy Dunlap 2017-04-01 2:45 ` Joe Perches 2017-04-01 3:18 ` Chewie Lin 2017-04-01 3:33 ` Joe Perches 2017-04-01 2:16 ` Joe Perches 2017-04-01 3:32 ` Al Viro 2017-04-01 3:36 ` Joe Perches 2017-04-01 3:46 ` Al Viro 2017-04-01 3:52 ` Joe Perches 2017-04-01 4:08 ` Al Viro 2017-04-01 4:27 ` Joe Perches 2017-04-01 4:33 ` Chewie Lin -- strict thread matches above, loose matches on Subject: below -- 2017-03-31 20:58 [PATCH] eudyptula challenge Chewie Lin 2017-03-31 10:01 [PATCH] Eudyptula challenge Chewie Lin 2017-03-31 8:40 Chewie Lin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.