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 9BB771B3A1; Fri, 12 Oct 2018 11:55:18 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EBE5C80F7D; Fri, 12 Oct 2018 09:55:17 +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 240775C1B2; Fri, 12 Oct 2018 09:55:11 +0000 (UTC) From: Maxime Coquelin 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> <888fb7e9-8293-8a38-cba3-450a54295e40@redhat.com> Message-ID: <98b6b4da-2224-cb99-1173-109c6ac91072@redhat.com> Date: Fri, 12 Oct 2018 11:55:09 +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: <888fb7e9-8293-8a38-cba3-450a54295e40@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Fri, 12 Oct 2018 09:55:18 +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:55:19 -0000 On 10/12/2018 11:03 AM, Maxime Coquelin wrote: > > > 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 So, makes sense, but does not build... as vhost_user_extern_ops struct is used in struct virtio_net. I will move the enum definition in vhost.h for the time being, but we might want to do some rework in the future. >>>   /** >>>    * 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. Actually, looking again at the details, we should not return an error here. Indeed, when the external backend register a post handling callback, it gets called for every message, even the ones that don't have to be handled by the external backend. So I propose to just return VH_RESULT_OK here in case the message is not handled by the handler. > >>> +    } 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); >>>