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

[Paul Burton <paul@archlinuxmips.org>]

[-- Attachment #1: Type: text/plain, Size: 3285 bytes --]

On Tue, Jun 24, 2014 at 12:21:42AM +0100, Peter Maydell wrote:
> On 24 June 2014 00:06, Paul Burton <paul@archlinuxmips.org> wrote:
> > On Mon, Jun 23, 2014 at 11:35:17PM +0100, Peter Maydell wrote:
> >> and so I'm dubious about a patch that's
> >> trying to make a very small change to it
> >
> > Isn't that precisely how good bisectable bug fixes should be made?
> 
> The key is in the second half of this sentence:
> 
> >> without looking
> >> at the larger picture and testing and fixing bugs on more
> >> than one architecture.
> >
> > I pointed you to the kernel code which dereferences the pointer & it's
> > quite clearly architecture neutral, so I'm not sure what you're getting
> > at with the architecture comment.
> >
> > There's quite clearly a bug in QEMU here, and this patch fixes it. I
> > hope you're not saying you don't want it merged because it doesn't fix
> > some other hypothetical bugs in the same area.
> 
> What I mean is that I would expect that any attempt to fix
> behaviour in this area ought to result in a series of three
> or four patches which fix various bugs (of which this
> would just be one). When an area of code is pretty
> clearly bogus like this one, then in my experience an
> attempt to make a small fix to it without just going ahead
> and overhauling it is likely to result in accidentally
> breaking existing working uses which happened to work
> more or less by fluke. This is particularly true if you only
> test one guest architecture; you can reasonably make that
> level of limited testing in areas where the codebase is
> sane, but not where it is clearly dubious.
> 
> So yes, I would prefer this not to be merged unless either
> (a) it comes as part of a series that cleans up the code for
> handling semctl so it's not obviously broken
> (b) it has been tested to confirm that it doesn't obviously
> regress any guest architecture (or at least not any of the
> more important ones),
> and ideally both.
> 
> I don't think this is an enormous amount of work (maybe a
> couple of days?); I'm really just recommending the approach
> and amount of cleanup that I would do if I found I needed
> to make a fix in this area myself.

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?

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?

I traced the issue with fakeroot then compared the code & behaviour of
QEMU with that of Linux. I found a difference. I fixed it. I checked
that the kernel code for this is architecture neutral. So as far as I'm
aware this patch fixes a bug and QEMU would be better with this patch
than it is without it.

But anyway, please do enlighten me: where are the bugs of which you
speak? I'd like to see them fixed too :)

Thanks,
    Paul

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]