From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43407) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZePVl-0007Fk-Ju for qemu-devel@nongnu.org; Tue, 22 Sep 2015 11:29:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZePVh-0003qO-GD for qemu-devel@nongnu.org; Tue, 22 Sep 2015 11:29:41 -0400 Received: from mx4-phx2.redhat.com ([209.132.183.25]:50209) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZePVh-0003qE-A6 for qemu-devel@nongnu.org; Tue, 22 Sep 2015 11:29:37 -0400 Date: Tue, 22 Sep 2015 11:29:01 -0400 (EDT) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <2013259271.15250046.1442935741847.JavaMail.zimbra@redhat.com> In-Reply-To: <5601606B.1040709@huawei.com> References: <1442333283-13119-1-git-send-email-marcandre.lureau@redhat.com> <1442333283-13119-7-git-send-email-marcandre.lureau@redhat.com> <5601606B.1040709@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 06/46] ivshmem: remove unnecessary dup() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Claudio Fontana Cc: marcandre lureau , drjones@redhat.com, cam@cs.ualberta.ca, qemu-devel@nongnu.org, stefanha@redhat.com Hi ----- Original Message ----- > On 15.09.2015 18:07, marcandre.lureau@redhat.com wrote: > > From: Marc-Andr=C3=A9 Lureau > >=20 > > qemu_chr_fe_get_msgfd() transfers ownership, there is no need to dup th= e > > fd. > >=20 >=20 > Are you sure? (tested?) > There is a specific comment that the dup is done because of the get_msgfd= s > implementation, > I checked tcp_get_msgfds and it closes unused fds, is this ok? >=20 > static int tcp_get_msgfds(...) > { > ... > /* Close unused fds */ > for (i =3D to_copy, i < s->read_msgfds_num; i++) { > close(s->read_msgfds[i]); > } > ... Yes, it closes the remaining fds. But for the others, the one it got, there= is no need for an extra dup() since the ownership is transfered. >=20 > > Signed-off-by: Marc-Andr=C3=A9 Lureau > > --- > > hw/misc/ivshmem.c | 21 ++++++--------------- > > 1 file changed, 6 insertions(+), 15 deletions(-) > >=20 > > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > > index dd15f0e..fbeb731 100644 > > --- a/hw/misc/ivshmem.c > > +++ b/hw/misc/ivshmem.c > > @@ -480,7 +480,7 @@ static bool fifo_update_and_get(IVShmemState *s, co= nst > > uint8_t *buf, int size, > > static void ivshmem_read(void *opaque, const uint8_t *buf, int size) > > { > > IVShmemState *s =3D opaque; > > - int incoming_fd, tmp_fd; > > + int incoming_fd; > > int guest_max_eventfd; > > long incoming_posn; > > =20 > > @@ -495,21 +495,21 @@ static void ivshmem_read(void *opaque, const uint= 8_t > > *buf, int size) > > } > > =20 > > /* pick off s->server_chr->msgfd and store it, posn should accompa= ny > > msg */ > > - tmp_fd =3D qemu_chr_fe_get_msgfd(s->server_chr); > > - IVSHMEM_DPRINTF("posn is %ld, fd is %d\n", incoming_posn, tmp_fd); > > + incoming_fd =3D qemu_chr_fe_get_msgfd(s->server_chr); > > + IVSHMEM_DPRINTF("posn is %ld, fd is %d\n", incoming_posn, > > incoming_fd); > > =20 > > /* make sure we have enough space for this guest */ > > if (incoming_posn >=3D s->nb_peers) { > > if (increase_dynamic_storage(s, incoming_posn) < 0) { > > error_report("increase_dynamic_storage() failed"); > > - if (tmp_fd !=3D -1) { > > - close(tmp_fd); > > + if (incoming_fd !=3D -1) { > > + close(incoming_fd); > > } > > return; > > } > > } > > =20 > > - if (tmp_fd =3D=3D -1) { > > + if (incoming_fd =3D=3D -1) { > > /* if posn is positive and unseen before then this is our posn= */ > > if ((incoming_posn >=3D 0) && > > (s->peers[incoming_posn].eventfds =3D=3D N= ULL)) { > > @@ -524,15 +524,6 @@ static void ivshmem_read(void *opaque, const uint8= _t > > *buf, int size) > > } > > } > > =20 > > - /* because of the implementation of get_msgfd, we need a dup */ > > - incoming_fd =3D dup(tmp_fd); > > - > > - if (incoming_fd =3D=3D -1) { > > - error_report("could not allocate file descriptor %s", > > strerror(errno)); > > - close(tmp_fd); > > - return; > > - } > > - > > /* if the position is -1, then it's shared memory region fd */ > > if (incoming_posn =3D=3D -1) { > > =20 > >=20 >=20 >=20