[PATCH] FS-Cache: print hexadecimal value for special cookies type

[PATCH] FS-Cache: print hexadecimal value for special cookies type

[Jérémy Lefaure]

When building object-list.o, gcc 6 raises a warning on the sprintf call
in fscache_objlist_show:

  CC      fs/fscache/object-list.o
fs/fscache/object-list.c: In function ‘fscache_objlist_show’:
fs/fscache/object-list.c:265:19: warning: ‘sprintf’ may write a
terminating nul past the end of the destination [-Wformat-overflow=]
    sprintf(_type, "%02u", cookie->def->type);
                   ^~~~~~
fs/fscache/object-list.c:265:4: note: ‘sprintf’ output between 3 and 4
bytes into a destination of size 3
    sprintf(_type, "%02u", cookie->def->type);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Moreover, the documentation says that we should have an hex value for
special cookies (see Documentation/filesystems/caching/fscache.txt).

Printing hexadecimal value for special cookies fixes the overflow
warning and complies with the documentation.

Signed-off-by: Jérémy Lefaure <jeremy.lefaure@lse.epita.fr>
---
 fs/fscache/object-list.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/fscache/object-list.c b/fs/fscache/object-list.c
index 67f940892ef8..d51303124889 100644
--- a/fs/fscache/object-list.c
+++ b/fs/fscache/object-list.c
@@ -262,7 +262,7 @@ static int fscache_objlist_show(struct seq_file *m, void *v)
 			type = "DT";
 			break;
 		default:
-			sprintf(_type, "%02u", cookie->def->type);
+			sprintf(_type, "%02x", cookie->def->type);
 			type = _type;
 			break;
 		}
-- 
2.12.2

Re: [PATCH] FS-Cache: print hexadecimal value for special cookies type

[Andy Shevchenko]

On Wed, Apr 19, 2017 at 11:38 PM, Jérémy Lefaure
<jeremy.lefaure@lse.epita.fr> wrote:
> When building object-list.o, gcc 6 raises a warning on the sprintf call
> in fscache_objlist_show:
>
>   CC      fs/fscache/object-list.o
> fs/fscache/object-list.c: In function ‘fscache_objlist_show’:
> fs/fscache/object-list.c:265:19: warning: ‘sprintf’ may write a
> terminating nul past the end of the destination [-Wformat-overflow=]
>     sprintf(_type, "%02u", cookie->def->type);
>                    ^~~~~~
> fs/fscache/object-list.c:265:4: note: ‘sprintf’ output between 3 and 4
> bytes into a destination of size 3
>     sprintf(_type, "%02u", cookie->def->type);
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Moreover, the documentation says that we should have an hex value for
> special cookies (see Documentation/filesystems/caching/fscache.txt).
>
> Printing hexadecimal value for special cookies fixes the overflow
> warning and complies with the documentation.

If this is used by some user space tool the safest fix is to print in
BCD and fix documentation.

> Signed-off-by: Jérémy Lefaure <jeremy.lefaure@lse.epita.fr>
> ---
>  fs/fscache/object-list.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/fscache/object-list.c b/fs/fscache/object-list.c
> index 67f940892ef8..d51303124889 100644
> --- a/fs/fscache/object-list.c
> +++ b/fs/fscache/object-list.c
> @@ -262,7 +262,7 @@ static int fscache_objlist_show(struct seq_file *m, void *v)
>                         type = "DT";
>                         break;
>                 default:
> -                       sprintf(_type, "%02u", cookie->def->type);
> +                       sprintf(_type, "%02x", cookie->def->type);
>                         type = _type;
>                         break;
>                 }
> --
> 2.12.2
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] FS-Cache: print hexadecimal value for special cookies type

[Jérémy Lefaure]

