[dpdk-dev] [PATCH v6 01/19] vhost: fix messages results handling
Maxime Coquelin
maxime.coquelin at redhat.com
Fri Oct 12 11:03:51 CEST 2018
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
>>
>> /**
>> * 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);
>>
More information about the dev
mailing list