Re: fix format warning

2017-08-07 Thread Florian Obser
OK florian@

On Mon, Aug 07, 2017 at 11:53:13AM +0200, Mark Kettenis wrote:
> > Date: Mon, 7 Aug 2017 11:32:55 +0200 (CEST)
> > From: Mark Kettenis 
> > 
> > > Date: Mon, 7 Aug 2017 11:04:51 +0200 (CEST)
> > > From: Markus Hennecke 
> > > 
> > > On Thu, 3 Aug 2017, Mark Kettenis wrote:
> > > 
> > > > Currently clang ignores the "kprintf" format attribute.  I've got a
> > > > fix for that, but with that fix it complains about the code fixed in
> > > > the diff below.
> > > > 
> > > > Now our manual page says:
> > > > 
> > > >FORMAT OPTIONS
> > > >  The kernel functions don't support as many formatting specifiers 
> > > > as their
> > > >  user space counterparts.  In addition to the floating point 
> > > > formatting
> > > >  specifiers, the following integer type specifiers are not 
> > > > supported:
> > > > 
> > > >  %hhArgument of char type.  This format specifier is accepted 
> > > > by the
> > > > kernel but will be handled as %h.
> > > > 
> > > > So as far as the kernel is concerned, this change is a no-op.
> > > > 
> > > > ok?
> > > > 
> > > > P.S. While printing as a short is technically incorrect, it works
> > > >  because the arguments are unsigned and promoted to int.
> > > > 
> > > > 
> > > > Index: subr_disk.c
> > > > ===
> > > > RCS file: /cvs/src/sys/kern/subr_disk.c,v
> > > > retrieving revision 1.230
> > > > diff -u -p -r1.230 subr_disk.c
> > > > --- subr_disk.c 4 May 2017 22:47:27 -   1.230
> > > > +++ subr_disk.c 3 Aug 2017 17:38:32 -
> > > > @@ -1864,7 +1864,7 @@ duid_format(u_char *duid)
> > > > static char duid_str[17];
> > > >  
> > > > snprintf(duid_str, sizeof(duid_str),
> > > > -   "%02hx%02hx%02hx%02hx%02hx%02hx%02hx%02hx",
> > > > +   "%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx",
> > > > duid[0], duid[1], duid[2], duid[3],
> > > > duid[4], duid[5], duid[6], duid[7]);
> > > 
> > > This breaks kernel builds with gcc. It looks like gcc in base is too old 
> > > for handling %hhx. I tried to build a kernel for armv7 with rev 1.231 
> > > today.
> > 
> > Damn.  Thanks for spotting this.  It is fairly easy to add %hh support
> > to our gcc, but maybe we shouldn't.
> > 
> > So here is an alternative approach.  Just cast to u_short in
> > recognition that this is what the kernel actually supports.
> > 
> > ok?
> 
> Or as pirofti@ suggests, simply droppy the 'h' modifier altogether is
> safe as well as u_char gets promoted to int in a varargs function.
> And clang doesn't want about this one.
> 
> Any preference?
> 
> > Index: kern/subr_disk.c
> > ===
> > RCS file: /cvs/src/sys/kern/subr_disk.c,v
> > retrieving revision 1.231
> > diff -u -p -r1.231 subr_disk.c
> > --- kern/subr_disk.c6 Aug 2017 19:56:29 -   1.231
> > +++ kern/subr_disk.c7 Aug 2017 09:30:29 -
> > @@ -1864,9 +1864,10 @@ duid_format(u_char *duid)
> > static char duid_str[17];
> >  
> > snprintf(duid_str, sizeof(duid_str),
> > -   "%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx",
> > -   duid[0], duid[1], duid[2], duid[3],
> > -   duid[4], duid[5], duid[6], duid[7]);
> > +   "%02hx%02hx%02hx%02hx%02hx%02hx%02hx%02hx",
> > +   (u_short)duid[0], (u_short)duid[1], (u_short)duid[2],
> > +   (u_short)duid[3], (u_short)duid[4], (u_short)duid[5],
> > +   (u_short)duid[6], (u_short)duid[7]);
> >  
> > return (duid_str);
> >  }
> 
> Index: kern/subr_disk.c
> ===
> RCS file: /cvs/src/sys/kern/subr_disk.c,v
> retrieving revision 1.231
> diff -u -p -r1.231 subr_disk.c
> --- kern/subr_disk.c  6 Aug 2017 19:56:29 -   1.231
> +++ kern/subr_disk.c  7 Aug 2017 09:50:43 -
> @@ -1864,7 +1864,7 @@ duid_format(u_char *duid)
>   static char duid_str[17];
>  
>   snprintf(duid_str, sizeof(duid_str),
> - "%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx",
> + "%02x%02x%02x%02x%02x%02x%02x%02x",
>   duid[0], duid[1], duid[2], duid[3],
>   duid[4], duid[5], duid[6], duid[7]);
>  
> 