On Fri, 21 Apr 2017 22:41:54 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Apr 19, 2017 at 11:38 PM, Jérémy Lefaure
> <jeremy.lefaure@lse.epita.fr> wrote:
> > When building object-list.o, gcc 6 raises a warning on the sprintf call
> > in fscache_objlist_show:
> >
> >   CC      fs/fscache/object-list.o
> > fs/fscache/object-list.c: In function ‘fscache_objlist_show’:
> > fs/fscache/object-list.c:265:19: warning: ‘sprintf’ may write a
> > terminating nul past the end of the destination [-Wformat-overflow=]
> >     sprintf(_type, "%02u", cookie->def->type);
> >                    ^~~~~~
> > fs/fscache/object-list.c:265:4: note: ‘sprintf’ output between 3 and 4
> > bytes into a destination of size 3
> >     sprintf(_type, "%02u", cookie->def->type);
> >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Moreover, the documentation says that we should have an hex value for
> > special cookies (see Documentation/filesystems/caching/fscache.txt).
> >
> > Printing hexadecimal value for special cookies fixes the overflow
> > warning and complies with the documentation.  
> 
> If this is used by some user space tool the safest fix is to print in
> BCD and fix documentation.
> 
Are you talking about the Binary Coded Decimal format ? I don't get why
it would be better. If we change the printed format, it would break
user space tool anyway, right ?

> > Signed-off-by: Jérémy Lefaure <jeremy.lefaure@lse.epita.fr>
> > ---
> >  fs/fscache/object-list.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/fscache/object-list.c b/fs/fscache/object-list.c
> > index 67f940892ef8..d51303124889 100644
> > --- a/fs/fscache/object-list.c
> > +++ b/fs/fscache/object-list.c
> > @@ -262,7 +262,7 @@ static int fscache_objlist_show(struct seq_file *m, void *v)
> >                         type = "DT";
> >                         break;
> >                 default:
> > -                       sprintf(_type, "%02u", cookie->def->type);
> > +                       sprintf(_type, "%02x", cookie->def->type);
> >                         type = _type;
> >                         break;
> >                 }
> > --
> > 2.12.2
> >  
> 
> 
> 

Re: [PATCH] FS-Cache: print hexadecimal value for special cookies type

[Andy Shevchenko]

On Sat, Apr 22, 2017 at 2:09 AM, Jérémy Lefaure
<jeremy.lefaure@lse.epita.fr> wrote:
> On Fri, 21 Apr 2017 22:41:54 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> On Wed, Apr 19, 2017 at 11:38 PM, Jérémy Lefaure
>> <jeremy.lefaure@lse.epita.fr> wrote:
>> > When building object-list.o, gcc 6 raises a warning on the sprintf call
>> > in fscache_objlist_show:
>> >
>> >   CC      fs/fscache/object-list.o
>> > fs/fscache/object-list.c: In function ‘fscache_objlist_show’:
>> > fs/fscache/object-list.c:265:19: warning: ‘sprintf’ may write a
>> > terminating nul past the end of the destination [-Wformat-overflow=]
>> >     sprintf(_type, "%02u", cookie->def->type);
>> >                    ^~~~~~
>> > fs/fscache/object-list.c:265:4: note: ‘sprintf’ output between 3 and 4
>> > bytes into a destination of size 3
>> >     sprintf(_type, "%02u", cookie->def->type);
>> >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> >
>> > Moreover, the documentation says that we should have an hex value for
>> > special cookies (see Documentation/filesystems/caching/fscache.txt).
>> >
>> > Printing hexadecimal value for special cookies fixes the overflow
>> > warning and complies with the documentation.
>>
>> If this is used by some user space tool the safest fix is to print in
>> BCD and fix documentation.
>>
> Are you talking about the Binary Coded Decimal format ?

Yes.

> I don't get why
> it would be better. If we change the printed format, it would break
> user space tool anyway, right ?

If userspace tool is bound to the format it might go crazy about hex
digits in the cookie.

Is that cookie important somehow? (for example, user space might
request something based on it?) It might be a case when you got let
say '75' and then ask for 117 (0x75), however real value is 75.

So, if it's okay in this, perhaps paragraph explaining side effects in
commit message would be good to have/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] FS-Cache: print hexadecimal value for special cookies type

[Jérémy Lefaure]

