[dpdk-dev] [PATCH v6 01/19] vhost: fix messages results handling

Maxime Coquelin maxime.coquelin at redhat.com
Fri Oct 12 11:55:09 CEST 2018



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 <maxime.coquelin at redhat.com>
>>> ---
>>>   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);
>>>


More information about the dev mailing list