On 8 June 2016 at 21:24, Laurent Vivier <laur...@vivier.eu> wrote:
> From: Laurent Vivier <laur...@vivier.eu>
>
> Signed-off-by: Laurent Vivier <laur...@vivier.eu>
> ---
> 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
