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

[Alex Bennée]

Laurent Vivier <address@hidden> 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 <address@hidden>
>
> Thanks,
> Lauren


-- 
Alex Bennée