Re: [PATCH] linux-user: Add strace support for printing arguments for ioctls used for terminals and serial lines

[Laurent Vivier]

Le 14/07/2020 à 22:04, Filip Bozuta a écrit :
> Functions "print_ioctl()" and "print_syscall_ret_ioctl()" are used
> to print arguments of "ioctl()" with "-strace". These functions
> use "thunk_print()", which is defined in "thunk.c", to print the
> contents of ioctl's third arguments that are not basic types.
> However, this function doesn't handle ioctls of group ioctl_tty which
> are used for terminals and serial lines. These ioctls use a type
> "struct termios" which thunk type is defined in a non standard
> way using "STRUCT_SPECIAL()". This means that this type is not decoded
> regularly using "thunk_convert()" and uses special converting functions
> "target_to_host_termios()" and "host_to_target_termios()", which are defined
> in "syscall.c" to decode it's values. For simillar reasons, this type is
> also not printed regularly using "thunk_print()". That is the reason
> why a separate printing function "print_termios()" is defined in
> file "strace.c". This function decodes and prints flag values of the
> "termios" structure.
> 
> Implementation notes:
> 
>     Function "print_termios()" was implemented in "strace.c" using
>     an existing function "print_flags()" to print flag values of
>     "struct termios" fields. These flag values were defined
>     using an existing macro "FLAG_TARGET()" that generates aproppriate
>     target flag values and string representations of these flags.
>     This function was declared in "qemu.h" so that it can be accessed
>     in "syscall.c". Type "StructEntry" defined in "exec/user/thunk.h"
>     contains information that is used to decode structure values.
>     Field "void print(void *arg)" was added in this structure as a
>     special print function. Also, function "thunk_print()" was changed
>     a little so that it uses this special print function in case
>     it is defined. This printing function was instantiated with the
>     defined "print_termios()" in "syscall.c" in "struct_termios_def".
> 
> Signed-off-by: Filip Bozuta <Filip.Bozuta@syrmia.com>
> ---
>  include/exec/user/thunk.h |   1 +
>  linux-user/qemu.h         |   2 +
>  linux-user/strace.c       | 193 ++++++++++++++++++++++++++++++++++++++
>  linux-user/syscall.c      |   1 +
>  thunk.c                   |  23 +++--
>  5 files changed, 211 insertions(+), 9 deletions(-)
> 
> diff --git a/include/exec/user/thunk.h b/include/exec/user/thunk.h
> index 7992475c9f..a5bbb2c733 100644
> --- a/include/exec/user/thunk.h
> +++ b/include/exec/user/thunk.h
> @@ -55,6 +55,7 @@ typedef struct {
>      int *field_offsets[2];
>      /* special handling */
>      void (*convert[2])(void *dst, const void *src);
> +    void (*print)(void *arg);
>      int size[2];
>      int align[2];
>      const char *name;
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 5c964389c1..e51a0ededb 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -706,6 +706,8 @@ static inline uint64_t target_offset64(uint64_t word0, 
> uint64_t word1)
>  }
>  #endif /* TARGET_ABI_BITS != 32 */
>  
> +extern void print_termios(void *arg);
> +
>  /**
>   * preexit_cleanup: housekeeping before the guest exits
>   *
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index 5235b2260c..7b5408cf4a 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -1193,6 +1193,138 @@ UNUSED static struct flags falloc_flags[] = {
>  #endif
>  };
>  
> +UNUSED static struct flags termios_iflags[] = {
> +    FLAG_TARGET(IGNBRK),
> +    FLAG_TARGET(BRKINT),
> +    FLAG_TARGET(IGNPAR),
> +    FLAG_TARGET(PARMRK),
> +    FLAG_TARGET(INPCK),
> +    FLAG_TARGET(ISTRIP),
> +    FLAG_TARGET(INLCR),
> +    FLAG_TARGET(IGNCR),
> +    FLAG_TARGET(ICRNL),
> +    FLAG_TARGET(IUCLC),
> +    FLAG_TARGET(IXON),
> +    FLAG_TARGET(IXANY),
> +    FLAG_TARGET(IXOFF),
> +    FLAG_TARGET(IMAXBEL),

IUTF8 is missing

> +    FLAG_END,
> +};
> +
> +UNUSED static struct flags termios_oflags[] = {
> +    FLAG_TARGET(OPOST),
> +    FLAG_TARGET(OLCUC),
> +    FLAG_TARGET(ONLCR),
> +    FLAG_TARGET(OCRNL),
> +    FLAG_TARGET(ONOCR),
> +    FLAG_TARGET(ONLRET),
> +    FLAG_TARGET(OFILL),
> +    FLAG_TARGET(OFDEL),
> +    FLAG_END,
> +};
> +

the following entries are not flags: flags are bits, while we have
enumerated values in these cases.

NL0, NL1 = 0,1

> +UNUSED static struct flags termios_oflags_NLDLY[] = {
> +    FLAG_TARGET(NL0),
> +    FLAG_TARGET(NL1),
> +    FLAG_END,
> +};

CR0, CR1, CR2, CR3 = 0, 1, 2, 3

> +UNUSED static struct flags termios_oflags_CRDLY[] = {
> +    FLAG_TARGET(CR0),
> +    FLAG_TARGET(CR1),
> +    FLAG_TARGET(CR2),
> +    FLAG_TARGET(CR3),
> +    FLAG_END,
> +};
>

TAB0 is 0 so it cannot be detected

> +UNUSED static struct flags termios_oflags_TABDLY[] = {
> +    FLAG_TARGET(TAB0),
> +    FLAG_TARGET(TAB1),
> +    FLAG_TARGET(TAB2),
> +    FLAG_TARGET(TAB3),
> +    FLAG_END,
> +};

VT0 is 0

> +UNUSED static struct flags termios_oflags_VTDLY[] = {
> +    FLAG_TARGET(VT0),
> +    FLAG_TARGET(VT1),
> +    FLAG_END,
> +};

FF0 is 0

> +UNUSED static struct flags termios_oflags_FFDLY[] = {
> +    FLAG_TARGET(FF0),
> +    FLAG_TARGET(FF1),
> +    FLAG_END,
> +};

BS0 is 0

> +UNUSED static struct flags termios_oflags_BSDLY[] = {
> +    FLAG_TARGET(BS0),
> +    FLAG_TARGET(BS1),
> +    FLAG_END,
> +};

ditto: these are not bits but enumerated values (B0 is 0).

> +UNUSED static struct flags termios_cflags_CBAUD[] = {
> +    FLAG_TARGET(B0),
> +    FLAG_TARGET(B50),
> +    FLAG_TARGET(B75),
> +    FLAG_TARGET(B110),
> +    FLAG_TARGET(B134),
> +    FLAG_TARGET(B150),
> +    FLAG_TARGET(B200),
> +    FLAG_TARGET(B300),
> +    FLAG_TARGET(B600),
> +    FLAG_TARGET(B1200),
> +    FLAG_TARGET(B1800),
> +    FLAG_TARGET(B2400),
> +    FLAG_TARGET(B4800),
> +    FLAG_TARGET(B9600),
> +    FLAG_TARGET(B19200),
> +    FLAG_TARGET(B38400),
> +    FLAG_TARGET(B57600),
> +    FLAG_TARGET(B115200),
> +    FLAG_TARGET(B230400),
> +    FLAG_TARGET(B460800),
> +    FLAG_END,
> +};

CS5 is 0

> +UNUSED static struct flags termios_cflags_CSIZE[] = {
> +    FLAG_TARGET(CS5),
> +    FLAG_TARGET(CS6),
> +    FLAG_TARGET(CS7),
> +    FLAG_TARGET(CS8),
> +    FLAG_END,
> +};

These ones are ok:

> +UNUSED static struct flags termios_cflags[] = {
> +    FLAG_TARGET(CSTOPB),
> +    FLAG_TARGET(CREAD),
> +    FLAG_TARGET(PARENB),
> +    FLAG_TARGET(PARODD),
> +    FLAG_TARGET(HUPCL),
> +    FLAG_TARGET(CLOCAL),
> +    FLAG_TARGET(CRTSCTS),
> +    FLAG_END,
> +};

These ones too:

> +UNUSED static struct flags termios_lflags[] = {
> +    FLAG_TARGET(ISIG),
> +    FLAG_TARGET(ICANON),
> +    FLAG_TARGET(XCASE),
> +    FLAG_TARGET(ECHO),
> +    FLAG_TARGET(ECHOE),
> +    FLAG_TARGET(ECHOK),
> +    FLAG_TARGET(ECHONL),
> +    FLAG_TARGET(NOFLSH),
> +    FLAG_TARGET(TOSTOP),
> +    FLAG_TARGET(ECHOCTL),
> +    FLAG_TARGET(ECHOPRT),
> +    FLAG_TARGET(ECHOKE),
> +    FLAG_TARGET(FLUSHO),
> +    FLAG_TARGET(PENDIN),
> +    FLAG_TARGET(IEXTEN),

missing EXTPROC

> +    FLAG_END,
> +};
> +
>  /*
>   * print_xxx utility functions.  These are used to print syscall
>   * parameters in certain format.  All of these have parameter
> @@ -1420,6 +1552,67 @@ print_timezone(abi_ulong tz_addr, int last)
>      }
>  }
>  
> +void
> +print_termios(void *arg)
> +{
> +    const struct target_termios *target = arg;
> +
> +    abi_long iflags = tswap32(target->c_iflag);
> +    abi_long oflags = tswap32(target->c_oflag);
> +    abi_long cflags = tswap32(target->c_cflag);
> +    abi_long lflags = tswap32(target->c_lflag);

You should use target_tcflag_t here, rather than abi_long.
To be able to do that we need some cleanup before:
- define target_tcflag_t, target_cc_t, target_speed_t in a new file
  linux-user/generic/termbits.h
- update all the existing target_termios to use them

> +
> +    qemu_log("{");
> +
> +    qemu_log("c_iflag = ");
> +    print_flags(termios_iflags, iflags, 0);
> +
> +    qemu_log("c_oflag = ");
> +    abi_long oflags_clean =  oflags & ~(TARGET_NLDLY) & ~(TARGET_CRDLY) &
> +                                      ~(TARGET_TABDLY) & ~(TARGET_BSDLY) &
> +                                      ~(TARGET_VTDLY) & ~(TARGET_FFDLY);

I think would simpler to write:

oflags & ~(TARGET_NLDLY | TARGET_CRDLY | TARGET_TABDLY | TARGET_BSDLY |
TARGET_VTDLY | TARGET_FFDLY)

> +    print_flags(termios_oflags, oflags_clean, 0);
> +    if (oflags & TARGET_NLDLY) {

NL0 is 0 so you never display it.

> +        print_flags(termios_oflags_NLDLY, oflags & TARGET_NLDLY, 0);
> +    }
> +    if (oflags & TARGET_CRDLY) {

CR0 is 0, same comment for the following ones.

> +        print_flags(termios_oflags_CRDLY, oflags & TARGET_CRDLY, 0);
> +    }
> +    if (oflags & TARGET_TABDLY) {
> +        print_flags(termios_oflags_TABDLY, oflags & TARGET_TABDLY, 0);
> +    }
> +    if (oflags & TARGET_BSDLY) {
> +        print_flags(termios_oflags_BSDLY, oflags & TARGET_BSDLY, 0);
> +    }
> +    if (oflags & TARGET_VTDLY) {
> +        print_flags(termios_oflags_VTDLY, oflags & TARGET_VTDLY, 0);
> +    }
> +    if (oflags & TARGET_FFDLY) {
> +        print_flags(termios_oflags_FFDLY, oflags & TARGET_FFDLY, 0);
> +    }
> +
> +    qemu_log("c_cflag = ");
> +    if (cflags & TARGET_CBAUD) {
> +        print_flags(termios_cflags_CBAUD, cflags & TARGET_CBAUD, 0);
> +    }
> +    if (cflags & TARGET_CSIZE) {
> +        print_flags(termios_cflags_CSIZE, cflags & TARGET_CSIZE, 0);
> +    }
> +    abi_long cflags_clean = cflags & ~(TARGET_CBAUD) & ~(TARGET_CSIZE);
> +    print_flags(termios_cflags, cflags_clean, 0);
> +
> +    qemu_log("c_lflag = ");
> +    print_flags(termios_lflags, lflags, 0);
> +
> +    qemu_log("c_cc = ");
> +    qemu_log("\"%s\",", target->c_cc);
> +
> +    qemu_log("c_line = ");
> +    print_raw_param("\'%c\'", target->c_line, 1);
> +
> +    qemu_log("}");
> +}
> +
>  #undef UNUSED
>  
>  #ifdef TARGET_NR_accept

Thanks,
Laurent