From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout1.w1.samsung.com (mailout1.w1.samsung.com [210.118.77.11]) by dpdk.org (Postfix) with ESMTP id 9530A1B4DA for ; Thu, 11 Oct 2018 18:15:43 +0200 (CEST) Received: from eucas1p1.samsung.com (unknown [182.198.249.206]) by mailout1.w1.samsung.com (KnoxPortal) with ESMTP id 20181011161542euoutp01ea4ae6d925ce4eecc776882e20a9d223~cmgp3zGz90249002490euoutp01c for ; Thu, 11 Oct 2018 16:15:42 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout1.w1.samsung.com 20181011161542euoutp01ea4ae6d925ce4eecc776882e20a9d223~cmgp3zGz90249002490euoutp01c DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1539274542; bh=OOcuLlJkeHwdHKwSC3VPwHQ3tuV/vLtruyftxXqSNeo=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=j+nTBCQnGhFzzsNd8guf22GJRHslvD47oPqKhg9bD76lNrLkYp2ZEu68Mwjji9hC/ Ufm0gi02gRjOFhxt/ApOzguxB2vrvflptd7gYzgnxWvQyi1RXr9dbkEUArHZuaTdCM 6EWiZLB6sGKBBkUyf/G1cu/yTzvREjqcRiUspW18= Received: from eusmges3new.samsung.com (unknown [203.254.199.245]) by eucas1p1.samsung.com (KnoxPortal) with ESMTP id 20181011161541eucas1p1b4d28179d92f734a708cfd00694ba656~cmgpMf8Dm0172701727eucas1p1V; Thu, 11 Oct 2018 16:15:41 +0000 (GMT) Received: from eucas1p1.samsung.com ( [182.198.249.206]) by eusmges3new.samsung.com (EUCPMTA) with SMTP id 4F.32.04806.D277FBB5; Thu, 11 Oct 2018 17:15:41 +0100 (BST) Received: from eusmtrp2.samsung.com (unknown [182.198.249.139]) by eucas1p2.samsung.com (KnoxPortal) with ESMTPA id 20181011161540eucas1p2dbf54108af22eb68b2c87003d9d2d511~cmgoaUNMO3252632526eucas1p2e; Thu, 11 Oct 2018 16:15:40 +0000 (GMT) Received: from eusmgms1.samsung.com (unknown [182.198.249.179]) by eusmtrp2.samsung.com (KnoxPortal) with ESMTP id 20181011161540eusmtrp2bbf6388cc8dea41fd08f2eb16549fda6~cmgoJFVJ72398023980eusmtrp2j; Thu, 11 Oct 2018 16:15:40 +0000 (GMT) X-AuditID: cbfec7f5-34dff700000012c6-55-5bbf772dfb70 Received: from eusmtip2.samsung.com ( [203.254.199.222]) by eusmgms1.samsung.com (EUCPMTA) with SMTP id 5F.6B.04284.C277FBB5; Thu, 11 Oct 2018 17:15:40 +0100 (BST) Received: from [106.109.129.180] (unknown [106.109.129.180]) by eusmtip2.samsung.com (KnoxPortal) with ESMTPA id 20181011161539eusmtip23b38b8bd4244183c5825ca3e3f570859~cmgncfYze1499814998eusmtip2T; Thu, 11 Oct 2018 16:15:39 +0000 (GMT) To: Maxime Coquelin , dev@dpdk.org, tiwei.bie@intel.com, zhihong.wang@intel.com, jfreimann@redhat.com, nicknickolaev@gmail.com, bruce.richardson@intel.com, alejandro.lucero@netronome.com Cc: dgilbert@redhat.com, stable@dpdk.org From: Ilya Maximets Date: Thu, 11 Oct 2018 19:18:10 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181011092432.22275-2-maxime.coquelin@redhat.com> Content-Language: en-GB Content-Transfer-Encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA02SaUhUURTHufPezHuaI89xO1pUTFqWaxjxAnHJggG/SFFEYjXmQyVHZca1 PmRqOY5p5YdGxygHc0kLyX3DaNxQYVBRc0nTknBJKzdySXPmFvntd///c849/8ulCZGOb09H RMVy8ihppFhgStZ2rOtdXRPeBXlMpVuz+qViHjtc5sMuLtXx2KzaCYodSF+nWP3rIpLtyGgm 2R7tEMluK7cotiZ5h8dW9eXwfPdJNgqK+JIGzTglKWye5UnUmVOE5HvLoECSXV2GAgVXTb1C uciIeE7u7n3DNPxbRjEVM+SVmPVokJ+MMj1UyIQG5hSUdCp5KmRKi5hSBK2jEwQ+rCBoT00l DFUiZhnB1mdahWhjx9N8fyyXIFibk+H6nwhU/WMCg2HJnIX1wiy+wbBiPiIYKOo3GgTjBBWD 08jAAsYFusvbjEwyjqBS7pAGtmauQNuk1qgLGQvoyps26iaML7Q3qEg8xxZSVl7xMR+CuoVn xq2BGaVAm1IuwM3xkLb2m8Q5z0FzSwEfsyXMdVZTmA/ATsMLHua78CltFuFBSgRq3fZfwweq 5/WUIT7BHIeKRncs+0HT5BAfv4o5DC9Y4H3MIadWTWBZCMoHIlztAJvvSwjM9jCyuEw9RmLN npSaPck0e5Jp/t9bgMgyZMvFKWRhnMIziktwU0hlirioMLeb0bJKtPuzerY7V+tRy1aIDjE0 EpsJf9m3BIn40nhFkkyHgCbEVkL33F1JGCpNus3Jo6/L4yI5hQ7tp0mxrbD4+dsgERMmjeVu cVwMJ//n8mgT+2TUnluTpDbL9q60LCi20x85H5Bre3Dr4qVopwm7iR6XyjcL5tfUD7v9Go/m f70czAY71u+MDVbE+P1wED9Zb6jrWjx2eCbgwsveO/59ifctUmWn8zRVgffmV1rDQhJdmtVn ZiAtb/yDTZDYu02bX9o4Euzc9CV6syzL07m31mZ5dUMoJhXh0pMnCLlC+gcjrx5ZVQMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrOIsWRmVeSWpSXmKPExsVy+t/xe7o65fujDbZOkLY492kZk8WNVfYW 7z5tZ7Lo3XaP3eJK+092i3NrlrJYHOvcw2JxeuE1Fot/HX/YLbY2/Gey2HxxEpMDt8evBUtZ PXbOusvusXjPSyaP6d0PmT3e77vK5tG3ZRVjAFuUnk1RfmlJqkJGfnGJrVK0oYWRnqGlhZ6R iaWeobF5rJWRqZK+nU1Kak5mWWqRvl2CXsabzmXsBddsKnr7r7I2MHYbdDFycEgImEhMm+3c xcjFISSwlFHi6vUtrF2MnEBxKYkfvy5A2cISf651sUEUvWeUuPH/OBtIQljASeLn4l5WkISI wB1GiS2zzjCCJJgFNCTWX33CCNFxklHi1NmVTCAJNgEdiVOrj4AV8QrYSaz+fpYZxGYRUJXo 6vjPAmKLCkRIrF7+ghWiRlDi5MwnYHFOAQeJozu7WCAWqEv8mXeJGcIWl2j6spIVwpaX2P52 DvMERqFZSNpnIWmZhaRlFpKWBYwsqxhFUkuLc9Nziw31ihNzi0vz0vWS83M3MQIjdduxn5t3 MF7aGHyIUYCDUYmH94fUvmgh1sSy4srcQ4wSHMxKIrz6M4BCvCmJlVWpRfnxRaU5qcWHGE2B npvILCWanA9MInkl8YamhuYWlobmxubGZhZK4rznDSqjhATSE0tSs1NTC1KLYPqYODilGhgN mBZZFa7RMs3Mm1jYenHDKd9cufyLtyu61R4Zd1etyxVyZF2Q5v1v8b6yXa+jcjsNxGyOPe73 EhFYI5K8Z+32A9crnTdUz6o+FLRC6v9JnfgfF8LC9z3PaT1933i7/17z74mPtk4NcOV2a326 QXjqq+nfzKon2AlMmzbvuKB51oVH1qUbbNKVWIozEg21mIuKEwHnhYHy6gIAAA== Message-Id: <20181011161540eucas1p2dbf54108af22eb68b2c87003d9d2d511~cmgoaUNMO3252632526eucas1p2e@eucas1p2.samsung.com> X-CMS-MailID: 20181011161540eucas1p2dbf54108af22eb68b2c87003d9d2d511 X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20181011092452epcas5p3df2d93a1b9a61dcb8dd127f48ef15346 X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20181011092452epcas5p3df2d93a1b9a61dcb8dd127f48ef15346 References: <20181011092432.22275-1-maxime.coquelin@redhat.com> <20181011092432.22275-2-maxime.coquelin@redhat.com> Subject: Re: [dpdk-dev] [PATCH v6 01/19] vhost: fix messages results handling X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Oct 2018 16:15:43 -0000 On 11.10.2018 12:24, Maxime Coquelin wrote: > Return of message handling has now changed to an enum that can > take non-negative value that is not zero in case a reply is > needed. But the code checking the variable afterwards has not > been updated, leading to success messages handling being > treated as errors. > > External post and pre callbacks return type needs also to be > changed to the new enum, so that its handling is consistent. > This is done in this patch alongside with the convertion of > its only user, vhost-crypto backend. > > Fixes: 0bff510b5ea6 ("vhost: unify message handling function signature") > > Signed-off-by: Maxime Coquelin > --- > lib/librte_vhost/vhost.h | 19 +++++++----------- > lib/librte_vhost/vhost_crypto.c | 24 +++++++++++------------ > lib/librte_vhost/vhost_user.c | 34 +++++++++------------------------ > lib/librte_vhost/vhost_user.h | 9 +++++++++ > 4 files changed, 36 insertions(+), 50 deletions(-) > > diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h > index 25ffd7614..341b0a147 100644 > --- a/lib/librte_vhost/vhost.h > +++ b/lib/librte_vhost/vhost.h > @@ -292,17 +292,15 @@ struct guest_page { > * vhost device id > * @param msg > * Message pointer. > - * @param require_reply > - * If the handler requires sending a reply, this varaible shall be written 1, > - * otherwise 0. > * @param skip_master > * If the handler requires skipping the master message handling, this variable > * shall be written 1, otherwise 0. > * @return > - * 0 on success, -1 on failure > + * VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply, > + * VH_RESULT_ERR on failure > */ > -typedef int (*vhost_msg_pre_handle)(int vid, void *msg, > - uint32_t *require_reply, uint32_t *skip_master); > +typedef enum vh_result (*vhost_msg_pre_handle)(int vid, void *msg, > + uint32_t *skip_master); > > /** > * function prototype for the vhost backend to handler specific vhost user > @@ -312,14 +310,11 @@ typedef int (*vhost_msg_pre_handle)(int vid, void *msg, > * vhost device id > * @param msg > * Message pointer. > - * @param require_reply > - * If the handler requires sending a reply, this varaible shall be written 1, > - * otherwise 0. > * @return > - * 0 on success, -1 on failure > + * VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply, > + * VH_RESULT_ERR on failure > */ > -typedef int (*vhost_msg_post_handle)(int vid, void *msg, > - uint32_t *require_reply); > +typedef enum vh_result (*vhost_msg_post_handle)(int vid, void *msg); I think that function prototypes and the enum definition should be in the same header, because headers are not including each other directly. Not sure in which one, but it seems that handlers are vhost-user specific. Are they? If so, meybe we could move the prototypes to vhost_user.h. > > /** > * pre and post vhost user message handlers > diff --git a/lib/librte_vhost/vhost_crypto.c b/lib/librte_vhost/vhost_crypto.c > index 57341ef8f..e534e1187 100644 > --- a/lib/librte_vhost/vhost_crypto.c > +++ b/lib/librte_vhost/vhost_crypto.c > @@ -425,35 +425,33 @@ vhost_crypto_close_sess(struct vhost_crypto *vcrypto, uint64_t session_id) > return 0; > } > > -static int > -vhost_crypto_msg_post_handler(int vid, void *msg, uint32_t *require_reply) > +static enum vh_result > +vhost_crypto_msg_post_handler(int vid, void *msg) > { > struct virtio_net *dev = get_device(vid); > struct vhost_crypto *vcrypto; > VhostUserMsg *vmsg = msg; > - int ret = 0; > + enum vh_result ret = VH_RESULT_ERR; > > - if (dev == NULL || require_reply == NULL) { > + if (dev == NULL) { > VC_LOG_ERR("Invalid vid %i", vid); > - return -EINVAL; > + return VH_RESULT_ERR; > } > > vcrypto = dev->extern_data; > if (vcrypto == NULL) { > VC_LOG_ERR("Cannot find required data, is it initialized?"); > - return -ENOENT; > + return VH_RESULT_ERR; > } > > - *require_reply = 0; > - > if (vmsg->request.master == VHOST_USER_CRYPTO_CREATE_SESS) { > vhost_crypto_create_sess(vcrypto, > &vmsg->payload.crypto_session); > - *require_reply = 1; > - } else if (vmsg->request.master == VHOST_USER_CRYPTO_CLOSE_SESS) > - ret = vhost_crypto_close_sess(vcrypto, vmsg->payload.u64); > - else > - ret = -EINVAL; > + ret = VH_RESULT_REPLY; Maybe we need to print error message here? Seems that we're loosing the information about error type. > + } else if (vmsg->request.master == VHOST_USER_CRYPTO_CLOSE_SESS) { > + if (!vhost_crypto_close_sess(vcrypto, vmsg->payload.u64)) > + ret = VH_RESULT_OK; > + } > > return ret; > } > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c > index 7ef3fb4a4..e8375ca34 100644 > --- a/lib/librte_vhost/vhost_user.c > +++ b/lib/librte_vhost/vhost_user.c > @@ -71,16 +71,6 @@ static const char *vhost_message_str[VHOST_USER_MAX] = { > [VHOST_USER_CRYPTO_CLOSE_SESS] = "VHOST_USER_CRYPTO_CLOSE_SESS", > }; > > -/* The possible results of a message handling function */ > -enum vh_result { > - /* Message handling failed */ > - VH_RESULT_ERR = -1, > - /* Message handling successful */ > - VH_RESULT_OK = 0, > - /* Message handling successful and reply prepared */ > - VH_RESULT_REPLY = 1, > -}; > - > static uint64_t > get_blk_size(int fd) > { > @@ -1738,14 +1728,11 @@ vhost_user_msg_handler(int vid, int fd) > } > > if (dev->extern_ops.pre_msg_handle) { > - uint32_t need_reply; > - > ret = (*dev->extern_ops.pre_msg_handle)(dev->vid, > - (void *)&msg, &need_reply, &skip_master); > - if (ret < 0) > + (void *)&msg, &skip_master); > + if (ret == VH_RESULT_ERR) > goto skip_to_reply; > - > - if (need_reply) > + else if (ret == VH_RESULT_REPLY) > send_vhost_reply(fd, &msg); > > if (skip_master) > @@ -1783,15 +1770,12 @@ vhost_user_msg_handler(int vid, int fd) > } > > skip_to_post_handle: > - if (!ret && dev->extern_ops.post_msg_handle) { > - uint32_t need_reply; > - > + if (ret != VH_RESULT_ERR && dev->extern_ops.post_msg_handle) { > ret = (*dev->extern_ops.post_msg_handle)( > - dev->vid, (void *)&msg, &need_reply); > - if (ret < 0) > + dev->vid, (void *)&msg); > + if (ret == VH_RESULT_ERR) > goto skip_to_reply; > - > - if (need_reply) > + else if (ret == VH_RESULT_REPLY) > send_vhost_reply(fd, &msg); > } > > @@ -1800,10 +1784,10 @@ vhost_user_msg_handler(int vid, int fd) > vhost_user_unlock_all_queue_pairs(dev); > > if (msg.flags & VHOST_USER_NEED_REPLY) { > - msg.payload.u64 = !!ret; > + msg.payload.u64 = ret == VH_RESULT_ERR; > msg.size = sizeof(msg.payload.u64); > send_vhost_reply(fd, &msg); > - } else if (ret) { > + } else if (ret == VH_RESULT_ERR) { > RTE_LOG(ERR, VHOST_CONFIG, > "vhost message handling failed.\n"); > return -1; > diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h > index 42166adf2..62654f736 100644 > --- a/lib/librte_vhost/vhost_user.h > +++ b/lib/librte_vhost/vhost_user.h > @@ -139,6 +139,15 @@ typedef struct VhostUserMsg { > /* The version of the protocol we support */ > #define VHOST_USER_VERSION 0x1 > > +/* The possible results of a message handling function */ > +enum vh_result { > + /* Message handling failed */ > + VH_RESULT_ERR = -1, > + /* Message handling successful */ > + VH_RESULT_OK = 0, > + /* Message handling successful and reply prepared */ > + VH_RESULT_REPLY = 1, > +}; > > /* vhost_user.c */ > int vhost_user_msg_handler(int vid, int fd); >