From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1M9mQ4-0005AS-2d for qemu-devel@nongnu.org; Thu, 28 May 2009 16:41:44 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1M9mPy-00058v-Br for qemu-devel@nongnu.org; Thu, 28 May 2009 16:41:42 -0400 Received: from [199.232.76.173] (port=45703 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1M9mPy-00058o-3z for qemu-devel@nongnu.org; Thu, 28 May 2009 16:41:38 -0400 Received: from mx20.gnu.org ([199.232.41.8]:1615) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1M9mPx-0008JA-Lv for qemu-devel@nongnu.org; Thu, 28 May 2009 16:41:37 -0400 Received: from fmmailgate02.web.de ([217.72.192.227]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1M9mPw-0001Tp-I5 for qemu-devel@nongnu.org; Thu, 28 May 2009 16:41:36 -0400 Message-ID: <4A1EF6FA.4050102@web.de> Date: Thu, 28 May 2009 22:41:30 +0200 From: Jan Kiszka MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 10/11] slirp: Rework external configuration interface References: <20090508103416.6080.44298.stgit@mchn012c.ww002.siemens.net> <20090508103418.6080.46645.stgit@mchn012c.ww002.siemens.net> <1243523247.4046.197.camel@blaa> <4A1EB3D4.7030804@siemens.com> <1243531394.18588.63.camel@blaa> In-Reply-To: <1243531394.18588.63.camel@blaa> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enigED3A525C0A497B929F1333E7" Sender: jan.kiszka@web.de List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mark McLoughlin Cc: qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigED3A525C0A497B929F1333E7 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Mark McLoughlin wrote: > On Thu, 2009-05-28 at 17:55 +0200, Jan Kiszka wrote: >>> You're renaming "redir" and "channel" here which isn't a big deal sin= ce >>> you've only introduced them in the previous patch, but it would be >>> better to use the final names in the original patch. >> Well, the purpose of the original patch was only to pull "redir" and >> "channel" as-is into the -net parameter list, not to refactor the >> interface otherwise. >=20 > It's a bisectability thing - we shouldn't introduce parameters in a > patch that we intend to rename in a subsequent patch. I do not only rename them, I also change their syntax. >=20 > e.g. if a distro cherry-picked this patch, we'd have options in the wil= d > that aren't available upstream. If distros cherry-pick transient public interface changes, I really can't help - or I would consequently have to merge internal, external interface rework and the final hostfwd syntax extension into a single patch. I don't think this is desired either. >=20 >>> More importantly, though, you've dropped the "ip" parameter which is >> in >>> the 0.10.x releases. We can't just drop existing parameters. >> OK, I see the problem - though this parameter was probable rarely used= >> (it was undocumented and suffered from the poor configurability). Will= >> have a closer look. >=20 > Great. Probably best to re-send 7-11 with both of these things fixed up= =2E >=20 >>> By way of meta-comment, some of these patches are much harder to >> review >>> than they need to be, because e.g. you're doing lots of cleanups >>> together with the real changes, or not breaking changes into smaller >>> chunks. >> There might be a few coding style updates included, when I touch >> related >> lines. Or please give some (or the most annoying) example. >=20 > Okay: >=20 > - In "Avoid zombie processes after fork_exec" you needlessly=20 > re-arrange the code in launch_script() - it could have very=20 > reasonably been an easy to review +6 hunk rather than a -24/+36=20 > hunk if you'd split the cleanup out into its own patch >=20 > - "Real fix for check_params users" could have been split into the > revert followed by the better fix >=20 > - In "Improve parameter error reporting", you replace a strdup() with= =20 > qemu_strdup(), unrelated to the goal of the patch >=20 > - In "Reorder initialization", you change the formatting of the args = > to the qemu_new_vlan_client() call - conflicted with my patches >=20 > - In the same patch you've added a whole pile of braces in=20 > what was net_slirp_redir() - made re-basing onto Alexander's slirp = > stuff that bit more involved >=20 Agreed (though the last conflict was not only related to that hunk), some could have just been dropped, others split up without too much effor= t. > - You could have split up "slirp: Move smb, redir, tftp and bootp > parameters and -net channel", maybe even made a separate patch for = > each move But that would have been overkill (they share a lot). >=20 > - It's very hard to understand why each of the changes in "slirp: = > Rework internal configuration" is needed and it's a big patch -=20 > e.g. you completely re-wrote tcp_ctl(). Could that have been done=20 > with a cleanup patch with no functional changes followed by the=20 > actual functional changes? Yes, tcp_ctl could have been sanitized beforehand. But the rest is related to scope of the patch. >=20 > - "slirp: Rework external configuration interface" introduces several= =20 > new slirp options. Surely should be possible to split up into=20 > smaller patches. Not impossible, but significant additional effort. Thanks for the detailed comments! Will try harder to avoid the needless mixups in the future. Jan --------------enigED3A525C0A497B929F1333E7 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.9 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org iEYEARECAAYFAkoe9v4ACgkQniDOoMHTA+kv8QCeKFUy5fi62fiWGSitO7aVwraq p9IAnjUhHSMLbZQ1w4MrAIrha7Rme2Uc =XBTQ -----END PGP SIGNATURE----- --------------enigED3A525C0A497B929F1333E7--