Laurent Vivier <laur...@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");
>>
>> 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.
>
> For the other changes:
>
> Reviewed-by: Laurent Vivier <laur...@vivier.eu>
>
> Thanks,
> Lauren
--
Alex Bennée
