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

[Jan Kiszka <jan.kiszka@siemens.com>]

Mark McLoughlin wrote:
> On Fri, 2009-05-08 at 12:34 +0200, Jan Kiszka wrote:
>> With the internal IP configuration made more flexible, we can now
>> enhance the user interface. This patch adds a number of new options to
>> "-net user": net (address and mask), host, dhcpstart, dns and smbserver.
>> It also renames "redir" to "hostfwd" and "channel" to "guestfwd" in
>> order to (hopefully) clarify their meanings. The format of guestfwd is
>> extended so that the user can define not only the port but also the
>> virtual server's IP address the forwarding starts from.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> ....
> 
>> @@ -1988,15 +2117,21 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
>>  #ifdef CONFIG_SLIRP
>>      if (!strcmp(device, "user")) {
>>          static const char * const slirp_params[] = {
>> -            "vlan", "name", "hostname", "restrict", "ip", "tftp", "bootfile",
>> -            "smb", "redir", "channel", NULL
>> +            "vlan", "name", "hostname", "restrict", "net", "host",
>> +            "tftp", "bootfile", "dhcpstart", "dns", "smb", "smbserver",
>> +            "hostfwd", "guestfwd", NULL
> 
> 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.

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

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

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux