[PATCH v6 06/13] print_timespec: Add support for printing the kernel old_timespec

[Dmitry V. Levin]

On Wed, Apr 01, 2020 at 11:01:18AM -0700, Alistair Francis wrote:
> On Tue, Mar 31, 2020 at 5:31 PM Dmitry V. Levin <ldv at altlinux.org> wrote:
> >
> > On Fri, Mar 20, 2020 at 03:09:32PM -0700, Alistair Francis wrote:
> > > Add support for printing the newly added kernel_old_timespec_t.
> > >
> > > Signed-off-by: Alistair Francis <alistair.francis at wdc.com>
> > > ---
> > >  Makefile.am          |  1 +
> > >  defs.h               |  5 +++++
> > >  print_timespec_old.c | 19 +++++++++++++++++++
> > >  3 files changed, 25 insertions(+)
> > >  create mode 100644 print_timespec_old.c
> > >
> > > diff --git a/Makefile.am b/Makefile.am
> > > index cf45d952..d81004f7 100644
> > > --- a/Makefile.am
> > > +++ b/Makefile.am
> > > @@ -258,6 +258,7 @@ strace_SOURCES =  \
> > >       print_time.c    \
> > >       print_timespec.c \
> > >       print_timespec.h \
> > > +     print_timespec_old.c \
> > >       print_timespec32.c \
> > >       print_timespec64.c \
> > >       print_timeval.c \
> > > diff --git a/defs.h b/defs.h
> > > index fa746c99..c4aefd32 100644
> > > --- a/defs.h
> > > +++ b/defs.h
> > > @@ -1296,6 +1296,11 @@ extern int print_itimerspec32(struct tcb *, kernel_ulong_t);
> > >  extern int print_timex32(struct tcb *, kernel_ulong_t);
> > >  # endif /* HAVE_ARCH_TIME32_SYSCALLS */
> > >
> > > +extern bool print_old_timespec_data_size(const void *arg, size_t size);
> > > +extern bool print_old_timespec_array_data_size(const void *arg,
> > > +                                          unsigned int nmemb,
> > > +                                          size_t size);
> > > +
> > >  extern bool print_timespec64_data_size(const void *arg, size_t size);
> > >  extern bool print_timespec64_array_data_size(const void *arg,
> > >                                            unsigned int nmemb,
> > > diff --git a/print_timespec_old.c b/print_timespec_old.c
> > > new file mode 100644
> > > index 00000000..d8737e43
> > > --- /dev/null
> > > +++ b/print_timespec_old.c
> > > @@ -0,0 +1,19 @@
> > > +/*
> > > + * Copyright (c) 2020 The strace developers.
> > > + * All rights reserved.
> > > + *
> > > + * SPDX-License-Identifier: LGPL-2.1-or-later
> > > + */
> > > +
> > > +#include "defs.h"
> > > +
> > > +#define TIMESPEC_T kernel_old_timespec_t
> > > +#define PRINT_TIMESPEC_DATA_SIZE print_old_timespec_data_size
> > > +#define PRINT_TIMESPEC_ARRAY_DATA_SIZE print_old_timespec_array_data_size
> > > +#define PRINT_TIMESPEC print_old_timespec
> > > +#define SPRINT_TIMESPEC sprint_old_timespec
> > > +#define PRINT_TIMESPEC_UTIME_PAIR print_old_timespec_utime_pair
> > > +#define PRINT_ITIMERSPEC print_old_itimerspec
> > > +
> > > +#include "kernel_timespec.h"
> > > +#include "print_timespec.h"
> > > --
> > > 2.25.1
> >
> > No, this would penalize all systems that already have two copies
> > of printers, for 32-bit time_t and for 64-bit time_t.
> > They definitely don't need a third copy, do they?
> 
> I'm not sure how else to do this.
> 
> There are three different timespecs that need to be printed.

Well, I don't see it this way.
It's either kernel_timespec64_t (64-bit tv_sec and tv_nsec)
or kernel_timespec32_t (32-bit tv_sec and tv_nsec).

> Do you have a better idea?

Use either of these two sets of printers?

> > Even if you decided that RV32 needs all these insane 32-bit
> > SO_TIMESTAMP*_OLD implemented, this cannot be the right way
> > to do it.
> 
> I didn't decide anything, this is what the kernel supports.

Since the RV32 kernel ABI is not yet stable, this still can be fixed
on the kernel side.  Could you ask kernel people to remove
SO_TIMESTAMP*_OLD support on RV32?

> I am happy just not supporting it and removing the SO_TIMESTAMP*_OLD
> from the tests, but you didn't approve of my patch to do that.

It wouldn't be sufficient to remove SO_TIMESTAMP*_OLD just from tests
because there is a parser in strace itself that has to be tested.


-- 
ldv