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

[Laurent Vivier <laurent@vivier.eu>]

Le 28/01/2020 à 17:53, Alex Bennée a écrit :
> 
> Laurent Vivier <laurent@vivier.eu> writes:
> 
>> 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");

Should it be in fact :

  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.
> 
> I suggested it should be an assert in the first place. It certainly
> makes sense to keep it in a separate patch though. I guess you could
> argue for:
> 
>   qemu_log_mask(LOG_UNIMP, "%s: unhandled message size");
> 
> but is it really better to partially work and continue? It seems like
> you would get more subtle hidden bugs.

ok, you're right. crash seems to be a better solution.

So, we only need to move this change to a separate patch.

Thanks,
Laurent