[PATCH] afs: use correct format characters

[PATCH] afs: use correct format characters

[Louis Taylor]

When compiling with -Wformat, clang warns:

fs/afs/flock.c:632:29: warning: format specifies type 'short' but the argument has type
      'unsigned char' [-Wformat]
        _leave(" = %d [%hd]", ret, fl->fl_type);
                       ~~~         ^~~~~~~~~~~

fl_type is declared as an unsigned char unconditionally in
include/linux/fs.h, so use the correct format characters.

Link: https://github.com/ClangBuiltLinux/linux/issues/378
Signed-off-by: Louis Taylor <louis@kragniz.eu>
---
 fs/afs/flock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/afs/flock.c b/fs/afs/flock.c
index 6a0174258382..be4c3f6a3178 100644
--- a/fs/afs/flock.c
+++ b/fs/afs/flock.c
@@ -629,7 +629,7 @@ static int afs_do_getlk(struct file *file, struct file_lock *fl)
 
 	ret = 0;
 error:
-	_leave(" = %d [%hd]", ret, fl->fl_type);
+	_leave(" = %d [%hhu]", ret, fl->fl_type);
 	return ret;
 }
 
-- 
2.21.0

Re: [PATCH] afs: use correct format characters

[Nick Desaulniers]

On Wed, Apr 10, 2019 at 3:03 PM Louis Taylor <louis@kragniz.eu> wrote:
>
> When compiling with -Wformat, clang warns:
>
> fs/afs/flock.c:632:29: warning: format specifies type 'short' but the argument has type
>       'unsigned char' [-Wformat]
>         _leave(" = %d [%hd]", ret, fl->fl_type);
>                        ~~~         ^~~~~~~~~~~
>
> fl_type is declared as an unsigned char unconditionally in
> include/linux/fs.h, so use the correct format characters.

Thanks for the patch, LGTM.  I had in my notes that there's a similar warning in
fs/afs/dir.c
line 138
format specifies type 'unsigned short' but the argument has type 'int'

can you verify if that's still the case, and if so, roll that change
into this one in a v2?

Otherwise, I'll post my reviewed by tag, and we can take just this one.

>
> Link: https://github.com/ClangBuiltLinux/linux/issues/378
> Signed-off-by: Louis Taylor <louis@kragniz.eu>
> ---
>  fs/afs/flock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/afs/flock.c b/fs/afs/flock.c
> index 6a0174258382..be4c3f6a3178 100644
> --- a/fs/afs/flock.c
> +++ b/fs/afs/flock.c
> @@ -629,7 +629,7 @@ static int afs_do_getlk(struct file *file, struct file_lock *fl)
>
>         ret = 0;
>  error:
> -       _leave(" = %d [%hd]", ret, fl->fl_type);
> +       _leave(" = %d [%hhu]", ret, fl->fl_type);
>         return ret;
>  }
>
> --
> 2.21.0
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To post to this group, send email to clang-built-linux@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20190410220301.2332-1-louis%40kragniz.eu.
> For more options, visit https://groups.google.com/d/optout.



-- 
Thanks,
~Nick Desaulniers

[PATCH v2] afs: use correct format characters

[Louis Taylor]

When compiling with -Wformat, clang warns:

fs/afs/flock.c:632:29: warning: format specifies type 'short' but the argument has type
      'unsigned char' [-Wformat]
        _leave(" = %d [%hd]", ret, fl->fl_type);
                       ~~~         ^~~~~~~~~~~

fs/afs/dir.c:138:11: warning: format specifies type 'unsigned short' but
the argument has type 'int' [-Wformat]
       ntohs(dbuf->blocks[tmp].hdr.magic));
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

fl_type is declared as an unsigned char unconditionally in
include/linux/fs.h, so use the correct format characters.

