[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