Re: [Qemu-devel] [PATCH 1/5] linux-user: add socketcall() strace

[Peter Maydell]

On 8 June 2016 at 21:24, Laurent Vivier <address@hidden> wrote:
> From: Laurent Vivier <address@hidden>
>
> Signed-off-by: Laurent Vivier <address@hidden>
> ---
>  include/exec/user/abitypes.h |  23 ++
>  linux-user/strace.c          | 550 
> +++++++++++++++++++++++++++++++++++++++++++
>  linux-user/strace.list       |   2 +-
>  linux-user/syscall_defs.h    |  22 +-
>  4 files changed, 592 insertions(+), 5 deletions(-)

I have a few comments, but this mostly looks good.

> diff --git a/include/exec/user/abitypes.h b/include/exec/user/abitypes.h
> index 80eedac..e33b1f8 100644
> --- a/include/exec/user/abitypes.h
> +++ b/include/exec/user/abitypes.h
> @@ -46,6 +46,15 @@ static inline abi_ulong tswapal(abi_ulong v)
>      return tswap32(v);
>  }
>
> +static inline abi_ulong abi_ntohl(abi_ulong v)
> +{
> +#if defined(HOST_BIG_ENDIAN)
> +    return v;
> +#else
> +    return bswap_32(v);
> +#endif
> +}
> +
>  #else
>  typedef target_ulong abi_ulong __attribute__((aligned(ABI_LONG_ALIGNMENT)));
>  typedef target_long abi_long __attribute__((aligned(ABI_LONG_ALIGNMENT)));
> @@ -62,5 +71,19 @@ static inline abi_ulong tswapal(abi_ulong v)
>      return tswapl(v);
>  }
>
> +static inline abi_ulong abi_ntohl(abi_ulong v)
> +{
> +#if defined(HOST_BIG_ENDIAN)
> +    return v;
> +#else
> +#if TARGET_LONG_SIZE == 4
> +    return bswap_32(v);
> +#else
> +    return bswap_64(v);
> +#endif
> +#endif
> +}
> +
> +
>  #endif
>  #endif

I suspect we don't actually need an abi_ntohl() -- see below.

> @@ -1004,6 +1221,339 @@ print__llseek(const struct syscallname *name,
>  }
>  #endif
>
> +#if defined(TARGET_NR_socketcall)
> +static void
> +print_socketcall(const struct syscallname *name,
> +                 abi_long arg0, abi_long arg1, abi_long arg2,
> +                 abi_long arg3, abi_long arg4, abi_long arg5)
> +{
> +    const int n = sizeof(abi_ulong);

This is a kind of confusing variable name for this.

(Should we use the same code that do_socketcall in syscall.c
does to read the right number of arguments into an array
of abi_ulongs ?)

> +    const char *socketcallname;
> +
> +    switch (arg0) {
> +    case SOCKOP_bind: {
> +        abi_ulong sockfd, addr, addrlen;
> +
> +        socketcallname = "bind";
> +
> +print_sockaddr:
> +        get_user_ual(sockfd, arg1);
> +        get_user_ual(addr, arg1 + n);
> +        get_user_ual(addrlen, arg1 + 2 * n);
> +
> +        gemu_log("%s(", socketcallname);
> +        print_raw_param(TARGET_ABI_FMT_ld, sockfd, 0);
> +        print_sockaddr(addr, addrlen);
> +        gemu_log(")");
> +        break;

I think a helper function so you just say
do_print_sockaddr("bind", arg1);
would be nicer than these gotos. Similarly with the other code
like this below.

> +    }
> +    case SOCKOP_connect:
> +        socketcallname = "connect";
> +        goto print_sockaddr;
> +    case SOCKOP_accept:
> +        socketcallname = "accept";
> +        goto print_sockaddr;
> +    case SOCKOP_getsockname:
> +        socketcallname = "getsockname";
> +        goto print_sockaddr;
> +    case SOCKOP_getpeername:
> +        socketcallname = "getpeername";
> +        goto print_sockaddr;
> +    case SOCKOP_socket: {
> +        abi_ulong domain, type, protocol;
> +
> +        get_user_ual(domain, arg1);
> +        get_user_ual(type, arg1 + n);
> +        get_user_ual(protocol, arg1 + 2 * n);
> +        gemu_log("socket(");
> +        print_socket_domain(domain);
> +        gemu_log(",");
> +        print_socket_type(type);
> +        gemu_log(",");
> +        if (domain == AF_PACKET ||
> +            type == TARGET_SOCK_PACKET) {
> +            protocol = tswapal(protocol); /* restore network endian long */
> +            protocol = abi_ntohl(protocol); /* a host endian long */

This doesn't seem to match the kind of byteswapping we do in the
syscall.c code, which just does a tswap16().

> +        }
> +        print_socket_protocol(domain, type, protocol);
> +        gemu_log(")");
> +        break;
> +    }

thanks
-- PMM