On Tue, Jun 24, 2014 at 12:21:42AM +0100, Peter Maydell wrote: > On 24 June 2014 00:06, Paul Burton <p...@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
signature.asc
Description: Digital signature
