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

[Alistair Francis]

On Wed, Apr 1, 2020 at 11:27 AM Dmitry V. Levin <ldv at altlinux.org> wrote:
>
> 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).

The kernel_old_timespec32_t and kernel_old_timespec_t aren't the same though.

On a 64-bit architecutre it will look something like this:

typedef struct {
    32-bit tv_sec;
    32-bit tv_nsec;
} kernel_old_timespec32_t;

typedef struct {
    64-bit tv_sec;
    64-bit tv_nsec;
} kernel_old_timespec_t;

On a 32-bit architectures the types are the same bit lengths, but not
the same types.

I guess it's possible to dynamically switch between
print_timespec32_data_size and print_timespec64_data_size for
kernel_old_timespec_t and just ignore that we aren't actually printing
the correct types.

Would you prefer that then adding a new printing type?

>
> > 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?

There is no RV32 kernel ABI, it's just the generic ABI which is
stable. I don't see how it can be fixed.

>
> > 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.

Good point.

Alistair

>
>
> --
> ldv