Re: [Qemu-devel] [PATCH 10/11] slirp: Rework external configuration interface

[Mark McLoughlin]

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.