From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1M9jK6-0006eS-Rc for qemu-devel@nongnu.org; Thu, 28 May 2009 13:23:22 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1M9jK2-0006dJ-6S for qemu-devel@nongnu.org; Thu, 28 May 2009 13:23:22 -0400 Received: from [199.232.76.173] (port=39239 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1M9jK2-0006dG-0o for qemu-devel@nongnu.org; Thu, 28 May 2009 13:23:18 -0400 Received: from mx2.redhat.com ([66.187.237.31]:32905) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1M9jK1-0002BZ-DG for qemu-devel@nongnu.org; Thu, 28 May 2009 13:23:17 -0400 Subject: Re: [Qemu-devel] [PATCH 10/11] slirp: Rework external configuration interface From: Mark McLoughlin In-Reply-To: <4A1EB3D4.7030804@siemens.com> 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> Content-Type: text/plain Date: Thu, 28 May 2009 18:23:14 +0100 Message-Id: <1243531394.18588.63.camel@blaa> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Reply-To: Mark McLoughlin List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: qemu-devel@nongnu.org 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 since > > 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. It's a bisectability thing - we shouldn't introduce parameters in a patch that we intend to rename in a subsequent patch. e.g. if a distro cherry-picked this patch, we'd have options in the wild that aren't available upstream. > > 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. Great. Probably best to re-send 7-11 with both of these things fixed up. > > 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. Okay: - In "Avoid zombie processes after fork_exec" you needlessly re-arrange the code in launch_script() - it could have very reasonably been an easy to review +6 hunk rather than a -24/+36 hunk if you'd split the cleanup out into its own patch - "Real fix for check_params users" could have been split into the revert followed by the better fix - In "Improve parameter error reporting", you replace a strdup() with qemu_strdup(), unrelated to the goal of the patch - In "Reorder initialization", you change the formatting of the args to the qemu_new_vlan_client() call - conflicted with my patches - In the same patch you've added a whole pile of braces in what was net_slirp_redir() - made re-basing onto Alexander's slirp stuff that bit more involved - 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 - It's very hard to understand why each of the changes in "slirp: Rework internal configuration" is needed and it's a big patch - e.g. you completely re-wrote tcp_ctl(). Could that have been done with a cleanup patch with no functional changes followed by the actual functional changes? - "slirp: Rework external configuration interface" introduces several new slirp options. Surely should be possible to split up into smaller patches. Cheers, Mark.