On Tue, 25 Apr 2017 00:47:41 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sat, Apr 22, 2017 at 2:09 AM, Jérémy Lefaure
> <jeremy.lefaure@lse.epita.fr> wrote:
> > On Fri, 21 Apr 2017 22:41:54 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> >> On Wed, Apr 19, 2017 at 11:38 PM, Jérémy Lefaure
> >> <jeremy.lefaure@lse.epita.fr> wrote:  
> >> > When building object-list.o, gcc 6 raises a warning on the sprintf call
> >> > in fscache_objlist_show:
> >> >
> >> >   CC      fs/fscache/object-list.o
> >> > fs/fscache/object-list.c: In function ‘fscache_objlist_show’:
> >> > fs/fscache/object-list.c:265:19: warning: ‘sprintf’ may write a
> >> > terminating nul past the end of the destination [-Wformat-overflow=]
> >> >     sprintf(_type, "%02u", cookie->def->type);
> >> >                    ^~~~~~
> >> > fs/fscache/object-list.c:265:4: note: ‘sprintf’ output between 3 and 4
> >> > bytes into a destination of size 3
> >> >     sprintf(_type, "%02u", cookie->def->type);
> >> >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> >
> >> > Moreover, the documentation says that we should have an hex value for
> >> > special cookies (see Documentation/filesystems/caching/fscache.txt).
> >> >
> >> > Printing hexadecimal value for special cookies fixes the overflow
> >> > warning and complies with the documentation.  
> >>
> >> If this is used by some user space tool the safest fix is to print in
> >> BCD and fix documentation.
> >>  
> > Are you talking about the Binary Coded Decimal format ?  
> 
> Yes.
> 
> > I don't get why
> > it would be better. If we change the printed format, it would break
> > user space tool anyway, right ?  
> 
> If userspace tool is bound to the format it might go crazy about hex
> digits in the cookie.
> 
> Is that cookie important somehow? (for example, user space might
> request something based on it?) It might be a case when you got let
> say '75' and then ask for 117 (0x75), however real value is 75.
> 
Ok I get it. But changing the format can break the userland tool
anyway, can't it ? For example if a userland tool expects the value 75,
it won't read it properpy in BCD.

> So, if it's okay in this, perhaps paragraph explaining side effects in
> commit message would be good to have/
> 
Maybe the best fix would be to increase the size of the _type buffer to
4 so the unsigned byte can fit in it. And change the documentation to
decimal value. What do you think about this solution ?

Thanks,

Re: [PATCH] FS-Cache: print hexadecimal value for special cookies type

[Andy Shevchenko]

On Wed, Apr 26, 2017 at 12:37 AM, Jérémy Lefaure
<jeremy.lefaure@lse.epita.fr> wrote:
> On Tue, 25 Apr 2017 00:47:41 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> Ok I get it. But changing the format can break the userland tool
> anyway, can't it ? For example if a userland tool expects the value 75,
> it won't read it properpy in BCD.

Maybe.

>> So, if it's okay in this, perhaps paragraph explaining side effects in
>> commit message would be good to have/
>>
> Maybe the best fix would be to increase the size of the _type buffer to
> 4 so the unsigned byte can fit in it. And change the documentation to
> decimal value. What do you think about this solution ?

Yes, it would work.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2] FS-Cache: fix buffer size for decimal value of special cookie type

[Jérémy Lefaure]

When building object-list.o, gcc (version 7) raises a warning on the
sprintf call in fscache_objlist_show:

  CC      fs/fscache/object-list.o
fs/fscache/object-list.c: In function ‘fscache_objlist_show’:
fs/fscache/object-list.c:265:19: warning: ‘sprintf’ may write a
terminating nul past the end of the destination [-Wformat-overflow=]
    sprintf(_type, "%02u", cookie->def->type);
                   ^~~~~~
fs/fscache/object-list.c:265:4: note: ‘sprintf’ output between 3 and 4
bytes into a destination of size 3
    sprintf(_type, "%02u", cookie->def->type);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Moreover, the documentation says that value for special cookies is a
