From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59902) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WzE3s-0000N8-85 for qemu-devel@nongnu.org; Mon, 23 Jun 2014 19:54:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WzE3m-0002nx-9J for qemu-devel@nongnu.org; Mon, 23 Jun 2014 19:54:08 -0400 Received: from mail-we0-x233.google.com ([2a00:1450:400c:c03::233]:40327) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WzE3l-0002nn-Vq for qemu-devel@nongnu.org; Mon, 23 Jun 2014 19:54:02 -0400 Received: by mail-we0-f179.google.com with SMTP id w62so7998865wes.10 for ; Mon, 23 Jun 2014 16:54:01 -0700 (PDT) Sender: Paul Burton Date: Tue, 24 Jun 2014 00:53:56 +0100 From: Paul Burton Message-ID: <20140623235356.GG4377@gmail.com> References: <1403559614-4096-1-git-send-email-paul@archlinuxmips.org> <20140623221825.GC4377@gmail.com> <20140623230602.GE4377@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="WBsA/oQW3eTA3LlM" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH] linux-user: fix ipc(SEMCTL, ...) argument handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Riku Voipio , QEMU Developers , Paul Burton --WBsA/oQW3eTA3LlM Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jun 24, 2014 at 12:21:42AM +0100, Peter Maydell wrote: > On 24 June 2014 00:06, Paul Burton 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? >=20 > The key is in the second half of this sentence: >=20 > >> 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. >=20 > 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. >=20 > 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. >=20 > 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 --WBsA/oQW3eTA3LlM Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJTqL4UAAoJENzvn0paErs5MXYQAKY+7VqS2jumVh9he/jLnhSw /rUxihVzYS9Q+tmuyp4yt6JfEwLcO9hoE2NTBeXCMnn2MkWUxCw1nJF7xFxujiQC 1CUG7uUG2Lu3kdBIkBV4x5tWDtDKSUAIBQc6DFIcpm2ZeLN+7PnIqXT6aEUCLp/Q OjRfPskLsbWVncoZj2tQ3daS4cfNSzkKt4+A663iy0/EqZWNJwxchENC4QwkWTiS 8uQRm3nCBozAjZQHzz9qLbLgUMbDGgc1KkJN8i8LgLSIGe9hP+RqxuVC82rqZSvI liCJFi4VIR/3LacFXYuQldXrbJme5KCvkpV9cth67Owjoh0mD02LRME81L4iqnGy QX+J8Z0EIfLjjvOOp3VqiHX8PgIu5JWJcP6DFEv701j0BRch9P7u0QYo4Q2oElQv V/RynGvW4x7NVhkXhZMBzgBxqnzbwvIXdMRvk/UicVdz5mvcTA1rUCregqCjlnsk F59TGkFHimeDjcIVGvK585jChVH8vneM3Q7P/8/TnFRUlyWnuSmaXyU6eUvymwuh EJJox7IOQVveq3ilbVjyWbxMUzca8nEQ1sz1Qvhf1Ibuklk0mkJzxKMS8nkH/XOi 9LJfoIwLHDkcFcwgYYUq90wnRCC4pEBEiglJlwBevYe4D+a+cioRoV1Wb+8ULrKa oZAAynoilz0thMMuCm54 =SFce -----END PGP SIGNATURE----- --WBsA/oQW3eTA3LlM--