[PATCH v3 2/4] printrusage: Correctly print the long types from the rusage struct

[Alistair Francis]

 insideOn Fri, Mar 6, 2020 at 7:12 PM Dmitry V. Levin <ldv at altlinux.org> wrote:
>
> On Fri, Mar 06, 2020 at 06:22:21PM -0800, Alistair Francis wrote:
> > The kernel's rusage struct uses the kernel's __kernel_old_timeval which
> > means that even for 32-bit archs with 64-bit time_t (like RV32) the time
> > values are 32-bit.
> >
> > There are two possible options here:
> >  1. Use the glibc getrusage() function. Which takes a struct rusage with
> >     a 64-bit time_t but does a syscall with a 32-bit time_t and handles
> >     the conversion internally.
> >  2. Specify our own rusage struct that uses a long for the time.
> >
> > This patch fixes the failure useing option 2 above as we then get to
> > call the raw syscall.
> >
> > Signed-off-by: Alistair Francis <alistair.francis at wdc.com>
> > ---
> >  print_timeval.c   | 13 +++++++++++++
> >  printrusage.c     |  4 ++--
> >  tests/getrusage.c | 28 ++++++++++++++++++++++++++--
> >  3 files changed, 41 insertions(+), 4 deletions(-)
> >
> > diff --git a/print_timeval.c b/print_timeval.c
> > index ff87b27b..4cc400e1 100644
> > --- a/print_timeval.c
> > +++ b/print_timeval.c
> > @@ -7,6 +7,7 @@
> >   */
> >
> >  #include "defs.h"
> > +#include <linux/time_types.h>
>
> We cannot rely on this header because it's too new: it was introduced by
> commit v5.1-rc1~178^2~227^2~1 while strace still builds with kernel
> headers many years older than v5.1.
>
> In other words, we have to provide a correct definition of the structure
> we need without any help from kernel headers.

Yep, I won't use this then.

>
> >  #include DEF_MPERS_TYPE(timeval_t)
> >
> > @@ -25,6 +26,13 @@ print_timeval_t(const timeval_t *t)
> >               zero_extend_signed_to_ull(t->tv_usec));
> >  }
> >
> > +static void
> > +print_old_timeval_t(const struct __kernel_old_timeval *t)
> > +{
> > +     tprintf(timeval_fmt, (long long) t->tv_sec,
> > +             zero_extend_signed_to_ull(t->tv_usec));
> > +}
>
> Yet again, I don't understand why do we need yet another printer.
>
> Since there are no architectures where kernel definitions of struct timeval
> and struct __kernel_old_timeval wouldn't match, print_timeval_t should be

They don't match for RV32 (and all y2038 safe architectures).

> enough if timeval_t is defined correctly.

I don't know how it could be defined to print both as 32-bit y2038
safe archs have a 32-bit and 64-bit timeval.

>
> In other words, if you need print_old_timeval_t, then print_timeval_t
> is already broken.

It's not completely broken, there are just two different timeval structs.

>
> > +
> >  static void
> >  print_timeval_t_utime(const timeval_t *t)
> >  {
> > @@ -38,6 +46,11 @@ MPERS_PRINTER_DECL(void, print_struct_timeval, const void *arg)
> >       print_timeval_t(arg);
> >  }
> >
> > +MPERS_PRINTER_DECL(void, print_struct_old_timeval, const void *arg)
> > +{
> > +     print_old_timeval_t(arg);
> > +}
> > +
> >  MPERS_PRINTER_DECL(bool, print_struct_timeval_data_size,
> >                  const void *arg, const size_t size)
> >  {
> > diff --git a/printrusage.c b/printrusage.c
> > index 7cfeb7a0..04da400a 100644
> > --- a/printrusage.c
> > +++ b/printrusage.c
> > @@ -51,9 +51,9 @@ MPERS_PRINTER_DECL(void, printrusage,
> >               return;
> >
> >       tprints("{ru_utime=");
> > -     MPERS_FUNC_NAME(print_struct_timeval)(&ru.ru_utime);
> > +     MPERS_FUNC_NAME(print_struct_old_timeval)(&ru.ru_utime);
> >       tprints(", ru_stime=");
> > -     MPERS_FUNC_NAME(print_struct_timeval)(&ru.ru_stime);
> > +     MPERS_FUNC_NAME(print_struct_old_timeval)(&ru.ru_stime);
> >       if (abbrev(tcp)) {
> >               tprints(", ...");
> >       } else {
> > diff --git a/tests/getrusage.c b/tests/getrusage.c
> > index b046fde2..cb2c4835 100644
> > --- a/tests/getrusage.c
> > +++ b/tests/getrusage.c
> > @@ -21,8 +21,32 @@
> >  # include "xlat.h"
> >  # include "xlat/usagewho.h"
> >
> > +struct kernel_old_timeval {
> > +     kernel_long_t   tv_sec;
> > +     long            tv_usec;
> > +};
> > +
> > +struct rusage_t {
> > +     struct kernel_old_timeval ru_utime;
> > +     struct kernel_old_timeval ru_stime;
> > +     kernel_long_t   ru_maxrss;
> > +     kernel_long_t   ru_ixrss;
> > +     kernel_long_t   ru_idrss;
> > +     kernel_long_t   ru_isrss;
> > +     kernel_long_t   ru_minflt;
> > +     kernel_long_t   ru_majflt;
> > +     kernel_long_t   ru_nswap;
> > +     kernel_long_t   ru_inblock;
> > +     kernel_long_t   ru_oublock;
> > +     kernel_long_t   ru_msgsnd;
> > +     kernel_long_t   ru_msgrcv;
> > +     kernel_long_t   ru_nsignals;
> > +     kernel_long_t   ru_nvcsw;
> > +     kernel_long_t   ru_nivcsw;
> > +};
>
> Could we use <linux/resource.h> instead of <sys/resource.h> in this test?

I don't see why not, as long as it uses the kernel_old_timeval.

Alistair

>
>
> --
> ldv