Re: svn commit: r310171 - head/sys/sys

[Bruce Evans]

On Mon, 19 Dec 2016, Ravi Pokala wrote:


-Original Message-

From: <owner-src-committ...@freebsd.org> on behalf of Ian Lepore 
<i...@freebsd.org>
Date: 2016-12-19, Monday at 11:20
To: Warner Losh <i...@bsdimp.com>, Ravi Pokala <rpok...@mac.com>
Cc: Sepherosa Ziehau <sepher...@gmail.com>, Dimitry Andric <d...@freebsd.org>, src-committers 
<src-committ...@freebsd.org>, "svn-src-...@freebsd.org" <svn-src-...@freebsd.org>, 
"svn-src-head@freebsd.org" <svn-src-head@freebsd.org>
Subject: Re: svn commit: r310171 - head/sys/sys

On Mon, 2016-12-19 at 11:58 -0700, Warner Losh wrote:


...

Are there other precedence for avoiding the SCN macros in the tree as
well, or is this new art?


There was another commit recently the fixed the same kind of scanf
error by making the variable fit the scanf type (changing uint64_t to
an explicit long long unsigned, iirc).  I don't know if that alone
counts as a precedent, but IMO it's a more palatible fix than the
SCN/PRI ugliness.


That was apparently a bug, not a fix.  Someone said that the variable
needs to have type precisely uint64_t.
  (Extensive use of fixed-width types in FreeBSD is another bug.  It
  asks for fixed width at all cost.  Using the "fast" or "least" types
  would as for time or space efficiency instead.  Or just use basic
  types whenever possible for simplicity.  Fixed-width types should
  only be used on ABI boundaries.)
