Re: [musl] Re: [PATCH] add statx

[Rich Felker <dalias@libc.org>]

On Tue, Jan 28, 2020 at 09:59:56AM +0100, Ben Noordhuis wrote:
> On Fri, Jan 24, 2020 at 3:01 PM Rich Felker <dalias@libc.org> wrote:
> >
> > On Fri, Jan 24, 2020 at 09:38:49AM +0100, Ben Noordhuis wrote:
> > >
> > > Can I get some feedback on this patch, even if it's just "no because"? Thanks.
> >
> > Sorry aboout that; I'd just had my mind on other things and hadn't
> > taken the time to make a good review yet.
> 
> Thanks for the feedback and no worries, I'm no saint in that regard either.
> 
> Before I post a v2, did I understand the following issues correctly?
> 
> 1. Switch _GNU_SOURCE || _BSD_SOURCE -> just _GNU_SOURCE?

Yes. 

> FWIW, _BSD_SOURCE currently exposes the AT_STATX_* flags in fcntl.h.

Being that they're "namespaced" under AT_*, I don't think this is a
big deal. I was generally of the opinion that AT_* flags make sense to
always expose since they might be used with standard interfaces as
extensions, rather than with nonstandard interfaces. Even though
AT_STATX_* seem statx-exclusive, maybe they make sense as extension
flags to fstatat too?

In any case I think we can leave any consideration of a change here
for later; it's separate from adding statx().

> 2. uint64_t -> unsigned long long guarded by #ifdef __GNUC__
> __extension__? Or just leave it as-is?

long long does not use __extension__ guard in musl, but I think we
should stick with the semantically correct types, [u]intNN_t.

> 4. An ENOSYS fallback to fstatat()? glibc's fallback returns EINVAL
> for AT_* flags it doesn't understand and ignores all STATX_* flags: it
> sets stx_mask to STATX_BASIC_STATS, fills in stx_uid/stx_gid/etc. and
> sets stx_btime to zero. Does that sound reasonable?

Sounds right.

Note that filling in stx_[r]dev_{major,minor} requires the
sys/sysmacros.h major()/minor()/ macros since statx oddly separated
them out of dev_t.

I don't think setting btime to 0 is strictly necessary, but since it's
in the range of the struct that's unconditionally present, it's okay
to do so, and perhaps guards against info leaks from old stack
contents.

Rich