| Submitter | James Bottomley |
|---|---|
| Date | 2009-11-03 18:38:08 |
| Message ID | <1257273488.9427.27.camel@mulgrave.site> |
| Download | mbox | patch |
| Permalink | /patch/57353/ |
| State | New |
| Headers | show |
Comments
On Tue, 03 Nov 2009 12:38:08 -0600 James Bottomley <James.Bottomley@suse.de> wrote: > strstrip strips whitespace from the beginning and end of a string. I > agree you have to take the returned pointer if you want to strip from > the beginning. However, if you wish to keep the whitespace at the > beginning and only wish strstrip to remove it from the end, then it's > entirely legitimate to discard the returned pointer. > > This is what we have in drivers/scsi/ipr.c and the patch to make > strstrip __must_check is now causing SCSI spurious warnings in that > code. > Would prefer to keep the warning and to patch ipr.c, please. We found I think three call sites which were incorrectly ignoring the strstrip() return value and it's reasonable to fear that others will make the same mistake in the future. And maybe ipr.c _should_ be patched. Right now it's assuming that the string coming back from the device has no leading whitespace. Why trim any possible trailing whitespace but not trim any possible leading whitespace? Or.. /* * Comment goes here */ static inline void strsrip_tail(char *str) { char *x __used; x = strstrip(str); } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
static inline void strsrip_tail(char *str)
{
- char *x __used;
- x = strstrip(str);
+ char *x = strstrip(str);
+ BUG_ON(x != str);
}
2009/11/4 Matthew Wilcox <matthew@wil.cx>: > > static inline void strsrip_tail(char *str) > { > - char *x __used; > - x = strstrip(str); > + char *x = strstrip(str); > + BUG_ON(x != str); > } Looks reasonable to me :) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Tue, 3 Nov 2009 12:04:20 -0700 Matthew Wilcox <matthew@wil.cx> wrote: > > static inline void strsrip_tail(char *str) > { > - char *x __used; > - x = strstrip(str); > + char *x = strstrip(str); > + BUG_ON(x != str); That breaks it. It was fine before you did that but now it blows up if there are leading spaces. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Tue, 2009-11-03 at 10:59 -0800, Andrew Morton wrote: > On Tue, 03 Nov 2009 12:38:08 -0600 > James Bottomley <James.Bottomley@suse.de> wrote: > > > strstrip strips whitespace from the beginning and end of a string. I > > agree you have to take the returned pointer if you want to strip from > > the beginning. However, if you wish to keep the whitespace at the > > beginning and only wish strstrip to remove it from the end, then it's > > entirely legitimate to discard the returned pointer. > > > > This is what we have in drivers/scsi/ipr.c and the patch to make > > strstrip __must_check is now causing SCSI spurious warnings in that > > code. > > > > Would prefer to keep the warning and to patch ipr.c, please. We found > I think three call sites which were incorrectly ignoring the strstrip() > return value and it's reasonable to fear that others will make the same > mistake in the future. What's the problem with the mistake ... additional leading whitespace? > And maybe ipr.c _should_ be patched. Right now it's assuming that the > string coming back from the device has no leading whitespace. Why trim > any possible trailing whitespace but not trim any possible leading > whitespace? I think it doesn't care. It wants to append an error code to the string, and to make it more visible it wants to strip trailing whitespace before doing so. > Or.. > > /* > * Comment goes here > */ > static inline void strsrip_tail(char *str) > { > char *x __used; > x = strstrip(str); > } Yes, I could go for that ... I just don't see such a problem with the currently overloaded uses of strstrip. James -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Tue, 2009-11-03 at 12:04 -0700, Matthew Wilcox wrote: > static inline void strsrip_tail(char *str) > { > - char *x __used; > - x = strstrip(str); > + char *x = strstrip(str); > + BUG_ON(x != str); > } Please, no. I said didn't care about leading whitespace ... I didn't say there wasn't any. James -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
> static inline void strsrip_tail(char *str) > { > char *x __used; > x = strstrip(str); > } Bikeshed time but its cleaner to do static inline __must_check void strstrip(char *str) { return strim(str); } and make strim() the old strstrip function without the check requirement Alan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
2009/11/4 Alan Cox <alan@lxorguk.ukuu.org.uk>: >> static inline void strsrip_tail(char *str) >> { >> char *x __used; >> x = strstrip(str); >> } > > Bikeshed time but its cleaner to do > > static inline __must_check void strstrip(char *str) > { > return strim(str); > } > > and make strim() the old strstrip function without the check requirement Okey... [quick hack and compile check] done :) sorry for attached file. I'm under poor mail environment now.
Hi, I have several places in my code where the new __must_check of strstrip will introduce unnecessary dummy variables to avoid the warnings. Therefore I would like to have the suggested new strim() or strstip_tail() function. Any chance to have this upstream soon? Michael On Wed, 2009-11-04 at 04:58 +0900, KOSAKI Motohiro wrote: > 2009/11/4 Alan Cox <alan@lxorguk.ukuu.org.uk>: > >> static inline void strsrip_tail(char *str) > >> { > >> char *x __used; > >> x = strstrip(str); > >> } > > > > Bikeshed time but its cleaner to do > > > > static inline __must_check void strstrip(char *str) > > { > > return strim(str); > > } > > > > and make strim() the old strstrip function without the check requirement > > Okey... > > [quick hack and compile check] > > done :) > sorry for attached file. I'm under poor mail environment now. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Patch
diff --git a/include/linux/string.h b/include/linux/string.h index b850886..489019e 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -62,7 +62,7 @@ extern char * strnchr(const char *, size_t, int); #ifndef __HAVE_ARCH_STRRCHR extern char * strrchr(const char *,int); #endif -extern char * __must_check strstrip(char *); +extern char * strstrip(char *); #ifndef __HAVE_ARCH_STRSTR extern char * strstr(const char *,const char *); #endif
strstrip strips whitespace from the beginning and end of a string. I agree you have to take the returned pointer if you want to strip from the beginning. However, if you wish to keep the whitespace at the beginning and only wish strstrip to remove it from the end, then it's entirely legitimate to discard the returned pointer. This is what we have in drivers/scsi/ipr.c and the patch to make strstrip __must_check is now causing SCSI spurious warnings in that code. Signed-off-by: James Bottomley <James.Bottomley@suse.de> --- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/