-- 
I'm not entirely sure you are real.



Re: fix format warning

2017-08-07 Thread Alexander Bluhm
On Mon, Aug 07, 2017 at 11:53:13AM +0200, Mark Kettenis wrote:
> Or as pirofti@ suggests, simply droppy the 'h' modifier altogether is
> safe as well as u_char gets promoted to int in a varargs function.
> And clang doesn't want about this one.
> 
> Any preference?

When I encountered this problem years ago, consensus was not to use
h in kernel.

OK bluhm@

> Index: kern/subr_disk.c
> ===
> RCS file: /cvs/src/sys/kern/subr_disk.c,v
> retrieving revision 1.231
> diff -u -p -r1.231 subr_disk.c
> --- kern/subr_disk.c  6 Aug 2017 19:56:29 -   1.231
> +++ kern/subr_disk.c  7 Aug 2017 09:50:43 -
> @@ -1864,7 +1864,7 @@ duid_format(u_char *duid)
>   static char duid_str[17];
>  
>   snprintf(duid_str, sizeof(duid_str),
> - "%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx",
> + "%02x%02x%02x%02x%02x%02x%02x%02x",
>   duid[0], duid[1], duid[2], duid[3],
>   duid[4], duid[5], duid[6], duid[7]);
>  



Re: fix format warning

2017-08-07 Thread Mark Kettenis
> Date: Mon, 7 Aug 2017 11:32:55 +0200 (CEST)
> From: Mark Kettenis 
> 
> > Date: Mon, 7 Aug 2017 11:04:51 +0200 (CEST)
> > From: Markus Hennecke 
> > 
> > On Thu, 3 Aug 2017, Mark Kettenis wrote:
> > 
> > > Currently clang ignores the "kprintf" format attribute.  I've got a
> > > fix for that, but with that fix it complains about the code fixed in
> > > the diff below.
> > > 
> > > Now our manual page says:
> > > 
> > >FORMAT OPTIONS
> > >  The kernel functions don't support as many formatting specifiers as 
> > > their
> > >  user space counterparts.  In addition to the floating point 
> > > formatting
> > >  specifiers, the following integer type specifiers are not supported:
> > > 
> > >  %hhArgument of char type.  This format specifier is accepted by 
> > > the
> > > kernel but will be handled as %h.
> > > 
> > > So as far as the kernel is concerned, this change is a no-op.
> > > 
> > > ok?
> > > 
> > > P.S. While printing as a short is technically incorrect, it works
> > >  because the arguments are unsigned and promoted to int.
> > > 
> > > 
> > > Index: subr_disk.c
> > > ===
> > > RCS file: /cvs/src/sys/kern/subr_disk.c,v
> > > retrieving revision 1.230
> > > diff -u -p -r1.230 subr_disk.c
> > > --- subr_disk.c   4 May 2017 22:47:27 -   1.230
> > > +++ subr_disk.c   3 Aug 2017 17:38:32 -
> > > @@ -1864,7 +1864,7 @@ duid_format(u_char *duid)
> > >   static char duid_str[17];
> > >  
> > >   snprintf(duid_str, sizeof(duid_str),
> > > - "%02hx%02hx%02hx%02hx%02hx%02hx%02hx%02hx",
> > > + "%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx",
> > >   duid[0], duid[1], duid[2], duid[3],
> > >   duid[4], duid[5], duid[6], duid[7]);
> > 
> > This breaks kernel builds with gcc. It looks like gcc in base is too old 
> > for handling %hhx. I tried to build a kernel for armv7 with rev 1.231 
> > today.
> 
> Damn.  Thanks for spotting this.  It is fairly easy to add %hh support
> to our gcc, but maybe we shouldn't.
> 
> So here is an alternative approach.  Just cast to u_short in
> recognition that this is what the kernel actually supports.
> 
> ok?

Or as pirofti@ suggests, simply droppy the 'h' modifier altogether is
safe as well as u_char gets promoted to int in a varargs function.
And clang doesn't want about this one.

Any preference?