hexadecimal value (see Documentation/filesystems/caching/fscache.txt).

Increasing the buffer size to use a decimal value fixes the problem
without any effect on userland tools. Also, this patch updates the
documentation.

Signed-off-by: Jérémy Lefaure <jeremy.lefaure@lse.epita.fr>
---
v2:
_ keep decimal value
_ fix _type buffer size
_ fix documentation

I didn't have the time to test this patch yet but everything should be fine.


 Documentation/filesystems/caching/fscache.txt |  2 +-
 fs/fscache/object-list.c                      | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/filesystems/caching/fscache.txt b/Documentation/filesystems/caching/fscache.txt
index 50f0a5757f48..9c3df61b3c16 100644
--- a/Documentation/filesystems/caching/fscache.txt
+++ b/Documentation/filesystems/caching/fscache.txt
@@ -374,7 +374,7 @@ and the second set of columns describe the object's cookie, if present:
 	COLUMN		DESCRIPTION
 	===============	=======================================================
 	NETFS_COOKIE_DEF Name of netfs cookie definition
-	TY		Cookie type (IX - index, DT - data, hex - special)
+	TY		Cookie type (IX - index, DT - data, decimal - special)
 	FL		Cookie flags
 	NETFS_DATA	Netfs private data stored in the cookie
 	OBJECT_KEY	Object key	} 1 column, with separating comma
diff --git a/fs/fscache/object-list.c b/fs/fscache/object-list.c
index 67f940892ef8..3452d7dbf1e1 100644
--- a/fs/fscache/object-list.c
+++ b/fs/fscache/object-list.c
@@ -169,7 +169,7 @@ static int fscache_objlist_show(struct seq_file *m, void *v)
 	struct fscache_object *obj = v;
 	struct fscache_cookie *cookie;
 	unsigned long config = data->config;
