[v6,01/19] vhost: fix messages results handling
Checks
Commit Message
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@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(-)
Comments
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@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.
>
> /**
> * 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);
>
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@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
>>
>> /**
>> * 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);
>>
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@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);
>>>
@@ -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);
/**
* pre and post vhost user message handlers
@@ -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;
+ } 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;
}
@@ -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;
@@ -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);