Re: [Qemu-devel] [PATCH] linux-user: fix ipc(SEMCTL, ...) argument handling

[Paul Burton]

On Tue, Jun 24, 2014 at 09:19:45AM +0100, Peter Maydell wrote:
> On 24 June 2014 00:53, Paul Burton <address@hidden> wrote:
> > Well I disagree with your logic, but perhaps that's primarily because of
> > your claim that the semctl code is "clearly bogus" and "obviously
> > broken". Could you back that up? I know there's the one bogus line in
> > the GETVAL/SETVAL case that was mentioned in another email, but is there
> > anything else you consider broken?
> 
> The type of the parameter we pass doesn't match what the
> kernel does, there's been at least one previous attempt to
> fix stuff in this area, and as you say the getval/setval stuff
> is broken.

Thanks, that's useful.

I'll change the code to use ulong instead of union target_semun if
that'll make you happier, but that is simply moving casting down a
level into do_semctl and won't change the way this runs.

I'm not aware of GETVAL & SETVAL being broken in the sense of not
working when used by target programs. There's one bogus line in the
code which as far as I can see is harmless with the exception of making
the code unclear to readers. I'm happy to submit a patch to remove that
line.

I can see that the fact that someone previously attempted, incorrectly,
to fix a bug in this code may make you doubt its correctness and be wary
of further patches to the code. What I disagree with is the notion that
a bug fix shouldn't be merged until the code has been thoroughly
examined for further bugs.

> That's three things, which means (to my mind) that
> the first thing that needs to be done is examine the whole
> routine and determine how it ought to work, independent of
> what it happens to be doing now. Then you can implement the
> necessary fixes. I *don't* think this is a big job, or even that the
> code needs to be completely rewritten.

I agree it's a good idea for that to be done, and I'll do it when I get
the time. I disagree that it means this bug fix shouldn't be merged, but
there's no sense arguing about it so I'll leave it there and Riku can do
as he wishes.

> > I see your point on testing, but frankly this code is generic for all
> > architectures in Linux. I don't have the time to test each architecture
> > and I don't have the time to test all software that may have previously
> > been working by fluke. So what's the bar you'd like to set here?
> 
> Riku's the submaintainer here, so it's his decision in the end
> (and I think he has a set of tests he runs on patches as well),
> but one of the bars I have for reviewing patches is that I
> should be reasonably confident it won't regress existing
> behaviour for anybody. A simple patch to existing clear and
> correct code gives me that confidence; a set of patches that
> take a holistic approach to an obvious trouble spot do too;
> a pile of testing ditto. A tiny point fix added to something we
> know to be dubious doesn't give any confidence that it's
> going to interact correctly with the dubious code.

That's the thing, you say the code is known to be dubious (previously
"bogus" and "broken"), but then when I ask you to back that up I get
this:

> > But anyway, please do enlighten me: where are the bugs of which you
> > speak? I'd like to see them fixed too :)
> 
> You're essentially asking me to do the work for you.

That's not what I intend at all. I simply wanted to know why you
consider the code bogus & broken. Hopefully the reasons are addressed
above.

> This is a recipe for me to say "sorry, I don't think we should
> accept your patch" -- it's you that has a reason to get
> this patch accepted, not me.

The only reason I'd like it merged is that I want QEMU to work better,
for the good of all who use it. I'd hope that's true of everyone working
on it and in that sense I have no more investment in this patch being
merged than anyone else. I'm not being paid to do this, and my build of
QEMU works better than it did previously.

Anyway things seem to be getting a little heated here which is probably
not a productive path forwards, so I'll leave this alone until I write
the GETVAL/SETVAL patch & look through the rest of the code.

Paul

signature.asc
Description: Digital signature