From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1M9hCj-0004Gi-S7
	for qemu-devel@nongnu.org; Thu, 28 May 2009 11:07:37 -0400
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1M9hCf-0004Fl-1v
	for qemu-devel@nongnu.org; Thu, 28 May 2009 11:07:37 -0400
Received: from [199.232.76.173] (port=44328 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1M9hCe-0004Fi-RH
	for qemu-devel@nongnu.org; Thu, 28 May 2009 11:07:32 -0400
Received: from mx2.redhat.com ([66.187.237.31]:33328)
	by monty-python.gnu.org with esmtp (Exim 4.60)
	(envelope-from <markmc@redhat.com>) id 1M9hCe-0005zf-2D
	for qemu-devel@nongnu.org; Thu, 28 May 2009 11:07:32 -0400
Subject: Re: [Qemu-devel] [PATCH 10/11] slirp: Rework external
	configuration interface
From: Mark McLoughlin <markmc@redhat.com>
In-Reply-To: <20090508103418.6080.46645.stgit@mchn012c.ww002.siemens.net>
References: <20090508103416.6080.44298.stgit@mchn012c.ww002.siemens.net>
	<20090508103418.6080.46645.stgit@mchn012c.ww002.siemens.net>
Content-Type: text/plain
Date: Thu, 28 May 2009 16:07:27 +0100
Message-Id: <1243523247.4046.197.camel@blaa>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Reply-To: Mark McLoughlin <markmc@redhat.com>
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Jan Kiszka <jan.kiszka@siemens.com>
Cc: qemu-devel@nongnu.org

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.

More importantly, though, you've dropped the "ip" parameter which is in
the 0.10.x releases. We can't just drop existing parameters.

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.

Cheers,
Mark.