This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [RFC PATCH] [AARCH64] ILP32: support stat syscall family
- From: Yury Norov <ynorov at caviumnetworks dot com>
- To: Joseph Myers <joseph at codesourcery dot com>
- Cc: <libc-alpha at sourceware dot org>, <arnd at arndb dot de>, <catalin dot marinas at arm dot com>, <davem at davemloft dot net>, <szabolcs dot nagy at arm dot com>, <maxim dot kuvyrkov at linaro dot org>, <pinskia at gmail dot com>, <bamvor dot zhangjian at huawei dot com>, <schwab at suse dot de>, <fweimer at redhat dot com>, <Prasun dot Kapoor at cavium dot com>, <adhemerval dot zanella at linaro dot org>, Yury Norov <yury dot norov at gmail dot com>
- Date: Fri, 1 Jul 2016 12:03:01 +0300
- Subject: Re: [RFC PATCH] [AARCH64] ILP32: support stat syscall family
- Authentication-results: sourceware.org; auth=none
- Authentication-results: spf=none (sender IP is ) smtp dot mailfrom=Yuri dot Norov at caviumnetworks dot com;
- References: <1467291615-30356-1-git-send-email-ynorov at caviumnetworks dot com> <alpine dot DEB dot 2 dot 20 dot 1606301347050 dot 27126 at digraph dot polyomino dot org dot uk>
- Spamdiagnosticmetadata: NSPM
- Spamdiagnosticoutput: 1:99
On Thu, Jun 30, 2016 at 01:51:22PM +0000, Joseph Myers wrote:
> On Thu, 30 Jun 2016, Yury Norov wrote:
>
> > - __32_BIT_ABI_SUPPORTS_64_BIT_TIME_T macro introduced to indicate that 32-bit
> > ABI has struct __timespec (and maybe __timeval in future) that is compatible
> > to 64-bit _timespec;
>
> You can't add a macro to one architecture like that then use it in generic
> code. You have to define it in *every* architecture's bits/typesizes.h
> before it can be used in generic code.
Is it OK to do like this then?
#define __32_BIT_ABI_SUPPORTS_64_BIT_TIME_T /* w/o 1 or 0 */
Where's a proper place for it? In features.h maybe?
> > + return INLINE_SYSCALL (fstatfs64, 2, fd, buf);
>
> Bad indentation. Likewise elsewhere in this patch.
>
> > +++ b/sysdeps/unix/sysv/linux/aarch64/ilp32/statfs64.c
> > @@ -0,0 +1,37 @@
> > +/* Copyright (C) 2011-2015 Free Software Foundation, Inc.
>
> Still missing descriptive first line and 2016 in copyright dates.
>
> > + Contributed by Chris Metcalf <cmetcalf@tilera.com>, 2011.
>
> There should be no "Contributed by" lines in new files.
>
> > + return result;
> > +#endif
> > +#if __32_BIT_ABI_SUPPORTS_64_BIT_TIME_T
> > + if (!result)
> > + {
> > + conv_timespec(&buf->st_atim, &buf->__st_atim);
> > + conv_timespec(&buf->st_mtim, &buf->__st_mtim);
> > + conv_timespec(&buf->st_ctim, &buf->__st_ctim);
> > + }
> > #endif
>
> Indentation way off.
It looks ugly only in email. If you apply patch, it will be better.
>
> > + conv_timespec(&st->st_atim, &st->__st_atim);
> > + conv_timespec(&st->st_mtim, &st->__st_mtim);
> > + conv_timespec(&st->st_ctim, &st->__st_ctim);
>
> Missing space before '('.
This rule is scary, for sure. I cannot autoreplace all such places as
it doesn't work for definitions, and I cannot drop whitespaces everywhere
because it breaks rules. So I have to pay attention to it again and
again. Script you pointed me doesn't understand difference between
definition and usage of macro. I spent 2 hours fixing the fixes I made
following the report of that script.
Could someone give me clear understanding, how exactly this rule
improves readability and helps finding bugs. I write it because this
rule makes real troubles to me, and helps with nothing.
> > + DECLARE_TIMESPEC (st_atim); /* Time of last access. */
> > + DECLARE_TIMESPEC (st_mtim); /* Time of last modification. */
> > + DECLARE_TIMESPEC (st_ctim); /* Time of last status change. */
>
> DECLARE_TIMESPEC is in the user's namespace, not valid in installed
> headers.
>
> > +# define conv_timespec(ts, _ts) \
>
> Again, not valid in installed headers.
What the proper place you suggest? New timer-related private header?
Yury