From patchwork Thu Jan 11 12:31:47 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: "Zhoujian (jay)" X-Patchwork-Id: 10157947 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id EFFB160170 for ; Thu, 11 Jan 2018 12:32:54 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0CCE928776 for ; Thu, 11 Jan 2018 12:32:55 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 0177B28783; Thu, 11 Jan 2018 12:32:54 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 23B5528776 for ; Thu, 11 Jan 2018 12:32:53 +0000 (UTC) Received: from localhost ([::1]:39875 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eZc2O-00018Z-Qx for patchwork-qemu-devel@patchwork.kernel.org; Thu, 11 Jan 2018 07:32:52 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45861) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eZc1c-0000hi-GN for qemu-devel@nongnu.org; Thu, 11 Jan 2018 07:32:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eZc1Y-0005PU-Gg for qemu-devel@nongnu.org; Thu, 11 Jan 2018 07:32:04 -0500 Received: from szxga02-in.huawei.com ([45.249.212.188]:2305 helo=huawei.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eZc1X-0005L2-1o for qemu-devel@nongnu.org; Thu, 11 Jan 2018 07:32:00 -0500 Received: from DGGEMM404-HUB.china.huawei.com (unknown [172.30.72.54]) by Forcepoint Email with ESMTP id 8F0139121E42E; Thu, 11 Jan 2018 20:31:52 +0800 (CST) Received: from DGGEMM424-HUB.china.huawei.com (10.1.198.41) by DGGEMM404-HUB.china.huawei.com (10.3.20.212) with Microsoft SMTP Server (TLS) id 14.3.361.1; Thu, 11 Jan 2018 20:31:53 +0800 Received: from DGGEMM505-MBS.china.huawei.com ([169.254.2.114]) by dggemm424-hub.china.huawei.com ([10.1.198.41]) with mapi id 14.03.0361.001; Thu, 11 Jan 2018 20:31:46 +0800 From: "Zhoujian (jay)" To: Jason Wang , "qemu-devel@nongnu.org" Thread-Topic: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize Thread-Index: AQHTiWiucuh7D86UNU6COMFccnb/8aNsd2Iw//+gsYCAAIv5oIAA3U4AgACIZzD//+ubgIAAmpmw Date: Thu, 11 Jan 2018 12:31:47 +0000 Message-ID: References: <1515516013-16604-1-git-send-email-jianjay.zhou@huawei.com> <48f2d18b-b58c-dd08-29ff-34b5f8c2d780@redhat.com> In-Reply-To: Accept-Language: zh-CN, en-US Content-Language: zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.177.19.14] MIME-Version: 1.0 X-CFilter-Loop: Reflected X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [fuzzy] X-Received-From: 45.249.212.188 Subject: Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "Huangweidong \(C\)" , "mst@redhat.com" , "wangxin \(U\)" , "Gonglei \(Arei\)" , "imammedo@redhat.com" , "Liuzhe \(Ahriy, Euler\)" Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP > -----Original Message----- > From: Jason Wang [mailto:jasowang@redhat.com] > Sent: Thursday, January 11, 2018 6:30 PM > To: Zhoujian (jay) ; qemu-devel@nongnu.org > Cc: Huangweidong (C) ; mst@redhat.com; wangxin (U) > ; Gonglei (Arei) ; > imammedo@redhat.com; Liuzhe (Ahriy, Euler) > Subject: Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only > vhost failed to initialize > > > > On 2018年01月11日 11:54, Zhoujian (jay) wrote: > > Hi Jason, > > > >> -----Original Message----- > >> From: Jason Wang [mailto:jasowang@redhat.com] > >> Sent: Thursday, January 11, 2018 11:35 AM > >> To: Zhoujian (jay) ; qemu-devel@nongnu.org > >> Cc: Huangweidong (C) ; mst@redhat.com; > >> wangxin (U) ; Gonglei (Arei) > >> ; imammedo@redhat.com; Liuzhe (Ahriy, Euler) > >> > >> Subject: Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if > >> only vhost failed to initialize > >> > >> > >> > >> On 2018年01月10日 15:39, Zhoujian (jay) wrote: > >>>>>> + *vhost_init_failed = true; > >>>> Why not simply check s->vhost_net after call net_init_tap_one()? > >>> s->vhost_net is always NULL if net_init_tap_one() failed, it can't > >> distinguish > >>> failure reasons. > >> On which condition net_init_tap_one() fail but s->vhost_net is set? > > No, it does not exist, so we could not use s->vhost_net to decide > > whether close the fd of tap when error occurred. > > > > > > Maybe the patch below will be much better to understand, please have a look. > > > > diff --git a/net/tap.c b/net/tap.c > > index 979e622..a5c5111 100644 > > --- a/net/tap.c > > +++ b/net/tap.c > > @@ -642,7 +642,8 @@ static void net_init_tap_one(const NetdevTapOptions > *tap, NetClientState *peer, > > const char *model, const char *name, > > const char *ifname, const char *script, > > const char *downscript, const char > *vhostfdname, > > - int vnet_hdr, int fd, Error **errp) > > + int vnet_hdr, int fd, Error **errp, > > + bool *error_is_set_sndbuf) > > { > > Error *err = NULL; > > TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr); > > @@ -651,6 +652,9 @@ static void net_init_tap_one(const NetdevTapOptions > *tap, NetClientState *peer, > > tap_set_sndbuf(s->fd, tap, &err); > > if (err) { > > error_propagate(errp, err); > > + if (error_is_set_sndbuf) { > > + *error_is_set_sndbuf = true; > > + } > > return; > > } > > > > @@ -748,6 +752,7 @@ int net_init_tap(const Netdev *netdev, const char *name, > > Error *err = NULL; > > const char *vhostfdname; > > char ifname[128]; > > + bool error_is_set_sndbuf = false; > > > > assert(netdev->type == NET_CLIENT_DRIVER_TAP); > > tap = &netdev->u.tap; > > @@ -783,7 +788,7 @@ int net_init_tap(const Netdev *netdev, const char > > *name, > > > > net_init_tap_one(tap, peer, "tap", name, NULL, > > script, downscript, > > - vhostfdname, vnet_hdr, fd, &err); > > + vhostfdname, vnet_hdr, fd, &err, NULL); > > if (err) { > > error_propagate(errp, err); > > return -1; > > @@ -835,7 +840,7 @@ int net_init_tap(const Netdev *netdev, const char *name, > > net_init_tap_one(tap, peer, "tap", name, ifname, > > script, downscript, > > tap->has_vhostfds ? vhost_fds[i] : NULL, > > - vnet_hdr, fd, &err); > > + vnet_hdr, fd, &err, NULL); > > if (err) { > > error_propagate(errp, err); > > goto free_fail; > > @@ -874,10 +879,13 @@ free_fail: > > > > net_init_tap_one(tap, peer, "bridge", name, ifname, > > script, downscript, vhostfdname, > > - vnet_hdr, fd, &err); > > + vnet_hdr, fd, &err, &error_is_set_sndbuf); > > if (err) { > > error_propagate(errp, err); > > - close(fd); > > + if (error_is_set_sndbuf || (tap->has_vhostforce && > > + tap->vhostforce)) { > > + close(fd); > > + } > > return -1; > > } > > } else { > > @@ -913,10 +921,14 @@ free_fail: > > net_init_tap_one(tap, peer, "tap", name, ifname, > > i >= 1 ? "no" : script, > > i >= 1 ? "no" : downscript, > > - vhostfdname, vnet_hdr, fd, &err); > > + vhostfdname, vnet_hdr, fd, &err, > > + &error_is_set_sndbuf); > > if (err) { > > error_propagate(errp, err); > > - close(fd); > > + if (error_is_set_sndbuf || (tap->has_vhostforce && > > + tap->vhostforce)) { > > + close(fd); > > + } > > return -1; > > } > > } > > > > > > > > PS: I think I do not express the meaning clearly... I can express it > > in Chinese in private if necessary > > > > Regards, > > Jay > > That's kind of ugly. Oops... > > I would suggest do all the correct thing in net_tap_init_one(). Yes, you're right, we could do it better, :) How about this one(I have done some tests, it works): --- --- > > Thanks diff --git a/net/tap.c b/net/tap.c index 979e622..3ed72eb 100644 --- a/net/tap.c +++ b/net/tap.c @@ -651,6 +651,9 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer, tap_set_sndbuf(s->fd, tap, &err); if (err) { error_propagate(errp, err); + if (!tap->has_fd && !tap->has_fds) { + close(fd); + } return; } @@ -687,14 +690,14 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer, vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err); if (vhostfd == -1) { error_propagate(errp, err); - return; + goto cleanup; } } else { vhostfd = open("/dev/vhost-net", O_RDWR); if (vhostfd < 0) { error_setg_errno(errp, errno, "tap: open vhost char device failed"); - return; + goto cleanup; } fcntl(vhostfd, F_SETFL, O_NONBLOCK); } @@ -704,11 +707,18 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer, if (!s->vhost_net) { error_setg(errp, "vhost-net requested but could not be initialized"); - return; + goto cleanup; } } else if (vhostfdname) { error_setg(errp, "vhostfd(s)= is not valid without vhost"); } + +cleanup: + if (!tap->has_fd && !tap->has_fds && tap->has_vhostforce && + tap->vhostforce) { + close(fd); + } + return; } static int get_fds(char *str, char *fds[], int max) @@ -877,7 +887,6 @@ free_fail: vnet_hdr, fd, &err); if (err) { error_propagate(errp, err); - close(fd); return -1; } } else { @@ -916,7 +925,6 @@ free_fail: vhostfdname, vnet_hdr, fd, &err); if (err) { error_propagate(errp, err); - close(fd); return -1; } }