[PATCH v4 3/8] timespec32: Rename to old_timespec32

[Alistair Francis]

On Tue, Mar 10, 2020 at 11:52 AM Dmitry V. Levin <ldv at altlinux.org> wrote:
>
> On Tue, Mar 10, 2020 at 11:32:24AM -0700, Alistair Francis wrote:
> > On Tue, Mar 10, 2020 at 11:34 AM Dmitry V. Levin <ldv at altlinux.org> wrote:
> > >
> > > On Tue, Mar 10, 2020 at 11:04:54AM -0700, Alistair Francis wrote:
> > > > On Mon, Mar 9, 2020 at 6:44 PM Dmitry V. Levin <ldv at altlinux.org> wrote:
> > > > > On Mon, Mar 09, 2020 at 05:42:59PM -0700, Alistair Francis wrote:
> > > > > > The Linux kernel calls the 32-bit timespec struct old_timespec32, so
> > > > > > let's rename it to be clear.
> > > > >
> > > > > I don't find old_timespec32 any more clear than timespec32, sorry.
> > > > > Why add a prefix when the suffix is quite descriptive already?
> > > >
> > > > The main reason is just to be explicitly clear.
> > > >
> > > > There are three timespecs in the Linux kernel:
> > > >  - timespec64 - include/linux/time64.h
> > > >     - This one gives a 64-bit version for all systems
> > > >  - old_timespec32 - include/linux/time64.h
> > > >     - This is only used on old 32-bit systems (with *_time32 syscalls)
> > > >  - timespec - include/uapi/linux/time.h
> > > >     - This is used for some syscalls (as seen in this series).
> > > >     - On 32-bit systems this is a 32-bit value (no matter what size
> > > > time_t is) so it can be confused with the current strace timespec32
> > > > (although it's not the same).
> > >
> > > Sorry, I don't follow.  Isn't it either kernel_timespec64_t or
> > > kernel_timespec32_t in every case?  Could you give an example where
> > > it doesn't match neither kernel_timespec64_t nor kernel_timespec32_t?
> >
> > The timespec here is the same as the struct __kernel_old_timespec.
> >
> > For 32-bit systems __kernel_old_timespec and kernel_timespec32_t will
> > end up being the same, but the definitions are different.
> >
> > struct timespec {
> >     __kernel_old_time_t    tv_sec;        /* seconds */
> >     long            tv_nsec;    /* nanoseconds */
> > };
> >
> > AND
> >
> > typedef s32        old_time32_t;
> >
> > struct old_timespec32 {
> >     old_time32_t    tv_sec;
> >     s32        tv_nsec;
> > };
>
> So there is no difference from strace decoders perspective which already
> do the right thing by invoking proper functions.

I don't think they do though.

For RV32 ARCH_TIMESIZE is defined as 8 (64-bit time_t).

Which means that TIMESPEC_IS_32BIT is defined as 0, which is correct.

Which then means that we have these two definitions:

# define PRINT_TIMESPEC_DATA_SIZE print_timespec64_data_size
# define PRINT_TIMESPEC_ARRAY_DATA_SIZE print_timespec64_array_data_size

So the print_scm_timestamp_old()/print_scm_timestampns_old() functions
in msghdr.c will print a 64-bit timespec, even though for the old
functions timespec is only 32-bit. This means strace is decoding the
wrong values.

I don't see how the decoders are already doing the correct thing. Am I
missing something here?

Alistair

>
> I really don't see where this third structure might be used in strace itself.
>
> I suggest creating kernel_timespec_t and kernel_old_timespec_t
> so they could be used in tests like tests/sockopt-timestamp.c.
>
>
> --
> ldv