-	char _type[3], *type;
+	char _type[4], *type;
 	u8 *buf = data->buf, *p;
 
 	if ((unsigned long) v == 1) {
@@ -194,7 +194,7 @@ static int fscache_objlist_show(struct seq_file *m, void *v)
 	if ((unsigned long) v == 2) {
 		seq_puts(m, "======== ======== ==== ===== === === === == ====="
 			 " == == == ="
-			 " | ================ == == ================");
+			 " | ================ === == ================");
 		if (config & (FSCACHE_OBJLIST_CONFIG_KEY |
 			      FSCACHE_OBJLIST_CONFIG_AUX))
 			seq_puts(m, " ================");
@@ -256,13 +256,13 @@ static int fscache_objlist_show(struct seq_file *m, void *v)
 
 		switch (cookie->def->type) {
 		case 0:
-			type = "IX";
+			type = " IX";
 			break;
 		case 1:
-			type = "DT";
+			type = " DT";
 			break;
 		default:
-			sprintf(_type, "%02u", cookie->def->type);
+			sprintf(_type, "%03u", cookie->def->type);
 			type = _type;
 			break;
 		}
-- 
2.12.2

Re: [PATCH] FS-Cache: print hexadecimal value for special cookies type

[David Howells]

Jérémy Lefaure <jeremy.lefaure@lse.epita.fr> wrote:

> When building object-list.o, gcc 6 raises a warning on the sprintf call
> in fscache_objlist_show:
> 
>   CC      fs/fscache/object-list.o
> fs/fscache/object-list.c: In function ‘fscache_objlist_show’:
> fs/fscache/object-list.c:265:19: warning: ‘sprintf’ may write a
> terminating nul past the end of the destination [-Wformat-overflow=]
>     sprintf(_type, "%02u", cookie->def->type);
>                    ^~~~~~
> fs/fscache/object-list.c:265:4: note: ‘sprintf’ output between 3 and 4
> bytes into a destination of size 3
>     sprintf(_type, "%02u", cookie->def->type);
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Moreover, the documentation says that we should have an hex value for
> special cookies (see Documentation/filesystems/caching/fscache.txt).
> 
> Printing hexadecimal value for special cookies fixes the overflow
> warning and complies with the documentation.

Fine by me.  We don't actually handle special type cookies at the moment, so
you're not going to see anything other than DT or IX for now anyway.

I'll push this in the next merge window if that's okay with you.

David

Re: [PATCH] FS-Cache: print hexadecimal value for special cookies type

[Jérémy Lefaure]

On Thu, 27 Apr 2017 16:03:45 +0100
David Howells <dhowells@redhat.com> wrote:

> Jérémy Lefaure <jeremy.lefaure@lse.epita.fr> wrote:
> 
> > When building object-list.o, gcc 6 raises a warning on the sprintf call
> > in fscache_objlist_show:
> > 
> >   CC      fs/fscache/object-list.o
> > fs/fscache/object-list.c: In function ‘fscache_objlist_show’:
> > fs/fscache/object-list.c:265:19: warning: ‘sprintf’ may write a
> > terminating nul past the end of the destination [-Wformat-overflow=]
> >     sprintf(_type, "%02u", cookie->def->type);
> >                    ^~~~~~
> > fs/fscache/object-list.c:265:4: note: ‘sprintf’ output between 3 and 4
> > bytes into a destination of size 3
> >     sprintf(_type, "%02u", cookie->def->type);
> >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > Moreover, the documentation says that we should have an hex value for
> > special cookies (see Documentation/filesystems/caching/fscache.txt).
> > 
> > Printing hexadecimal value for special cookies fixes the overflow
> > warning and complies with the documentation.  
> 
> Fine by me.  We don't actually handle special type cookies at the moment, so
> you're not going to see anything other than DT or IX for now anyway.
> 
> I'll push this in the next merge window if that's okay with you.
> 
Did you see the v2 of my patch (in which I keep the decimal value but
fix the buffer size) ? If special type cookies aren't handled, I guess
that this v1 is better, isn't it ?

Re: [PATCH] FS-Cache: print hexadecimal value for special cookies type

[David Howells]

Jérémy Lefaure <jeremy.lefaure@lse.epita.fr> wrote:

> Did you see the v2 of my patch (in which I keep the decimal value but
> fix the buffer size) ?

I did.

> If special type cookies aren't handled, I guess that this v1 is better,
> isn't it ?

Yes.  v1 is better.  The docs are right.

David

Re: [PATCH] FS-Cache: print hexadecimal value for special cookies type

[Jérémy Lefaure]

On Thu, 27 Apr 2017 16:03:45 +0100
David Howells <dhowells@redhat.com> wrote:

> Jérémy Lefaure <jeremy.lefaure@lse.epita.fr> wrote:
> 
> > When building object-list.o, gcc 6 raises a warning on the sprintf call
> > in fscache_objlist_show:
> > 
> >   CC      fs/fscache/object-list.o
> > fs/fscache/object-list.c: In function ‘fscache_objlist_show’:
> > fs/fscache/object-list.c:265:19: warning: ‘sprintf’ may write a
> > terminating nul past the end of the destination [-Wformat-overflow=]
> >     sprintf(_type, "%02u", cookie->def->type);
> >                    ^~~~~~
> > fs/fscache/object-list.c:265:4: note: ‘sprintf’ output between 3 and 4
> > bytes into a destination of size 3
> >     sprintf(_type, "%02u", cookie->def->type);
> >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > Moreover, the documentation says that we should have an hex value for
> > special cookies (see Documentation/filesystems/caching/fscache.txt).
> > 
> > Printing hexadecimal value for special cookies fixes the overflow
> > warning and complies with the documentation.  
> 
> Fine by me.  We don't actually handle special type cookies at the moment, so
> you're not going to see anything other than DT or IX for now anyway.
> 
> I'll push this in the next merge window if that's okay with you.
> 

Hi David,
I didn't clearly say that I'm okay about pushing this patch in my last
answer. I am okay with that.

I don't see this patch in v4.12-rc2. Is there an issue with this patch
or are you waiting for my answer ?

Thanks,
Jérémy