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

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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);
>>