[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 10/11] slirp: Rework external configuration inte
From: |
Mark McLoughlin |
Subject: |
Re: [Qemu-devel] [PATCH 10/11] slirp: Rework external configuration interface |
Date: |
Thu, 28 May 2009 18:23:14 +0100 |
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.
[Qemu-devel] [PATCH 05/11] net: Improve parameter error reporting, Jan Kiszka, 2009/05/08
[Qemu-devel] [PATCH 11/11] slirp: Bind support for host forwarding rules, Jan Kiszka, 2009/05/08
Re: [Qemu-devel] [PATCH 00/11] Networking fixes and slirp enhancements, Anthony Liguori, 2009/05/08