[Toybox] I aten't dead.

[Rob Landley]

On 02/24/2015 03:59 PM, Rich Felker wrote:
> On Tue, Feb 24, 2015 at 01:50:03PM -0800, enh wrote:
>> On Tue, Feb 24, 2015 at 1:48 PM, Rich Felker <dalias at libc.org> wrote:
>>> On Tue, Feb 24, 2015 at 01:30:20PM -0800, enh wrote:
>>>> On Tue, Feb 24, 2015 at 11:46 AM, Rob Landley <rob at landley.net> wrote:
>>>>> Sorry for the radio silence, $DAYJOB's been eating the majority of my
>>>>> programming time and what's left has gone to Aboriginal Linux recently,
>>>>> because I made a largeish design change over there (putting the base
>>>>> root filesystem in initmpfs and merging the native-compiler at runtime
>>>>> instead of compile time), and the aboriginal linux release soft-blocks
>>>>> the toybox release because I use "built aboriginal and then built linux
>>>>> from scratch under it" as my main toybox regression-smoketest.
>>>>>
>>>>> Last night's I hit a fun bug in toybox "stat" where it doesn't remotely
>>>>> work on arm. (Works fine on x86 host, but 3.18 kernel built against
>>>>> uClibc on armv5l: the -f numbers are way off, it thinks the blocksize of
>>>>> ext2fs is 32 bytes.)
>>>>
>>>> seems fine on armv7 3.10 with bionic. oh, specifically the -f output.
>>>> yeah, that looks wrong :-)
>>>>
>>>> a lot of those fields are actually 64-bit, so you need an extra 'l' on LP32:
>>>>
>>>> diff --git a/toys/other/stat.c b/toys/other/stat.c
>>>> index d603316..a96c1de 100644
>>>> --- a/toys/other/stat.c
>>>> +++ b/toys/other/stat.c
>>>> @@ -106,11 +106,11 @@ static void print_stat(char type)
>>>>  static void print_statfs(char type) {
>>>>    struct statfs *statfs = (struct statfs *)&TT.stat;
>>>>
>>>> -  if (type == 'a') xprintf("%lu", statfs->f_bavail);
>>>> -  else if (type == 'b') xprintf("%lu", statfs->f_blocks);
>>>> -  else if (type == 'c') xprintf("%lu", statfs->f_files);
>>>> -  else if (type == 'd') xprintf("%lu", statfs->f_ffree);
>>>> -  else if (type == 'f') xprintf("%lu", statfs->f_bfree);
>>>> +  if (type == 'a') xprintf("%llu", statfs->f_bavail);
>>>> +  else if (type == 'b') xprintf("%llu", statfs->f_blocks);
>>>> +  else if (type == 'c') xprintf("%llu", statfs->f_files);
>>>> +  else if (type == 'd') xprintf("%llu", statfs->f_ffree);
>>>> +  else if (type == 'f') xprintf("%llu", statfs->f_bfree);
>>>>    else if (type == 'l') xprintf("%ld", statfs->f_namelen);
>>>>    else if (type == 't') xprintf("%lx", statfs->f_type);
>>>>    else if (type == 'i')

Yeah, that looks like the fix.

>>> Rather than hard-coding an assumption that these have type unsigned
>>> long long, the code should be casting them to unsigned long long or
>>> uintmax_t and using an appropriate format specifier. I prefer
>>> uintmax_t since it's idiomatic and can be used for formatting any
>>> unsigned type, but I know Rob can be allergic to these nice semantic
>>> types... :-)

I've used uint64_t sometimes, but they're really silly names and LP64 is
a real thing. The only part that's _not_ quite standardized is that
"long long" is "at least" 64 bits, meaning it could be 128. But it
currently isn't anywhere that I can find, and the uint64_t
implementations all boil down to unsigned long long, so...

No, what annoys me here is that the types in the man page are ridiculous:

   struct statfs {
       __SWORD_TYPE f_type;    /* type of filesystem (see below) */
       __SWORD_TYPE f_bsize;   /* optimal transfer block size */
       fsblkcnt_t   f_blocks;  /* total data blocks in filesystem */
       fsblkcnt_t   f_bfree;   /* free blocks in fs */
       fsblkcnt_t   f_bavail;  /* free blocks available to
                                  unprivileged user */
       fsfilcnt_t   f_files;   /* total file nodes in filesystem */
       fsfilcnt_t   f_ffree;   /* free file nodes in fs */
       fsid_t       f_fsid;    /* filesystem id */
       __SWORD_TYPE f_namelen; /* maximum length of filenames */
       __SWORD_TYPE f_frsize;  /* fragment size (since Linux 2.6) */
       __SWORD_TYPE f_spare[5];
   };

(In this cae "SWORD_TYPE" is "katana".) The reason buried later in the
man page is that statfs got replaced with statfs64 and they tried to
hide the differences with magic macros.

What it _also_ says is that there's a posix statvfs in which f_fsid is
unsigned long, instead of unsigned long long. So I could switch from the
LSB to the POSIX thing, at the expense of having a data type mangled.

>> yeah, i almost said the same but because i know that you can't even
>> rely on the signedness being the same between different architectures
>> i didn't want to get into that argument :-)
> 
> I wouldn't worry about the signedness unless negative values are
> actually meaningful. Here they're not so an unsigned type is safe; the
> full positive range of any type fits in uintmax_t.

On 32 bit systems glibc does horrible things under the covers. I don't
hugely care, but gratuitously unnecessary conversions are not a
substitute for understanding what your code is actually doing.

> Rich

Rob

 1424843416.0