I think there is an ABI boundary near the other commit.  Some hardware
wants uint64_t, and we should use uint_fast64_t or uint_least64_t, but
we used uint64_t.  These uses require translation at the ABI boundary,
since the fast and least types may be larger than the fixed-width types.
  (In more complicated cases, the relevant fixed-width type does't exist.
  IIRC, POSIX requires only fixed width types of width 8, 16 and 32 to
  exist, since old badly designed ABIs like inet_addr() require a fixed
  width, but newer ABIs don't repeat the mistake for width 64.  The C
  standard requires the "fast" and "least" types for widths 8, 16, 32 and
  64 to exist.  This is possible since they can all be [unsigned] long
  long or [u]intmax_t although these might be very far from being either
  fast or least.  All fixed-width types are optional in C.  So to support
  bad software ABIs and some hardware ABIs using portable code, you have
  to read write the data using some basic type (perhaps unsigned char)
  and convert it to a "fast" or "least" type (or maybe [u]intmax_t).
We changed to using the unsigned long long abomination.  This is basically
a bad spelling of uintmax_t.  All uint_fast64_t, uint_least64_t, unsigned
long long and uintmax_t are at least as large asthe ABI type uint64_t, so
they can be used to represent the ABI values, but all need translation at
ABI booundaries since they may be strictly larger.  The translation may
be as simple as assignment, depending on how the code is written.  Since
the code wasn't written with this in mind, it might have type puns instead.
Or the compiler might just warn about all implicit conversions that might
lose bits.


With all apologies to Churchill, SCN/PRI are the worst way to address this in a 
machine-independent way, except for all the other ways that have been tried 
from time to time. :-P


No, there are much better ways.  For PRI*, just cast to [u]intmax_t.  The
only thing wrong with this is that it lots wastes space and time when
[u]intmax_t is very wide.  We don't have this problem yet since [u]intmax_t
is only 64 bits.  That would be very wide on 8-bit systems but is merely
wide on 32-bit systems and not wide on 63-bit systems.  PRI* is not even
available for an average typedef like uid_t.  It is available for all
"fast" and "least" typedefs.

For SCN*, first don't use *scanf().  Use APIs that can actually handle errors,
like strtol().  If you have to maintain bad code that uses *scanf(), then
convert to uintmax_t.  The conversions are not as short as casting for
*printf().  They are much like what is needed at ABI boundaries:

uintmax_t tmp;
uint64_t val;

/*
 * Good code does tmp = strtoumax() here.  Then handle errors
 * reported by strtoumax().  Then check that the result is
 * representable in uint64_t.  Then handle errors from that.
 *
 * Bad code did *sscanf(str, "%llx", ) with no error checking
 * or handling of course.  The type didn't't match the format in
 * general, and the behaviour was undefined on overflow.
 */
sscanf(str, "%jx", ); /* keep null error handling */
val = tmp;  /* preserve undefined on overflow */
/*
 * Overflow in sscanf() is less likely than before (uintmax_t
 * might be larger).  Then overflow occurs much like before on
 * a

Re: svn commit: r310171 - head/sys/sys

[Sepherosa Ziehau]

On Tue, Dec 20, 2016 at 4:37 AM, Ravi Pokala <rpok...@mac.com> wrote:
> -Original Message-
>> From: <owner-src-committ...@freebsd.org> on behalf of Ian Lepore 
>> <i...@freebsd.org>
>> Date: 2016-12-19, Monday at 11:20
>> To: Warner Losh <i...@bsdimp.com>, Ravi Pokala <rpok...@mac.com>
>> Cc: Sepherosa Ziehau <sepher...@gmail.com>, Dimitry Andric 
>> <d...@freebsd.org>, src-committers <src-committ...@freebsd.org>, 
>> "svn-src-...@freebsd.org" <svn-src-...@freebsd.org>, 
>> "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>
>> Subject: Re: svn commit: r310171 - head/sys/sys
>>
>> On Mon, 2016-12-19 at 11:58 -0700, Warner Losh wrote:
>>>
>>> ...
>>>
>>> Are there other precedence for avoiding the SCN macros in the tree as
>>> well, or is this new art?
>>>
>>> Warner
>>
>> There was another commit recently the fixed the same kind of scanf
>> error by making the variable fit the scanf type (changing uint64_t to
>> an explicit long long unsigned, iirc).  I don't know if that alone
>> counts as a precedent, but IMO it's a more palatible fix than the
>> SCN/PRI ugliness.
>
> With all apologies to Churchill, SCN/PRI are the worst way to address this in 
> a machine-independent way, except for all the other ways that have been tried 
> from time to time. :-P
>

If no objection comes, I'd commit the posted patch as it is tomorrow in my time.

Thanks,
sephe

-- 
Tomorrow Will Never Die
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r310171 - head/sys/sys

[Ravi Pokala]

-Original Message-
> From: <owner-src-committ...@freebsd.org> on behalf of Ian Lepore 
> <i...@freebsd.org>
> Date: 2016-12-19, Monday at 11:20
> To: Warner Losh <i...@bsdimp.com>, Ravi Pokala <rpok...@mac.com>
> Cc: Sepherosa Ziehau <sepher...@gmail.com>, Dimitry Andric 
> <d...@freebsd.org>, src-committers <src-committ...@freebsd.org>, 
> "svn-src-...@freebsd.org" <svn-src-...@freebsd.org>, 
> "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>
> Subject: Re: svn commit: r310171 - head/sys/sys
> 
> On Mon, 2016-12-19 at 11:58 -0700, Warner Losh wrote:
>>
>> ...
>> 
>> Are there other precedence for avoiding the SCN macros in the tree as
>> well, or is this new art?
>> 
>> Warner
> 
> There was another commit recently the fixed the same kind of scanf
> error by making the variable fit the scanf type (changing uint64_t to
> an explicit long long unsigned, iirc).  I don't know if that alone
> counts as a precedent, but IMO it's a more palatible fix than the
> SCN/PRI ugliness.

With all apologies to Churchill, SCN/PRI are the worst way to address this in a 
machine-independent way, except for all the other ways that have been tried 
from time to time. :-P

-Ravi (rpokala@)

> -- Ian



___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r310171 - head/sys/sys

[Ian Lepore]

On Mon, 2016-12-19 at 11:58 -0700, Warner Losh wrote:
> On Mon, Dec 19, 2016 at 1:39 AM, Ravi Pokala <rpok...@mac.com> wrote:
> > 
> > -Original Message-
> > > 
> > > From: <owner-src-committ...@freebsd.org> on behalf of Sepherosa
> > > Ziehau <sepher...@gmail.com>
> > > Date: 2016-12-18, Sunday at 23:02
> > > To: Dimitry Andric <d...@freebsd.org>
> > > Cc: <src-committ...@freebsd.org>, <svn-src-...@freebsd.org>,  > > -src-h...@freebsd.org>
> > > Subject: Re: svn commit: r310171 - head/sys/sys
> > > 
> > > The following patch unbreaks the LINT builds on amd64 for me
> > > after this commit:
> > > https://people.freebsd.org/~sephe/geom_sscanf.diff
> > Wouldn't it be better to use the SCN macros?
> Are there other precedence for avoiding the SCN macros in the tree as
> well, or is this new art?
> 
> Warner

There was another commit recently the fixed the same kind of scanf
error by making the variable fit the scanf type (changing uint64_t to
an explicit long long unsigned, iirc).  I don't know if that alone
counts as a precedent, but IMO it's a more palatible fix than the
SCN/PRI ugliness.

-- Ian

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r310171 - head/sys/sys

[Dimitry Andric]

On 19 Dec 2016, at 19:58, Warner Losh <i...@bsdimp.com> wrote:
> 
> On Mon, Dec 19, 2016 at 1:39 AM, Ravi Pokala <rpok...@mac.com> wrote:
>> -Original Message-
>>> From: <owner-src-committ...@freebsd.org> on behalf of Sepherosa Ziehau 
>>> <sepher...@gmail.com>
>>> Date: 2016-12-18, Sunday at 23:02
>>> To: Dimitry Andric <d...@freebsd.org>
>>> Cc: <src-committ...@freebsd.org>, <svn-src-...@freebsd.org>, 
>>> <svn-src-head@freebsd.org>
>>> Subject: Re: svn commit: r310171 - head/sys/sys
>>> 
>>> The following patch unbreaks the LINT builds on amd64 for me after this 
>>> commit:
>>> https://people.freebsd.org/~sephe/geom_sscanf.diff
>> 
>> Wouldn't it be better to use the SCN macros?
> 
> Are there other precedence for avoiding the SCN macros in the tree as
> well, or is this new art?

I personally don't have anything against using the PRI or SCN macros,
but traditionally there has been some backlash against it, if I recall
correctly.  It also requires including either  or
, depending on circumstance or preference.

-Dimitry



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: svn commit: r310171 - head/sys/sys

[Warner Losh]

On Mon, Dec 19, 2016 at 1:39 AM, Ravi Pokala <rpok...@mac.com> wrote:
> -Original Message-
>> From: <owner-src-committ...@freebsd.org> on behalf of Sepherosa Ziehau 
>> <sepher...@gmail.com>
>> Date: 2016-12-18, Sunday at 23:02
>> To: Dimitry Andric <d...@freebsd.org>
>> Cc: <src-committ...@freebsd.org>, <svn-src-...@freebsd.org>, 
>> <svn-src-head@freebsd.org>
>> Subject: Re: svn commit: r310171 - head/sys/sys
>>
>> The following patch unbreaks the LINT builds on amd64 for me after this 
>> commit:
>> https://people.freebsd.org/~sephe/geom_sscanf.diff
>
> Wouldn't it be better to use the SCN macros?

Are there other precedence for avoiding the SCN macros in the tree as
well, or is this new art?

Warner

>> Thanks,
>> sephe
>>
>>
>> On Sat, Dec 17, 2016 at 3:49 AM, Dimitry Andric <d...@freebsd.org> wrote:
>>> Author: dim
>>> Date: Fri Dec 16 19:49:22 2016
>>> New Revision: 310171
>>> URL: https://svnweb.freebsd.org/changeset/base/310171
>>>
>>> Log:
>>>   Add __scanflike attributes to the kernel's sscanf() and vsscanf()
>>>   declarations.  This should help to catch future mismatches between
>>>   format strings and arguments.
>>>
>>>   MFC after:1 week
>>>
>>> Modified:
>>>   head/sys/sys/systm.h
>>>
>>> Modified: head/sys/sys/systm.h
>>> ==
>>> --- head/sys/sys/systm.hFri Dec 16 19:09:57 2016(r310170)
>>> +++ head/sys/sys/systm.hFri Dec 16 19:49:22 2016(r310171)
>>> @@ -227,8 +227,8 @@ int vsnprintf(char *, size_t, const char
>>>  intvsnrprintf(char *, size_t, int, const char *, __va_list) 
>>> __printflike(4, 0);
>>>  intvsprintf(char *buf, const char *, __va_list) __printflike(2, 0);
>>>  intttyprintf(struct tty *, const char *, ...) __printflike(2, 3);
>>> -intsscanf(const char *, char const *, ...) __nonnull(1) __nonnull(2);
>>> -intvsscanf(const char *, char const *, __va_list) __nonnull(1) 
>>> __nonnull(2);
>>> +intsscanf(const char *, char const *, ...) __nonnull(1) __nonnull(2) 
>>> __scanflike(2, 3);
>>> +intvsscanf(const char *, char const *, __va_list) __nonnull(1) 
>>> __nonnull(2) __scanflike(2, 0);
>>>  long   strtol(const char *, char **, int) __nonnull(1);
>>>  u_long strtoul(const char *, char **, int) __nonnull(1);
>>>  quad_t strtoq(const char *, char **, int) __nonnull(1);
>>> ___
>>> svn-src-...@freebsd.org mailing list
>>> https://lists.freebsd.org/mailman/listinfo/svn-src-all
>>> To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
>>
>> --
>> Tomorrow Will Never Die
>
>
>
>
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r310171 - head/sys/sys

[Ravi Pokala]

-Original Message-
> From: <owner-src-committ...@freebsd.org> on behalf of Sepherosa Ziehau 
> <sepher...@gmail.com>
> Date: 2016-12-18, Sunday at 23:02
> To: Dimitry Andric <d...@freebsd.org>
> Cc: <src-committ...@freebsd.org>, <svn-src-...@freebsd.org>, 
> <svn-src-head@freebsd.org>
> Subject: Re: svn commit: r310171 - head/sys/sys
> 
> The following patch unbreaks the LINT builds on amd64 for me after this 
> commit:
> https://people.freebsd.org/~sephe/geom_sscanf.diff

Wouldn't it be better to use the SCN macros?

-Ravi (rpokala@)

> Please review it.
> 
> Thanks,
> sephe
> 
> 
> On Sat, Dec 17, 2016 at 3:49 AM, Dimitry Andric <d...@freebsd.org> wrote:
>> Author: dim
>> Date: Fri Dec 16 19:49:22 2016
>> New Revision: 310171
>> URL: https://svnweb.freebsd.org/changeset/base/310171
>>
>> Log:
>>   Add __scanflike attributes to the kernel's sscanf() and vsscanf()
>>   declarations.  This should help to catch future mismatches between
>>   format strings and arguments.
>>
>>   MFC after:1 week
>>
>> Modified:
>>   head/sys/sys/systm.h
>>
>> Modified: head/sys/sys/systm.h
>> ==
>> --- head/sys/sys/systm.hFri Dec 16 19:09:57 2016(r310170)
>> +++ head/sys/sys/systm.hFri Dec 16 19:49:22 2016(r310171)
>> @@ -227,8 +227,8 @@ int vsnprintf(char *, size_t, const char
>>  intvsnrprintf(char *, size_t, int, const char *, __va_list) 
>> __printflike(4, 0);
>>  intvsprintf(char *buf, const char *, __va_list) __printflike(2, 0);
>>  intttyprintf(struct tty *, const char *, ...) __printflike(2, 3);
>> -intsscanf(const char *, char const *, ...) __nonnull(1) __nonnull(2);
>> -intvsscanf(const char *, char const *, __va_list) __nonnull(1) 
>> __nonnull(2);
>> +intsscanf(const char *, char const *, ...) __nonnull(1) __nonnull(2) 
>> __scanflike(2, 3);
>> +intvsscanf(const char *, char const *, __va_list) __nonnull(1) 
>> __nonnull(2) __scanflike(2, 0);
>>  long   strtol(const char *, char **, int) __nonnull(1);
>>  u_long strtoul(const char *, char **, int) __nonnull(1);
>>  quad_t strtoq(const char *, char **, int) __nonnull(1);
>> ___
>> svn-src-...@freebsd.org mailing list
>> https://lists.freebsd.org/mailman/listinfo/svn-src-all
>> To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
> 
> -- 
> Tomorrow Will Never Die



___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r310171 - head/sys/sys

[Sepherosa Ziehau]

The following patch unbreaks the LINT builds on amd64 for me after this commit:
https://people.freebsd.org/~sephe/geom_sscanf.diff

Please review it.

Thanks,
sephe


On Sat, Dec 17, 2016 at 3:49 AM, Dimitry Andric  wrote:
> Author: dim
> Date: Fri Dec 16 19:49:22 2016
> New Revision: 310171
> URL: https://svnweb.freebsd.org/changeset/base/310171
>
> Log:
>   Add __scanflike attributes to the kernel's sscanf() and vsscanf()
>   declarations.  This should help to catch future mismatches between
>   format strings and arguments.
>
>   MFC after:1 week
>
> Modified:
>   head/sys/sys/systm.h
>
> Modified: head/sys/sys/systm.h
> ==
> --- head/sys/sys/systm.hFri Dec 16 19:09:57 2016(r310170)
> +++ head/sys/sys/systm.hFri Dec 16 19:49:22 2016(r310171)
> @@ -227,8 +227,8 @@ int vsnprintf(char *, size_t, const char
>  intvsnrprintf(char *, size_t, int, const char *, __va_list) 
> __printflike(4, 0);
>  intvsprintf(char *buf, const char *, __va_list) __printflike(2, 0);
>  intttyprintf(struct tty *, const char *, ...) __printflike(2, 3);
> -intsscanf(const char *, char const *, ...) __nonnull(1) __nonnull(2);
> -intvsscanf(const char *, char const *, __va_list) __nonnull(1) 
> __nonnull(2);
> +intsscanf(const char *, char const *, ...) __nonnull(1) __nonnull(2) 
> __scanflike(2, 3);
> +intvsscanf(const char *, char const *, __va_list) __nonnull(1) 
> __nonnull(2) __scanflike(2, 0);
>  long   strtol(const char *, char **, int) __nonnull(1);
>  u_long strtoul(const char *, char **, int) __nonnull(1);
>  quad_t strtoq(const char *, char **, int) __nonnull(1);
> ___
> svn-src-...@freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/svn-src-all
> To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"



-- 
Tomorrow Will Never Die
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r310171 - head/sys/sys

[Conrad Meyer]

Thanks!

On Fri, Dec 16, 2016 at 11:49 AM, Dimitry Andric  wrote:
> Author: dim
> Date: Fri Dec 16 19:49:22 2016
> New Revision: 310171
> URL: https://svnweb.freebsd.org/changeset/base/310171
>
> Log:
>   Add __scanflike attributes to the kernel's sscanf() and vsscanf()
>   declarations.  This should help to catch future mismatches between
>   format strings and arguments.
>
>   MFC after:1 week
>
> Modified:
>   head/sys/sys/systm.h
>
> Modified: head/sys/sys/systm.h
> ==
> --- head/sys/sys/systm.hFri Dec 16 19:09:57 2016(r310170)
> +++ head/sys/sys/systm.hFri Dec 16 19:49:22 2016(r310171)
> @@ -227,8 +227,8 @@ int vsnprintf(char *, size_t, const char
>  intvsnrprintf(char *, size_t, int, const char *, __va_list) 
> __printflike(4, 0);
>  intvsprintf(char *buf, const char *, __va_list) __printflike(2, 0);
>  intttyprintf(struct tty *, const char *, ...) __printflike(2, 3);
> -intsscanf(const char *, char const *, ...) __nonnull(1) __nonnull(2);
> -intvsscanf(const char *, char const *, __va_list) __nonnull(1) 
> __nonnull(2);
> +intsscanf(const char *, char const *, ...) __nonnull(1) __nonnull(2) 
> __scanflike(2, 3);
> +intvsscanf(const char *, char const *, __va_list) __nonnull(1) 
> __nonnull(2) __scanflike(2, 0);
>  long   strtol(const char *, char **, int) __nonnull(1);
>  u_long strtoul(const char *, char **, int) __nonnull(1);
>  quad_t strtoq(const char *, char **, int) __nonnull(1);
>
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

svn commit: r310171 - head/sys/sys

[Dimitry Andric]

Author: dim
Date: Fri Dec 16 19:49:22 2016
New Revision: 310171
URL: https://svnweb.freebsd.org/changeset/base/310171

Log:
  Add __scanflike attributes to the kernel's sscanf() and vsscanf()
  declarations.  This should help to catch future mismatches between
  format strings and arguments.
  
  MFC after:1 week

Modified:
  head/sys/sys/systm.h

Modified: head/sys/sys/systm.h
==
--- head/sys/sys/systm.hFri Dec 16 19:09:57 2016(r310170)
+++ head/sys/sys/systm.hFri Dec 16 19:49:22 2016(r310171)
@@ -227,8 +227,8 @@ int vsnprintf(char *, size_t, const char
 intvsnrprintf(char *, size_t, int, const char *, __va_list) 
__printflike(4, 0);
 intvsprintf(char *buf, const char *, __va_list) __printflike(2, 0);
 intttyprintf(struct tty *, const char *, ...) __printflike(2, 3);
-intsscanf(const char *, char const *, ...) __nonnull(1) __nonnull(2);
-intvsscanf(const char *, char const *, __va_list) __nonnull(1) 
__nonnull(2);
+intsscanf(const char *, char const *, ...) __nonnull(1) __nonnull(2) 
__scanflike(2, 3);
+intvsscanf(const char *, char const *, __va_list) __nonnull(1) 
__nonnull(2) __scanflike(2, 0);
 long   strtol(const char *, char **, int) __nonnull(1);
 u_long strtoul(const char *, char **, int) __nonnull(1);
 quad_t strtoq(const char *, char **, int) __nonnull(1);
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"