[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 28 Jan 2020 08:18:45 -0500
From: Rich Felker <dalias@...c.org>
To: Florian Weimer <fweimer@...hat.com>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH] add statx
On Tue, Jan 28, 2020 at 11:41:33AM +0100, Florian Weimer wrote:
> * Rich Felker:
>
> >> >> We received feedback that our headers are not useful due to the
> >> >> __u64/uint64_t mismatch.
> >> >>
> >> >> <https://sourceware.org/bugzilla/show_bug.cgi?id=25292
> >> >> <https://sourceware.org/ml/libc-alpha/2019-12/msg00631.html>
> >> >
> >> > Uhg. That change seems unfortunate since it's incompatible with
> >> > theoretical future archs having 128-bit long long -- an idea I'm not
> >> > much a fan of, but don't actually want to preclude. Is there a reason
> >> > to actually care about compatibility with the kernel types?
> >>
> >> Yes, printf format strings. 8-(
> >
> > Why not let ppl get the warnings and fix them by casting to an
> > appropriate type to print. I prefer [u]intmax_t anyway to avoid the
> > PRIu64 etc. mess that makes your format strings unreadable and that
> > interferes with translation (*).
>
> I'm concerned that such casts hide or even introduce bugs, like casting
> tv_sec to int.
As you've found, people will do incorrect casts regardless, in places
where it makes a bigger difference than here, where using fixed basic
types isn't an option. I don't think the solution is throwing away
proper semantic use of types.
> Here's an example from the MySQL sources:
>
> int my_timeval_to_str(const struct timeval *tm, char *to, uint dec)
> {
> int len= sprintf(to, "%d", (int) tm->tv_sec);
> if (dec)
> len+= my_useconds_to_str(to + len, tm->tv_usec, dec);
> return len;
> }
In the particular case of time, we should probably teach GCC to issue
warnings (-Wy2038?) for casts from an expression whose type was
declared via a typedef named time_t (I know GCC tracks knowledge of
the typedef name used, since it shows it in other warnings) to a type
smaller than 64-bit.
> That's why the kernel always uses unsigned long long for __u64, which
> seems a reasonable trade-off to me. It will make porting to 128-bit
> architectures difficult. But I think we should focus on the
> architectures we have today.
I disagree strongly with the last sentence. Designing an *API* in a
way that's not compatible with anything but long long == 64-bit is bad
API design. Arguably you could get into having something like
k_[u]intNN_t or similar, and use these, but I feel like that's just
gratuitous ugliness. The userspace type for 64-bit fixed-size fields
is [u]int64_t and we should be promoting its consistent usage, not
fragmenting things around kernel uapi mistakes. (Perhaps kernel uapi
headers should be fixed so that, when __KERNEL__ is not defined, they
include <stdint.h> and define __u64 etc. in terms of the stdint
types?)
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
