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

[Jan Kiszka]

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 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.

I do not only rename them, I also change their syntax.

> 
> e.g. if a distro cherry-picked this patch, we'd have options in the wild
> 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.

> 
>>> 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
> 

Agreed (though the last conflict was not only related to that hunk),
some could have just been dropped, others split up without too much effort.

>   - 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).

> 
>   - 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?

Yes, tcp_ctl could have been sanitized beforehand. But the rest is
related to scope of the patch.

> 
>   - "slirp: Rework external configuration interface" introduces several 
>     new slirp options. Surely should be possible to split up into 
>     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

signature.asc
Description: OpenPGP digital signature