Re: [Qemu-devel] [PATCH 00/13] __{get, put}_user return value cleanup

[Riku Voipio]

On Wed, Apr 23, 2014 at 02:22:32PM +0100, Peter Maydell wrote:
> On 23 April 2014 14:11,  <address@hidden> wrote:
> > From: Riku Voipio <address@hidden>
> >
> > This series is primarily motivated to have a gcc-4.9 buildfix:
> >
> > linux-user/syscall.c: In function ‘host_to_target_stat64’:
> > linux-user/qemu.h:301:19: error: right-hand operand of comma expression has 
> > no effect [-Werror=unused-value]
> >       ((hptr), (x)), 0)
> >
> > removing the unused 0 moves the bar:
> >
> > linux-user/main.c: In function ‘arm_kernel_cmpxchg64_helper’:
> > linux-user/qemu.h:330:15: error: void value not ignored as it ought to be
> >          __ret = __put_user((x), __hptr);    \
> >
> > And after fixing that, we see there is a lot of reading the return
> > value of __put_user and __get_user in signal.c - apparently without
> > much of consistency as other functions do check and others don't...
> >
> > I'm not 100% sure that simply removing the checks is right way
> > and signal.c is quite a mess, so push this set early for comments
> > before spending more time on this approach.
> 
> So the reason for this inconsistency is that QEMU's approach
> to checking that memory is accessible differs from the Linux kernel.
> In Linux, the check effectively happens on each get/put access
> (for free courtesy of the MMU), so the __get/__put_user accessors
> can each individually succeed or fail, and the return value must
> always be checked.

Right. I guess this will be good to put the commit message 
or code comment in the 12/12 patch.

> In QEMU, we do an access check for an entire region via
> lock_user (or lock_user_struct) and catch the failure case there.
> So our __get/__put_user accessors can never fail. However we
> retained the return value because a lot of the code in signal.c
> is pretty much copy-and-pasted from the kernel sources with
> minor tweaks, and so it includes the "gather up and check
> return values at each step" code.

Ok, this explains. Thanks for quick answer!

> Generally in code review or if I've been tidying up or fixing the
> signal handling code for some other reason I've removed the
> pointless accumulation of return values. But a bunch of our
> less-maintained architectures still have it.

> >   signal.c: remove __get/__put_user return value reading
> >   signal.c setup_frame/x86: __put_user cleanup
> >   signal.c: remove return value from copy_siginfo_to_user
> >   signal.c: remove return value from setup_sigcontext
> >   signal.c: remove return value from restore_sigcontext
> >   RFC comment out restore_fpu_state (sparc)
> >   do_sigaltstack: remove __get_user value check
> >   do_sigreturn - remove __get_user checks
> >   signal.c: setup_frame remove __put_user checks
> >   remove __put/get error checks from ppc {save,restore}_user_regs
> >   sparc64_set_context: remove __get_user checks
> >   remove __get_user return check from PPC do_setcontext
> >   fix gcc-4.9 compiler error on __{get,put]}_user
> 
> Given that signal.c is a mix of code for all our supported
> architectures, it would be nice if these patch summary lines
> were a bit more consistent:
>  linux-user/signal.c: Generic change affecting all archs
>  linux-user/signal.c: PPC: Change affecting only PPC

Ok, no problem. As said, I wanted to see that the principle,
patch split etc is acceptable before going to detailed polishing
like summary lines. 

> and so on.
 
> I should probably have another go at a tidyup I was
> looking at ages ago which splits linux-user/signal.c
> into linux-user/$arch/signal.c...

Yes, it was in my mind when repeatedly compiling all archs
for any single change in signal.c...

Riku