Link: https://github.com/ClangBuiltLinux/linux/issues/378
Signed-off-by: Louis Taylor <louis@kragniz.eu>
---
 fs/afs/dir.c   | 2 +-
 fs/afs/flock.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index 8a2562e3a316..4ceaec94e9c5 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -133,7 +133,7 @@ static bool afs_dir_check_page(struct afs_vnode *dvnode, struct page *page,
 	dbuf = kmap(page);
 	for (tmp = 0; tmp < qty; tmp++) {
 		if (dbuf->blocks[tmp].hdr.magic != AFS_DIR_MAGIC) {
-			printk("kAFS: %s(%lx): bad magic %d/%d is %04hx\n",
+			printk("kAFS: %s(%lx): bad magic %d/%d is %04x\n",
 			       __func__, dvnode->vfs_inode.i_ino, tmp, qty,
 			       ntohs(dbuf->blocks[tmp].hdr.magic));
 			trace_afs_dir_check_failed(dvnode, off, i_size);
diff --git a/fs/afs/flock.c b/fs/afs/flock.c
index 6a0174258382..be4c3f6a3178 100644
--- a/fs/afs/flock.c
+++ b/fs/afs/flock.c
@@ -629,7 +629,7 @@ static int afs_do_getlk(struct file *file, struct file_lock *fl)
 
 	ret = 0;
 error:
-	_leave(" = %d [%hd]", ret, fl->fl_type);
+	_leave(" = %d [%hhu]", ret, fl->fl_type);
 	return ret;
 }
 
-- 
2.21.0

Re: [PATCH v2] afs: use correct format characters

[Nick Desaulniers]

On Wed, Apr 10, 2019 at 3:41 PM Louis Taylor <louis@kragniz.eu> wrote:
>
> When compiling with -Wformat, clang warns:
>
> fs/afs/flock.c:632:29: warning: format specifies type 'short' but the argument has type
>       'unsigned char' [-Wformat]
>         _leave(" = %d [%hd]", ret, fl->fl_type);
>                        ~~~         ^~~~~~~~~~~
>
> fs/afs/dir.c:138:11: warning: format specifies type 'unsigned short' but
> the argument has type 'int' [-Wformat]
>        ntohs(dbuf->blocks[tmp].hdr.magic));
>        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> fl_type is declared as an unsigned char unconditionally in
> include/linux/fs.h, so use the correct format characters.

Thanks for the v2, probably should include a note about ntohs.  That
case in particular looks more complicated, due to the definition of
ntohs (which uses __swab16).

If you keep the previous flag of %04hx, but add an explicit cast to
u16, does the warning go away? If so, that might be a better fix.

- ntohs(dbuf->blocks[tmp].hdr.magic));
+ (u16)ntohs(dbuf->blocks[tmp].hdr.magic));

?

Particularly, I'm curious about the return type of GNU C statement
expressions, in the definition of __swab16 if __HAVE_BUILTIN_BSWAP16__
is not defined.

