From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:40384)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lersek@redhat.com>) id 1Stq2k-0001R0-2J
	for qemu-devel@nongnu.org; Tue, 24 Jul 2012 21:05:38 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <lersek@redhat.com>) id 1Stq2j-0000gY-47
	for qemu-devel@nongnu.org; Tue, 24 Jul 2012 21:05:38 -0400
Received: from mx1.redhat.com ([209.132.183.28]:60632)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lersek@redhat.com>) id 1Stq2i-0000ej-Rt
	for qemu-devel@nongnu.org; Tue, 24 Jul 2012 21:05:37 -0400
Message-ID: <500F469B.9060302@redhat.com>
Date: Wed, 25 Jul 2012 03:06:35 +0200
From: Laszlo Ersek <lersek@redhat.com>
MIME-Version: 1.0
References: <1343144119-18757-1-git-send-email-stefanha@linux.vnet.ibm.com>
In-Reply-To: <1343144119-18757-1-git-send-email-stefanha@linux.vnet.ibm.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v2 00/16] net: Move legacy QEMU VLAN code
	into net/hub.c
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, qemu-devel@nongnu.org, Zhi Yong Wu <wuzhy@cn.ibm.com>

I'd like to review v2 tomorrow (ahem, today), ie. not right now, but
this catches my eye:

On 07/24/12 17:35, Stefan Hajnoczi wrote:

>  * Drop spurious closesocket(fd), probably merge conflict [Stefan]

I saw it last time and I thought you had fixed a leak with it on the
side. The function looks as follows (right after applying [01 .. 02]
from v1):

[net/socket.c]

  376  static void net_socket_accept(void *opaque)
  377  {
  378      NetSocketListenState *s = opaque;
  379      NetSocketState *s1;
  380      struct sockaddr_in saddr;
  381      socklen_t len;
  382      int fd;
  383
  384      for(;;) {
  385          len = sizeof(saddr);
  386          fd = qemu_accept(s->fd, (struct sockaddr *)&saddr, &len);
  387          if (fd < 0 && errno != EINTR) {
  388              return;
  389          } else if (fd >= 0) {
  390              break;
  391          }
  392      }
  393      s1 = net_socket_fd_init(s->peer, s->model, s->name, fd, 1);
  394      if (!s1) {
  395          closesocket(fd);
  396      } else {
  397          snprintf(s1->nc.info_str, sizeof(s1->nc.info_str),
  398                   "socket: connection from %s:%d",
  399                   inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
  400      }
  401  }

If net_socket_fd_init() fails, then the file descriptor extracted by
qemu_accept() should be closed. The fix is not strongly related to the
series but I figured I'd let it slide :)

... Oh. net_socket_fd_init() takes ownership even if it fails, in which
case it calls closesocket() itself.

Such functions need a banner :)

Laszlo