Re: [PATCH v2 1/4] linux-user: Use `qemu_log' for non-strace logging

[Laurent Vivier]

Le 17/01/2020 à 20:28, Josh Kunz a écrit :
> Since most calls to `gemu_log` are actually logging unimplemented features,
> this change replaces most non-strace calls to `gemu_log` with calls to
> `qemu_log_mask(LOG_UNIMP, ...)`.  This allows the user to easily log to
> a file, and to mask out these log messages if they desire.
> 
> Note: This change is slightly backwards incompatible, since now these
> "unimplemented" log messages will not be logged by default.

This is a good incompatibility as these messages were unexpected by  the
tools catching stderr. They don't happen on "real" systems.

...
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 249e4b95fc..629f3a21b5 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1545,20 +1545,18 @@ static inline abi_long target_to_host_cmsg(struct 
> msghdr *msgh,
>              - sizeof(struct target_cmsghdr);
>  
>          space += CMSG_SPACE(len);
> -        if (space > msgh->msg_controllen) {
> -            space -= CMSG_SPACE(len);
> -            /* This is a QEMU bug, since we allocated the payload
> -             * area ourselves (unlike overflow in host-to-target
> -             * conversion, which is just the guest giving us a buffer
> -             * that's too small). It can't happen for the payload types
> -             * we currently support; if it becomes an issue in future
> -             * we would need to improve our allocation strategy to
> -             * something more intelligent than "twice the size of the
> -             * target buffer we're reading from".
> -             */
> -            gemu_log("Host cmsg overflow\n");
> -            break;
> -        }
> +
> +        /*
> +         * This is a QEMU bug, since we allocated the payload
> +         * area ourselves (unlike overflow in host-to-target
> +         * conversion, which is just the guest giving us a buffer
> +         * that's too small). It can't happen for the payload types
> +         * we currently support; if it becomes an issue in future
> +         * we would need to improve our allocation strategy to
> +         * something more intelligent than "twice the size of the
> +         * target buffer we're reading from".
> +         */
> +        assert(space > msgh->msg_controllen && "Host cmsg overflow");
>  
>          if (tswap32(target_cmsg->cmsg_level) == TARGET_SOL_SOCKET) {
>              cmsg->cmsg_level = SOL_SOCKET;

Could you move this to a separate patch: you are not using qemu_log()
here and I'm not convinced that crashing is better than ignoring the
remaining part of the buffer.

For the other changes:

Reviewed-by: Laurent Vivier <address@hidden>

Thanks,
Lauren