>
> Link: https://github.com/ClangBuiltLinux/linux/issues/378
> Signed-off-by: Louis Taylor <louis@kragniz.eu>
> ---
>  fs/afs/dir.c   | 2 +-
>  fs/afs/flock.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/afs/dir.c b/fs/afs/dir.c
> index 8a2562e3a316..4ceaec94e9c5 100644
> --- a/fs/afs/dir.c
> +++ b/fs/afs/dir.c
> @@ -133,7 +133,7 @@ static bool afs_dir_check_page(struct afs_vnode *dvnode, struct page *page,
>         dbuf = kmap(page);
>         for (tmp = 0; tmp < qty; tmp++) {
>                 if (dbuf->blocks[tmp].hdr.magic != AFS_DIR_MAGIC) {
> -                       printk("kAFS: %s(%lx): bad magic %d/%d is %04hx\n",
> +                       printk("kAFS: %s(%lx): bad magic %d/%d is %04x\n",
>                                __func__, dvnode->vfs_inode.i_ino, tmp, qty,
>                                ntohs(dbuf->blocks[tmp].hdr.magic));
>                         trace_afs_dir_check_failed(dvnode, off, i_size);
> diff --git a/fs/afs/flock.c b/fs/afs/flock.c
> index 6a0174258382..be4c3f6a3178 100644
> --- a/fs/afs/flock.c
> +++ b/fs/afs/flock.c
> @@ -629,7 +629,7 @@ static int afs_do_getlk(struct file *file, struct file_lock *fl)
>
>         ret = 0;
>  error:
> -       _leave(" = %d [%hd]", ret, fl->fl_type);
> +       _leave(" = %d [%hhu]", ret, fl->fl_type);
>         return ret;
>  }

-
Thanks,
~Nick Desaulniers

Re: [PATCH] afs: use correct format characters

[Joe Perches]

On Wed, 2019-04-10 at 23:03 +0100, Louis Taylor wrote:
> When compiling with -Wformat, clang warns:
> 
> fs/afs/flock.c:632:29: warning: format specifies type 'short' but the argument has type
>       'unsigned char' [-Wformat]
>         _leave(" = %d [%hd]", ret, fl->fl_type);

I really think this clang message should be ignored.

It's really unnecessary as every vararg argument smaller
than int size is already promoted to int.

This particular error is pedantic and has no effect
_at all_ on output or runtime.

If there was some actual mismatch between the signedness
of the argument and the format type, it could make sense.

ie:

	signed char foo = (signed char)-1;
	printk("mismatched %%d emitted as %%u: %u\n", foo);

where the output is a somewhat unexpected 4294967295

> fl_type is declared as an unsigned char unconditionally in
> include/linux/fs.h, so use the correct format characters.
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/378
> Signed-off-by: Louis Taylor <louis@kragniz.eu>
> ---
>  fs/afs/flock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/afs/flock.c b/fs/afs/flock.c
> index 6a0174258382..be4c3f6a3178 100644
> --- a/fs/afs/flock.c
> +++ b/fs/afs/flock.c
> @@ -629,7 +629,7 @@ static int afs_do_getlk(struct file *file, struct file_lock *fl)
>  
>  	ret = 0;
>  error:
> -	_leave(" = %d [%hd]", ret, fl->fl_type);
> +	_leave(" = %d [%hhu]", ret, fl->fl_type);
>  	return ret;
>  }
>  

Re: [PATCH] afs: use correct format characters

[Linus Torvalds]

On Wed, Apr 10, 2019 at 4:01 PM Joe Perches <joe@perches.com> wrote:
>
> I really think this clang message should be ignored.

Agreed.

> It's really unnecessary as every vararg argument smaller
> than int size is already promoted to int.

Exactly. It's a pointless warning, making for more complex code, and
making people remember esoteric printf format details that have no
reason for existing.

The "h" and "hh" things should never be used. The only reason for them
being used if if you have an "int", but you want to print it out as a
"char" (and honestly, that is a really bad reason, you'd be better off
just using a proper cast to make the code more obvious).

So if what you have a "char" (or unsigned char) you should always just
print it out as an "int", knowing that the compiler already did the
proper type conversion.

                             Linus

Re: [PATCH] afs: use correct format characters

[Nick Desaulniers]

On Thu, Apr 11, 2019 at 9:31 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Wed, Apr 10, 2019 at 4:01 PM Joe Perches <joe@perches.com> wrote:
> >
> > I really think this clang message should be ignored.
>
> Agreed.
>
> > It's really unnecessary as every vararg argument smaller
> > than int size is already promoted to int.
>
> Exactly. It's a pointless warning, making for more complex code, and
> making people remember esoteric printf format details that have no
> reason for existing.
>
> The "h" and "hh" things should never be used. The only reason for them
> being used if if you have an "int", but you want to print it out as a
> "char" (and honestly, that is a really bad reason, you'd be better off
> just using a proper cast to make the code more obvious).
>
> So if what you have a "char" (or unsigned char) you should always just
> print it out as an "int", knowing that the compiler already did the
> proper type conversion.
>
>                              Linus

https://bugs.llvm.org/show_bug.cgi?id=41467

I still think -Wformat helpful for catching completely nonsensical
format strings like printing a floating point type as an integral
type, or not having the correct number of arguments for the number of
format strings.  We'll take a look to see if we can differentiate
between those and these "integer widening" ones better.
-- 
Thanks,
~Nick Desaulniers

RE: [PATCH v2] afs: use correct format characters

[David Laight]

From: Nick Desaulniers
> Sent: 10 April 2019 23:52
> On Wed, Apr 10, 2019 at 3:41 PM Louis Taylor <louis@kragniz.eu> wrote:
> >
> > When compiling with -Wformat, clang warns:
> >
> > fs/afs/flock.c:632:29: warning: format specifies type 'short' but the argument has type
> >       'unsigned char' [-Wformat]
> >         _leave(" = %d [%hd]", ret, fl->fl_type);
> >                        ~~~         ^~~~~~~~~~~
> >
> > fs/afs/dir.c:138:11: warning: format specifies type 'unsigned short' but
> > the argument has type 'int' [-Wformat]
> >        ntohs(dbuf->blocks[tmp].hdr.magic));
> >        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > fl_type is declared as an unsigned char unconditionally in
> > include/linux/fs.h, so use the correct format characters.
> 
> Thanks for the v2, probably should include a note about ntohs.  That
> case in particular looks more complicated, due to the definition of
> ntohs (which uses __swab16).
> 
> If you keep the previous flag of %04hx, but add an explicit cast to
> u16, does the warning go away? If so, that might be a better fix.
> 
> - ntohs(dbuf->blocks[tmp].hdr.magic));
> + (u16)ntohs(dbuf->blocks[tmp].hdr.magic));

Or just append '+ 0u' forcing the type to 'unsigned int'.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)