musl - Re: [PATCH] add statx

Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87blqneuci.fsf@oldenburg2.str.redhat.com>
Date: Tue, 28 Jan 2020 11:41:33 +0100
From: Florian Weimer <fweimer@...hat.com>
To: Rich Felker <dalias@...c.org>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH] add statx

* 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.

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;
  }

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.

>> > It's not like both struct definitions can be included in the same
>> > source anyway; the tags would clash. Using the canonical uintN_t types
>> > makes more sense from an API perspective, I think; kernel assumptions
>> > about types should not leak into an API intended to be clean and
>> > shared with non-Linux implementations.
>> 
>> I thought so too, which is why I used them.
>> 
>> But it is fairly compelling to use the kernel types if the header is
>> available, so that we don't have to patch and rebuild glibc if someone
>> backports new statx features into the kernel.  That's why I added the
>> __has_include kludge.
>
> I see. This is something of a tradeoff I guess; for musl users I'd
> expect folks to have libc headers newer than kernel headers in most
> cases since distros are slow to follow new kernels but fast to follow
> new libc, and you want compile-time support for future kernel
> features even if you don't have the kernel to use them yet. But on
> glibc-based dists it may be the other way around.

I think it's actually the same for self-built musl on most glibc-based
distributions because for that, developers will usually pick the latest
musl release and use the distribution's kernel headers.

But for glibc itself on glibc distributions, it's likely to have newer
kernel headers.  Fedora rebase kernels (including the UAPI headers) in a
release, but not glibc.  Debian has backport kernel packages for stable
releases (but no glibc backport).  Some enterprise distributions
aggressively backport features (including new userspace APIs) and even
rebase entire kernel subsystems from time to time.

Thanks,
Florian
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.