Re: [Qemu-devel] [PATCH v4] net: add the support for -netdev socket, listen

[Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>]

On Wed, Jun 06, 2012 at 09:26:23PM +0800, zwu.kernel@gmail.com wrote:
> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
> The -net socket,listen option does not work with the newer -netdev
> syntax:
>  http://lists.gnu.org/archive/html/qemu-devel/2011-11/msg01508.html
> 
> This patch makes it work now.
> 
> For the case where one vlan has multiple listenning sockets,
> the patch will also provide the support.
> 
> Supported syntax:
>  1.) -net socket,listen=127.0.0.1:1234,vlan=0
>  2.) -net socket,listen=127.0.0.1:1234,vlan=0 -net socket,listen=127.0.0.1:1235,vlan=0
>  3.) -netdev socket,listen=127.0.0.1:1234,id=socket0
> 
> Suggested-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> ---
>  net.c        |   24 +++++++++++
>  net.h        |    3 +
>  net/socket.c |  123 ++++++++++++++++++++++++++++++++++++++++++---------------
>  3 files changed, 117 insertions(+), 33 deletions(-)

Please include changelogs in new versions of patches.

It looks like you have implemented "1 connection at a time" semantics.
This is good, I think it maps best to the netdev peer model.  Allowing
multiple clients to connect to a single listen socket at the same time
doesn't fit into the netdev peer model.

I think the patch can be simplified a lot though.  There's no need to
modify net.c or add consumed booleans.

Instead, drop the NetSocketListenState struct and add a listen_fd field
to NetSocketState.  When a -netdev socket,listen= instance is created
there will be a NetSocketState with fd=-1 and a valid listen_fd.  The
net_socket_accept() handler waits for listen_fd to become readable and
then accepts the connection.  When this state transition happens, we no
longer monitor listen_fd for incoming connections...until the client
disconnects again.

This approach doesn't need to change net.c or VLANClientState.  It also
makes memory allocation simpler because we only have 1 struct:
NetSocketState.

Stefan