Re: [dpdk-dev] [PATCH v2 2/2] vhost: support requests only handled by external backend

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

Hi Ilya,

On 3/13/19 5:09 PM, Ilya Maximets wrote:
> On 13.03.2019 18:55, Maxime Coquelin wrote:
>> External backends may have specific requests to handle, and so
>> we don't want the vhost-user lib to handle these requests as
>> errors.
>>
>> This patch also changes the experimental API by introducing
>> RTE_VHOST_MSG_RESULT_NOT_HANDLED so that vhost-user lib
>> can report an error if a message is handled neither by
>> the vhost-user library nor by the external backend.
>>
>> The logic changes a bit so that if the callback returns
>> with ERR, OK or REPLY, it is considered the message
>> is handled by the external backend so it won't be
>> handled by the vhost-user library.
>> It is still possible for an external backend to listen
>> to requests that have to be handled by the vhost-user
>> library like SET_MEM_TABLE, but the callback have to
>> return NOT_HANDLED in that case.
>>
>> Vhost-crypto backend is ialso adapted to this API change.
>>
>> Suggested-by: Ilya Maximets<i.maximets@samsung.com>
>> Signed-off-by: Maxime Coquelin<maxime.coquelin@redhat.com>
>> Tested-by: Darek Stojaczyk<dariusz.stojaczyk@intel.com>
>> ---
>>   lib/librte_vhost/rte_vhost.h    | 33 ++++---------
>>   lib/librte_vhost/vhost_crypto.c | 10 +++-
>>   lib/librte_vhost/vhost_user.c   | 82 +++++++++++++++++++++------------
>>   3 files changed, 69 insertions(+), 56 deletions(-)
>>
>> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
>> index c9c392975..8ab4ff299 100644
>> --- a/lib/librte_vhost/rte_vhost.h
>> +++ b/lib/librte_vhost/rte_vhost.h
>> @@ -121,6 +121,8 @@ enum rte_vhost_msg_result {
>>   	RTE_VHOST_MSG_RESULT_OK =  0,
>>   	/* Message handling successful and reply prepared */
>>   	RTE_VHOST_MSG_RESULT_REPLY =  1,
>> +	/* Message not handled */
>> +	RTE_VHOST_MSG_RESULT_NOT_HANDLED,
>>   };
>>   
>>   /**
>> @@ -131,37 +133,20 @@ enum rte_vhost_msg_result {
> /**
>   * Function prototype for the vhost backend to handler specific vhost user
>   * messages prior to the master message handling
> 
> Now you have definition of pre handler for a common handler type.
> 
> It should be updated. Probably just cropped a bit (and s/handler/handle/):
> 
> /**
>   * Function prototype for the vhost backend to handle specific vhost user
>   * messages.

Indeed, I missed to fix that.

>>    *  vhost device id
>>    * @param msg
>>    *  Message pointer.
>> - * @param skip_master
>> - *  If the handler requires skipping the master message handling, this variable
>> - *  shall be written 1, otherwise 0.
>>    * @return
>> - *  VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply,
>> - *  VH_RESULT_ERR on failure
>> + *  RTE_VHOST_MSG_RESULT_OK on success,
>> + *  RTE_VHOST_MSG_RESULT_REPLY on success with reply,
>> + *  RTE_VHOST_MSG_RESULT_ERR on failure,
>> + *  RTE_VHOST_MSG_RESULT_NOT_HANDLED if message was not handled.
>>    */
>> -typedef enum rte_vhost_msg_result (*rte_vhost_msg_pre_handle)(int vid,
>> -		void *msg, uint32_t *skip_master);
>> -
>> -/**
>> - * Function prototype for the vhost backend to handler specific vhost user
>> - * messages after the master message handling is done
>> - *
>> - * @param vid
>> - *  vhost device id
>> - * @param msg
>> - *  Message pointer.
>> - * @return
>> - *  VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply,
>> - *  VH_RESULT_ERR on failure
>> - */
>> -typedef enum rte_vhost_msg_result (*rte_vhost_msg_post_handle)(int vid,
>> -		void *msg);
>> +typedef enum rte_vhost_msg_result (*rte_vhost_msg_handle)(int vid, void *msg);
>>   
>>   /**
>>    * Optional vhost user message handlers.
>>    */
>>   struct rte_vhost_user_extern_ops {
>> -	rte_vhost_msg_pre_handle pre_msg_handle;
>> -	rte_vhost_msg_post_handle post_msg_handle;
> What about some comments here?
> 
>          /* Called prior to the master message handling. */
>> +	rte_vhost_msg_handle pre_msg_handle;
>          /* Called after the master message handling. */
>> +	rte_vhost_msg_handle post_msg_handle;
>>   };
>>   

That's good suggestions, I'll post a v3 with these changes.

Thanks,
Maxime