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

[Peter Maydell]

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. 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 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.

> 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. 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.

thanks
-- PMM