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

[Dmitry V. Levin]

On Wed, Apr 01, 2020 at 12:34:54PM -0700, Alistair Francis wrote:
> 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.

I didn't say that kernel_timespec32_t and kernel_old_timespec_t are the
same, I said that either kernel_old_timespec_t is the same as
kernel_timespec64_t or it's the same as kernel_timespec32_t,
there is no third type different from both kernel_timespec64_t
and kernel_timespec32_t.

> 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'm not quite follow.
What's the difference between 32-bit int and 32-bit long int?
Aren't they the same data type?

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

I don't think the switch has to be dynamic.

I suppose tweaking ifdef's in print_timespec.c (to choose the right
implementation) and in print_timespec32.c (to enable the right
implementation) should be enough.

For example, we might add something like this to linux/arch_defs_.h:

#ifndef MIN_KLONGSIZE
# if SUPPORTED_PERSONALITIES > 1
#  define MIN_KLONGSIZE 4
# else
#  define MIN_KLONGSIZE SIZEOF_KERNEL_LONG_T
# endif
#endif

#ifndef HAVE_ARCH_TIMESPEC32
# define HAVE_ARCH_TIMESPEC32 (MIN_KLONGSIZE == 4)
#endif

... and use this HAVE_ARCH_TIMESPEC32 in print_timespec32.c


-- 
ldv