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

[Josh Kunz]

I've switched it to a LOG_UNIMP, similar to to the one several lines
below. I will follow up with a change to switch this to an assert as
recommended.


On Tue, Jan 28, 2020 at 9:07 AM Laurent Vivier <address@hidden> wrote:
>
> Le 28/01/2020 à 17:53, Alex Bennée a écrit :
> >
> > 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");
>
> 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
>