Re: [PATCH 3/6] y2038: Introduce struct __timeval64 - new internal glibc type

[Lukasz Majewski <lukma at denx dot de>]

Hi Adhemerval,

> On 21/01/2020 13:45, Lukasz Majewski wrote:
> > Hi Arnd, Adhemerval,
> >   
> >> On Mon, Jan 20, 2020 at 5:46 PM Adhemerval Zanella
> >> <adhemerval.zanella@linaro.org> wrote:  
> >>> On 20/01/2020 10:00, Arnd Bergmann wrote:    
> >>>> On Mon, Jan 20, 2020 at 1:34 PM Lukasz Majewski <lukma@denx.de>
> >>>> wrote:    
> >>>>>>> On Sun, Jan 19, 2020 at 4:22 PM Lukasz Majewski
> >>>>>>> <lukma@denx.de> wrote:    
> >>>>>>>>> On Sat, Jan 18, 2020 at 11:48 PM Alistair Francis
> >>>>>>>>> <alistair23@gmail.com> wrote:    
> >>>>>>>
> >>>>>>> It would certainly help if this could be consistent across
> >>>>>>> architectures and libraries.    
> >>>>>>
> >>>>>> Ok. Thanks for the input.    
> >>>>>
> >>>>> Would it be OK to have:
> >>>>>
> >>>>> struct __timeval64
> >>>>> {
> >>>>>   __time64_t tv_sec;         /* Seconds */
> >>>>>   __int64_t tv_usec;       /* Microseconds */
> >>>>> };    
> >>>>
> >>>> This would not work if you pass it into a kernel interface that
> >>>> expects a 32-bit suseconds_t on sparc64, but it should work as an
> >>>> internal type on all 32-bit architectures.
> >>>>    
> >>>>> I would prefer to avoid changing typedef of suseconds_t as it
> >>>>> may affect struct timeval related operations.
> >>>>>
> >>>>> Using explicitly __int64_t for tv_usec seems better option,
> >>>>> isn't it?    
> >>>>
> >>>> Maybe a __suseconds64_t that happens to be 32-bit wide on
> >>>> sparc64?    
> >>>
> >>> If timeval64 kernel ABI expects tv_usec being a different type
> >>> depending of the architecture, it is an indication we should
> >>> parametrize with a __suseconds64_t.
> >>>
> >>> As a side note, why tv_usec for timeval64 is a 64-bit value? Is it
> >>> related to some alignment constraint?    
> >>
> >> I think it's mostly for simplicity: we do need the microseconds to
> >> be in the low bytes for compatibility with the 64-bit syscalls in
> >> the kernel, so it's either a 64-bit microseconds value or the same
> >> conditional padding that exists in timespec, with an extra special
> >> case for sparc64.
> >>
> >> The 64-bit case is slightly simpler for the few kernel interfaces
> >> that take a timeval as input and then can avoid conditionally
> >> zeroing out the upper 32 bits before checking the lower 32 bits
> >> again 1000000. I think that is only needed for clock_adjtimex,
> >> VIDIOC_QBUF and PPSETTIME though, everything else only passes a
> >> timeval to user space.  
> > 
> > I would opt for simpler approach :-)
> > 
> > Is the below code correct?
> > ./include/time.h
> > 
> > struct __timeval64
> > {
> >    __time64_t tv_sec;         /* Seconds */
> >    __suseconds64_t tv_usec;       /* Microseconds */
> > };
> > 
> > However, I'm a bit confused on the correct place to define
> > __suseconds64_t.
> > 
> > Shall it be added to posix/bits/types.h?
> > __STD_TYPE __SUSECONDS_T_TYPE __suseconds_t; /* Signed count of
> > microseconds.  */  
> 
> 
> Yes, the idea is to add on posix/bits/types.h:
> 
> __STD_TYPE __SUSECONDS64_T_TYPE __suseconds64_t;
> 
> Then on generic bits bits/typesizes.h:
> 
> #define __SUSECONDS_T_TYPE      __SQUAD_TYPE;
> 
> And then also define it on each Linux bits/typesizes.h.

Ok.

> 
> > 
> > If yes, then wouldn't we break the compatibility by adding new type
> > to this file?  
> 
> What do you mean by break compatibility? It is a new type on an
> installed header used by new types.

Thanks for the explanation - now it is clear :-)


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

Attachment: pgpXSnRJD6BYG.pgp
Description: OpenPGP digital signature