From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 2F7C51B206; Fri, 12 Oct 2018 11:04:02 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6A645300294E; Fri, 12 Oct 2018 09:04:01 +0000 (UTC) Received: from [10.36.112.48] (ovpn-112-48.ams2.redhat.com [10.36.112.48]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8AFEE60BE8; Fri, 12 Oct 2018 09:03:54 +0000 (UTC) To: Ilya Maximets , 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 References: <20181011092432.22275-1-maxime.coquelin@redhat.com> <20181011092432.22275-2-maxime.coquelin@redhat.com> <20181011161540eucas1p2dbf54108af22eb68b2c87003d9d2d511~cmgoaUNMO3252632526eucas1p2e@eucas1p2.samsung.com> From: Maxime Coquelin Message-ID: <888fb7e9-8293-8a38-cba3-450a54295e40@redhat.com> Date: Fri, 12 Oct 2018 11:03:51 +0200 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: <20181011161540eucas1p2dbf54108af22eb68b2c87003d9d2d511~cmgoaUNMO3252632526eucas1p2e@eucas1p2.samsung.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.43]); Fri, 12 Oct 2018 09:04:01 +0000 (UTC) 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: Fri, 12 Oct 2018 09:04:05 -0000 On 10/11/2018 06:18 PM, Ilya Maximets wrote: > 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. Makes sense, I will move the definitions 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. Yes, I can add a message here. >> + } 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); >>