> Index: kern/subr_disk.c
> ===
> RCS file: /cvs/src/sys/kern/subr_disk.c,v
> retrieving revision 1.231
> diff -u -p -r1.231 subr_disk.c
> --- kern/subr_disk.c  6 Aug 2017 19:56:29 -   1.231
> +++ kern/subr_disk.c  7 Aug 2017 09:30:29 -
> @@ -1864,9 +1864,10 @@ duid_format(u_char *duid)
>   static char duid_str[17];
>  
>   snprintf(duid_str, sizeof(duid_str),
> - "%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx",
> - duid[0], duid[1], duid[2], duid[3],
> - duid[4], duid[5], duid[6], duid[7]);
> + "%02hx%02hx%02hx%02hx%02hx%02hx%02hx%02hx",
> + (u_short)duid[0], (u_short)duid[1], (u_short)duid[2],
> + (u_short)duid[3], (u_short)duid[4], (u_short)duid[5],
> + (u_short)duid[6], (u_short)duid[7]);
>  
>   return (duid_str);
>  }

Index: kern/subr_disk.c
===
RCS file: /cvs/src/sys/kern/subr_disk.c,v
retrieving revision 1.231
diff -u -p -r1.231 subr_disk.c
--- kern/subr_disk.c6 Aug 2017 19:56:29 -   1.231
+++ kern/subr_disk.c7 Aug 2017 09:50:43 -
@@ -1864,7 +1864,7 @@ duid_format(u_char *duid)
static char duid_str[17];
 
snprintf(duid_str, sizeof(duid_str),
-   "%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx",
+   "%02x%02x%02x%02x%02x%02x%02x%02x",
duid[0], duid[1], duid[2], duid[3],
duid[4], duid[5], duid[6], duid[7]);
 



Re: fix format warning

2017-08-07 Thread Mark Kettenis
> Date: Mon, 7 Aug 2017 11:04:51 +0200 (CEST)
> From: Markus Hennecke 
> 
> On Thu, 3 Aug 2017, Mark Kettenis wrote:
> 
> > Currently clang ignores the "kprintf" format attribute.  I've got a
> > fix for that, but with that fix it complains about the code fixed in
> > the diff below.
> > 
> > Now our manual page says:
> > 
> >FORMAT OPTIONS
> >  The kernel functions don't support as many formatting specifiers as 
> > their
> >  user space counterparts.  In addition to the floating point formatting
> >  specifiers, the following integer type specifiers are not supported:
> > 
> >  %hhArgument of char type.  This format specifier is accepted by the
> > kernel but will be handled as %h.
> > 
> > So as far as the kernel is concerned, this change is a no-op.
> > 
> > ok?
> > 
> > P.S. While printing as a short is technically incorrect, it works
> >  because the arguments are unsigned and promoted to int.
> > 
> > 
> > Index: subr_disk.c
> > ===
> > RCS file: /cvs/src/sys/kern/subr_disk.c,v
> > retrieving revision 1.230
> > diff -u -p -r1.230 subr_disk.c
> > --- subr_disk.c 4 May 2017 22:47:27 -   1.230
> > +++ subr_disk.c 3 Aug 2017 17:38:32 -
> > @@ -1864,7 +1864,7 @@ duid_format(u_char *duid)
> > static char duid_str[17];
> >  
> > snprintf(duid_str, sizeof(duid_str),
> > -   "%02hx%02hx%02hx%02hx%02hx%02hx%02hx%02hx",
> > +   "%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx",
> > duid[0], duid[1], duid[2], duid[3],
> > duid[4], duid[5], duid[6], duid[7]);
> 
> This breaks kernel builds with gcc. It looks like gcc in base is too old 
> for handling %hhx. I tried to build a kernel for armv7 with rev 1.231 
> today.

Damn.  Thanks for spotting this.  It is fairly easy to add %hh support
to our gcc, but maybe we shouldn't.

So here is an alternative approach.  Just cast to u_short in
recognition that this is what the kernel actually supports.

ok?


Index: kern/subr_disk.c
===
RCS file: /cvs/src/sys/kern/subr_disk.c,v
retrieving revision 1.231
diff -u -p -r1.231 subr_disk.c
--- kern/subr_disk.c6 Aug 2017 19:56:29 -   1.231
+++ kern/subr_disk.c7 Aug 2017 09:30:29 -
@@ -1864,9 +1864,10 @@ duid_format(u_char *duid)
static char duid_str[17];
 
snprintf(duid_str, sizeof(duid_str),
-   "%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx",
-   duid[0], duid[1], duid[2], duid[3],
-   duid[4], duid[5], duid[6], duid[7]);
+   "%02hx%02hx%02hx%02hx%02hx%02hx%02hx%02hx",
+   (u_short)duid[0], (u_short)duid[1], (u_short)duid[2],
+   (u_short)duid[3], (u_short)duid[4], (u_short)duid[5],
+   (u_short)duid[6], (u_short)duid[7]);
 
return (duid_str);
 }