[PATCH] strstrip incorrectly marked __must_check

[PATCH] strstrip incorrectly marked __must_check

[James Bottomley]

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>

---

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

Re: [PATCH] strstrip incorrectly marked __must_check

[Andrew Morton]

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);
}

Re: [PATCH] strstrip incorrectly marked __must_check

[Matthew Wilcox]

 static inline void strsrip_tail(char *str)
 {
-	char *x __used;
-	x = strstrip(str);
+	char *x = strstrip(str);
+	BUG_ON(x != str);
 }

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] strstrip incorrectly marked __must_check

[KOSAKI Motohiro]

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-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] strstrip incorrectly marked __must_check

[Alan Cox]

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.

Re: [PATCH] strstrip incorrectly marked __must_check

[James Bottomley]

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

Re: [PATCH] strstrip incorrectly marked __must_check

[James Bottomley]

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

Re: [PATCH] strstrip incorrectly marked __must_check

[Alan Cox]

> 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

Re: [PATCH] strstrip incorrectly marked __must_check

[KOSAKI Motohiro]

[-- Attachment #1: Type: text/plain, Size: 499 bytes --]

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.

[-- Attachment #2: 0001-lib-Introduce-strim.patch --]
[-- Type: application/octet-stream, Size: 3088 bytes --]

From 94a86424a882d52fcd11039d11fbd0363a31472f Mon Sep 17 00:00:00 2001
From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Date: Wed, 4 Nov 2009 04:47:23 +0900
Subject: [PATCH] lib: Introduce strim

Recently, We marked strstrip() as must_check. because it was frequently misused and
it should be checked. however, we found one exception. scsi/ipr.c intentionally ignore
return value of strstrip. because, it wish to keep the whitespace at the beginning.

Thus, we need to keep with and without checked whitespace trim function. This patch
makes strim new function and ipr.c use it.

Suggested-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 drivers/scsi/ipr.c     |    4 ++--
 include/linux/string.h |    9 ++++++++-
 lib/string.c           |    6 +++---
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index d40d5c7..6602fc4 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -1333,7 +1333,7 @@ static void ipr_log_enhanced_dual_ioa_error(struct ipr_ioa_cfg *ioa_cfg,
 
 	error = &hostrcb->hcam.u.error.u.type_17_error;
 	error->failure_reason[sizeof(error->failure_reason) - 1] = '\0';
-	strstrip(error->failure_reason);
+	strim(error->failure_reason);
 
 	ipr_hcam_err(hostrcb, "%s [PRC: %08X]\n", error->failure_reason,
 		     be32_to_cpu(hostrcb->hcam.u.error.prc));
@@ -1359,7 +1359,7 @@ static void ipr_log_dual_ioa_error(struct ipr_ioa_cfg *ioa_cfg,
 
 	error = &hostrcb->hcam.u.error.u.type_07_error;
 	error->failure_reason[sizeof(error->failure_reason) - 1] = '\0';
-	strstrip(error->failure_reason);
+	strim(error->failure_reason);
 
 	ipr_hcam_err(hostrcb, "%s [PRC: %08X]\n", error->failure_reason,
 		     be32_to_cpu(hostrcb->hcam.u.error.prc));
diff --git a/include/linux/string.h b/include/linux/string.h
index b850886..17a375e 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -62,7 +62,14 @@ 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 * strim(char *);
+
+static inline __must_check char* strstrip(char *str)
+{
+	return strim(str);
+}
+
 #ifndef __HAVE_ARCH_STRSTR
 extern char * strstr(const char *,const char *);
 #endif
diff --git a/lib/string.c b/lib/string.c
index b19b87a..28211c6 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -330,14 +330,14 @@ EXPORT_SYMBOL(strnchr);
 #endif
 
 /**
- * strstrip - Removes leading and trailing whitespace from @s.
+ * strim - Removes leading and trailing whitespace from @s.
  * @s: The string to be stripped.
  *
  * Note that the first trailing whitespace is replaced with a %NUL-terminator
  * in the given string @s. Returns a pointer to the first non-whitespace
  * character in @s.
  */
-char *strstrip(char *s)
+char *strim(char *s)
 {
 	size_t size;
 	char *end;
@@ -357,7 +357,7 @@ char *strstrip(char *s)
 
 	return s;
 }
-EXPORT_SYMBOL(strstrip);
+EXPORT_SYMBOL(strim);
 
 #ifndef __HAVE_ARCH_STRLEN
 /**
-- 
1.6.2.5

Re: [PATCH] strstrip incorrectly marked __must_check

[Michael Holzheu]

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.

Re: [PATCH] strstrip incorrectly marked __must_check

[KOSAKI Motohiro]

> 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?

strim() is in mmotm now. I expect it will merge mainline soon.

Thanks.


> 
> 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.
> 

Re: [PATCH] strstrip incorrectly marked __must_check

[Michael Holzheu]

On Tue, 2009-11-24 at 17:55 +0900, KOSAKI Motohiro wrote:
> > 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?
> 
> strim() is in mmotm now. I expect it will merge mainline soon.

That's really good news!

Thanks!

Michael