2014-11-28 15:50:42

by Andy Shevchenko

Subject: [PATCH v1] sunrpc/cache: convert to use string_escape_str()

There is nice kernel helper to escape a given strings by provided rules. Let's
use it instead of custom approach.

Signed-off-by: Andy Shevchenko <[email protected]>
---
net/sunrpc/cache.c | 27 ++++++---------------------
1 file changed, 6 insertions(+), 21 deletions(-)

diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index 0663621..5cf60a4 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -20,6 +20,7 @@
#include <linux/list.h>
#include <linux/module.h>
#include <linux/ctype.h>
+#include <linux/string_helpers.h>
#include <asm/uaccess.h>
#include <linux/poll.h>
#include <linux/seq_file.h>
@@ -1067,32 +1068,16 @@ void qword_add(char **bpp, int *lp, char *str)
{
char *bp = *bpp;
int len = *lp;
- char c;
+ int ret;

if (len < 0) return;

- while ((c=*str++) && len)
- switch(c) {
- case ' ':
- case '\t':
- case '\n':
- case '\\':
- if (len >= 4) {
- *bp++ = '\\';
- *bp++ = '0' + ((c & 0300)>>6);
- *bp++ = '0' + ((c & 0070)>>3);
- *bp++ = '0' + ((c & 0007)>>0);
- }
- len -= 4;
- break;
- default:
- *bp++ = c;
- len--;
- }
- if (c || len <1) len = -1;
+ ret = string_escape_str(str, &bp, len, ESCAPE_OCTAL, "\\ \n\t");
+ if (ret < 0 || ret == len)
+ len = -1;
else {
*bp++ = ' ';
- len--;
+ len -= ret - 1;
}
*bpp = bp;
*lp = len;
--
2.1.3

2014-12-08 22:38:53

by J. Bruce Fields

Subject: Re: [PATCH v1] sunrpc/cache: convert to use string_escape_str()

On Fri, Nov 28, 2014 at 05:50:28PM +0200, Andy Shevchenko wrote:
> There is nice kernel helper to escape a given strings by provided rules. Let's
> use it instead of custom approach.

Looks good, but it broke nfsd.

After staring at the patch for a while:

> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> net/sunrpc/cache.c | 27 ++++++---------------------
> 1 file changed, 6 insertions(+), 21 deletions(-)
>
> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> index 0663621..5cf60a4 100644
> --- a/net/sunrpc/cache.c
> +++ b/net/sunrpc/cache.c
> @@ -20,6 +20,7 @@
> #include <linux/list.h>
> #include <linux/module.h>
> #include <linux/ctype.h>
> +#include <linux/string_helpers.h>
> #include <asm/uaccess.h>
> #include <linux/poll.h>
> #include <linux/seq_file.h>
> @@ -1067,32 +1068,16 @@ void qword_add(char **bpp, int *lp, char *str)
> {
> char *bp = *bpp;
> int len = *lp;
> - char c;
> + int ret;
>
> if (len < 0) return;
>
> - while ((c=*str++) && len)
> - switch(c) {
> - case ' ':
> - case '\t':
> - case '\n':
> - case '\\':
> - if (len >= 4) {
> - *bp++ = '\\';
> - *bp++ = '0' + ((c & 0300)>>6);
> - *bp++ = '0' + ((c & 0070)>>3);
> - *bp++ = '0' + ((c & 0007)>>0);
> - }
> - len -= 4;
> - break;
> - default:
> - *bp++ = c;
> - len--;
> - }
> - if (c || len <1) len = -1;
> + ret = string_escape_str(str, &bp, len, ESCAPE_OCTAL, "\\ \n\t");
> + if (ret < 0 || ret == len)
> + len = -1;
> else {
> *bp++ = ' ';
> - len--;
> + len -= ret - 1;

Looks like that should be ret + 1, not ret - 1. With that change,
things work.

Inclined to actually commit that as:

len -= ret;
*bp++ = ' ';
len--;

just to make the arithmetic really obvious.

--b.

> }
> *bpp = bp;
> *lp = len;
> --
> 2.1.3
>

2014-12-09 09:24:10

by Andy Shevchenko

Subject: Re: [PATCH v1] sunrpc/cache: convert to use string_escape_str()

On Mon, 2014-12-08 at 17:38 -0500, J. Bruce Fields wrote:
> On Fri, Nov 28, 2014 at 05:50:28PM +0200, Andy Shevchenko wrote:
> > There is nice kernel helper to escape a given strings by provided rules. Let's
> > use it instead of custom approach.
>
> Looks good, but it broke nfsd.
>
> After staring at the patch for a while:

> > Signed-off-by: Andy Shevchenko <[email protected]>
> > ---
> > net/sunrpc/cache.c | 27 ++++++---------------------
> > 1 file changed, 6 insertions(+), 21 deletions(-)
> >
> > diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> > index 0663621..5cf60a4 100644
> > --- a/net/sunrpc/cache.c
> > +++ b/net/sunrpc/cache.c
> > @@ -20,6 +20,7 @@
> > #include <linux/list.h>
> > #include <linux/module.h>
> > #include <linux/ctype.h>
> > +#include <linux/string_helpers.h>
> > #include <asm/uaccess.h>
> > #include <linux/poll.h>
> > #include <linux/seq_file.h>
> > @@ -1067,32 +1068,16 @@ void qword_add(char **bpp, int *lp, char *str)
> > {
> > char *bp = *bpp;
> > int len = *lp;
> > - char c;
> > + int ret;
> >
> > if (len < 0) return;
> >
> > - while ((c=*str++) && len)
> > - switch(c) {
> > - case ' ':
> > - case '\t':
> > - case '\n':
> > - case '\\':
> > - if (len >= 4) {
> > - *bp++ = '\\';
> > - *bp++ = '0' + ((c & 0300)>>6);
> > - *bp++ = '0' + ((c & 0070)>>3);
> > - *bp++ = '0' + ((c & 0007)>>0);
> > - }
> > - len -= 4;
> > - break;
> > - default:
> > - *bp++ = c;
> > - len--;
> > - }
> > - if (c || len <1) len = -1;
> > + ret = string_escape_str(str, &bp, len, ESCAPE_OCTAL, "\\ \n\t");
> > + if (ret < 0 || ret == len)
> > + len = -1;
> > else {
> > *bp++ = ' ';
> > - len--;
> > + len -= ret - 1;
>
> Looks like that should be ret + 1, not ret - 1. With that change,
> things work.
>
> Inclined to actually commit that as:
>
> len -= ret;
> *bp++ = ' ';
> len--;
>
> just to make the arithmetic really obvious.
>
> --b.

Good catch, thanks! It should decrement length indeed. In the form of -=
it must be + 1. Shall I resubmit patch? If so, can I include your tag
(Tested-by I suppose) ?

>
> > }
> > *bpp = bp;
> > *lp = len;
> > --
> > 2.1.3
> >


--
Andy Shevchenko <[email protected]>
Intel Finland Oy

2014-12-09 14:42:42

by J. Bruce Fields

Subject: Re: [PATCH v1] sunrpc/cache: convert to use string_escape_str()

On Tue, Dec 09, 2014 at 11:24:09AM +0200, Andy Shevchenko wrote:
> On Mon, 2014-12-08 at 17:38 -0500, J. Bruce Fields wrote:
> > On Fri, Nov 28, 2014 at 05:50:28PM +0200, Andy Shevchenko wrote:
> > > There is nice kernel helper to escape a given strings by provided rules. Let's
> > > use it instead of custom approach.
> >
> > Looks good, but it broke nfsd.
> >
> > After staring at the patch for a while:
>
> > > Signed-off-by: Andy Shevchenko <[email protected]>
> > > ---
> > > net/sunrpc/cache.c | 27 ++++++---------------------
> > > 1 file changed, 6 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> > > index 0663621..5cf60a4 100644
> > > --- a/net/sunrpc/cache.c
> > > +++ b/net/sunrpc/cache.c
> > > @@ -20,6 +20,7 @@
> > > #include <linux/list.h>
> > > #include <linux/module.h>
> > > #include <linux/ctype.h>
> > > +#include <linux/string_helpers.h>
> > > #include <asm/uaccess.h>
> > > #include <linux/poll.h>
> > > #include <linux/seq_file.h>
> > > @@ -1067,32 +1068,16 @@ void qword_add(char **bpp, int *lp, char *str)
> > > {
> > > char *bp = *bpp;
> > > int len = *lp;
> > > - char c;
> > > + int ret;
> > >
> > > if (len < 0) return;
> > >
> > > - while ((c=*str++) && len)
> > > - switch(c) {
> > > - case ' ':
> > > - case '\t':
> > > - case '\n':
> > > - case '\\':
> > > - if (len >= 4) {
> > > - *bp++ = '\\';
> > > - *bp++ = '0' + ((c & 0300)>>6);
> > > - *bp++ = '0' + ((c & 0070)>>3);
> > > - *bp++ = '0' + ((c & 0007)>>0);
> > > - }
> > > - len -= 4;
> > > - break;
> > > - default:
> > > - *bp++ = c;
> > > - len--;
> > > - }
> > > - if (c || len <1) len = -1;
> > > + ret = string_escape_str(str, &bp, len, ESCAPE_OCTAL, "\\ \n\t");
> > > + if (ret < 0 || ret == len)
> > > + len = -1;
> > > else {
> > > *bp++ = ' ';
> > > - len--;
> > > + len -= ret - 1;
> >
> > Looks like that should be ret + 1, not ret - 1. With that change,
> > things work.
> >
> > Inclined to actually commit that as:
> >
> > len -= ret;
> > *bp++ = ' ';
> > len--;
> >
> > just to make the arithmetic really obvious.
> >
> > --b.
>
> Good catch, thanks! It should decrement length indeed. In the form of -=
> it must be + 1. Shall I resubmit patch? If so, can I include your tag
> (Tested-by I suppose) ?

I'll fix it up, thanks.

--b.