Re: [Qemu-devel] [PATCH] linux-user: fix wait* syscall status returns

[Alexander Graf <agraf@suse.de>]

On 23.11.2011, at 23:34, Peter Maydell wrote:

> On 23 November 2011 22:03, Alexander Graf <agraf@suse.de> wrote:
>> On 23.11.2011, at 22:55, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> If the problem is that waitpid() can return success without writing to
>>> status, then this code is still not right because we will get the
>>> initial target waitstatus into status, and then pass it through
>>> host_to_target_waitstatus(), possibly modifying it, before writing
>>> it back to guest memory.
>> 
>> Yes. Maybe we should add a check if input_state != output_state and
>> only then do the conversion?
> 
> I'm not sure this works (unless you go to the effort of implementing
> target_to_host_waitstatus() which seems overkill to me) but I'm not
> entirely sure what you're proposing to compare to what.

It's an integer. If the number is the same before and after the wait syscall, we can safely assume that it's the same thing, so we don't have to convert it, no?

> 
>>> I think waitpid() will always and only write to status if the return
>>> value is > 0 (ie it's a PID, not 0 or -1). So I think the right fix
>>> for this problem is to have the if() protecting the put_user_s32()
>>> read "if (ret && !is_error(ret) && ...".
>>> 
>>> (ret == 0 is of course the WNOHANG-and-no-child-yet case you are hitting.)
>> 
>> The man page wasn't really clear here. It sounded as if you can also
>> get 0 as return value and still have status change. That's why I
>> jumped through this hoop in the first place :)
> 
> I think POSIX is clear enough. Either
> * a child status is available, return pid and write to status
> * WNOHANG & not ready, return 0 (don't write to status)
> * -1 and set errno (don't write to status)
> 
> (strictly, whether status is written for EINTR is unpredictable, but I
> think we can assume nobody relies on this and I bet the kernel folks
> wouldn't worry too much about changing that between versions...)

Hrm, ok. I'll change it to your version then.


Alex