diff mbox series

[v2,5/5] vhost: message handling implemented as a callback array

Message ID 153202763503.21481.6074577075024787339.stgit@T460 (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series vhost_user.c code cleanup |

Checks

Context Check Description
ci/Intel-compilation success Compilation OK

Commit Message

Nikolay Nikolaev July 19, 2018, 7:13 p.m. UTC 
  Introduce vhost_message_handlers, which maps the message request
type to the message handler. Then replace the switch construct
with a map and call.

Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
---
 lib/librte_vhost/vhost_user.c |  143 +++++++++++++++--------------------------
 1 file changed, 54 insertions(+), 89 deletions(-)

Comments

Maxime Coquelin Sept. 10, 2018, 3:32 p.m. UTC | #1 
On 07/19/2018 09:13 PM, Nikolay Nikolaev wrote:
> Introduce vhost_message_handlers, which maps the message request
> type to the message handler. Then replace the switch construct
> with a map and call.
> 
> Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
> ---
>   lib/librte_vhost/vhost_user.c |  143 +++++++++++++++--------------------------
>   1 file changed, 54 insertions(+), 89 deletions(-)

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 6b39d1e30..1b164df27 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1466,6 +1466,34 @@ vhost_user_iotlb_msg(struct virtio_net **pdev, VhostUserMsg *msg)
>   	return VH_RESULT_OK;
>   }
>   
> +typedef int (*vhost_message_handler_t)(struct virtio_net **pdev, VhostUserMsg * msg);
> +static vhost_message_handler_t vhost_message_handlers[VHOST_USER_MAX] = {
> +	[VHOST_USER_NONE] = NULL,
> +	[VHOST_USER_GET_FEATURES] = vhost_user_get_features,
> +	[VHOST_USER_SET_FEATURES] = vhost_user_set_features,
> +	[VHOST_USER_SET_OWNER] = vhost_user_set_owner,
> +	[VHOST_USER_RESET_OWNER] = vhost_user_reset_owner,
> +	[VHOST_USER_SET_MEM_TABLE] = vhost_user_set_mem_table,
> +	[VHOST_USER_SET_LOG_BASE] = vhost_user_set_log_base,
> +	[VHOST_USER_SET_LOG_FD] = vhost_user_set_log_fd,
> +	[VHOST_USER_SET_VRING_NUM] = vhost_user_set_vring_num,
> +	[VHOST_USER_SET_VRING_ADDR] = vhost_user_set_vring_addr,
> +	[VHOST_USER_SET_VRING_BASE] = vhost_user_set_vring_base,
> +	[VHOST_USER_GET_VRING_BASE] = vhost_user_get_vring_base,
> +	[VHOST_USER_SET_VRING_KICK] = vhost_user_set_vring_kick,
> +	[VHOST_USER_SET_VRING_CALL] = vhost_user_set_vring_call,
> +	[VHOST_USER_SET_VRING_ERR] = vhost_user_set_vring_err,
> +	[VHOST_USER_GET_PROTOCOL_FEATURES] = vhost_user_get_protocol_features,
> +	[VHOST_USER_SET_PROTOCOL_FEATURES] = vhost_user_set_protocol_features,
> +	[VHOST_USER_GET_QUEUE_NUM] = vhost_user_get_queue_num,
> +	[VHOST_USER_SET_VRING_ENABLE] = vhost_user_set_vring_enable,
> +	[VHOST_USER_SEND_RARP] = vhost_user_send_rarp,
> +	[VHOST_USER_NET_SET_MTU] = vhost_user_net_set_mtu,
> +	[VHOST_USER_SET_SLAVE_REQ_FD] = vhost_user_set_req_fd,
> +	[VHOST_USER_IOTLB_MSG] = vhost_user_iotlb_msg,
> +};
> +
> +
>   /* return bytes# of read on success or negative val on failure. */
>   static int
>   read_vhost_message(int sockfd, VhostUserMsg *msg)
> @@ -1618,6 +1646,7 @@ vhost_user_msg_handler(int vid, int fd)
>   	int ret;
>   	int unlock_required = 0;
>   	uint32_t skip_master = 0;
> +	int request;
>   
>   	dev = get_device(vid);
>   	if (dev == NULL)
> @@ -1710,97 +1739,33 @@ vhost_user_msg_handler(int vid, int fd)
>   			goto skip_to_post_handle;
>   	}
>   
> -	switch (msg.request.master) {
> -	case VHOST_USER_GET_FEATURES:
> -		ret = vhost_user_get_features(&dev, &msg);
> -		send_vhost_reply(fd, &msg);
> -		break;
> -	case VHOST_USER_SET_FEATURES:
> -		ret = vhost_user_set_features(&dev, &msg);
> -		if (ret)
> -			return -1;
> -		break;
> -
> -	case VHOST_USER_GET_PROTOCOL_FEATURES:
> -		ret = vhost_user_get_protocol_features(&dev, &msg);
> -		send_vhost_reply(fd, &msg);
> -		break;
> -	case VHOST_USER_SET_PROTOCOL_FEATURES:
> -		ret = vhost_user_set_protocol_features(&dev, &msg);
> -		break;
> -
> -	case VHOST_USER_SET_OWNER:
> -		ret = vhost_user_set_owner(&dev, &msg);
> -		break;
> -	case VHOST_USER_RESET_OWNER:
> -		ret = vhost_user_reset_owner(&dev, &msg);
> -		break;
> -
> -	case VHOST_USER_SET_MEM_TABLE:
> -		ret = vhost_user_set_mem_table(&dev, &msg);
> -		break;
> -
> -	case VHOST_USER_SET_LOG_BASE:
> -		ret = vhost_user_set_log_base(&dev, &msg);
> -		send_vhost_reply(fd, &msg);
> -		break;
> -	case VHOST_USER_SET_LOG_FD:
> -		ret = vhost_user_set_log_fd(&dev, &msg);
> -		break;
> -
> -	case VHOST_USER_SET_VRING_NUM:
> -		ret = vhost_user_set_vring_num(&dev, &msg);
> -		break;
> -	case VHOST_USER_SET_VRING_ADDR:
> -		ret = vhost_user_set_vring_addr(&dev, &msg);
> -		break;
> -	case VHOST_USER_SET_VRING_BASE:
> -		ret = vhost_user_set_vring_base(&dev, &msg);
> -		break;
> -
> -	case VHOST_USER_GET_VRING_BASE:
> -		ret = vhost_user_get_vring_base(&dev, &msg);
> -		send_vhost_reply(fd, &msg);
> -		break;
> -
> -	case VHOST_USER_SET_VRING_KICK:
> -		ret = vhost_user_set_vring_kick(&dev, &msg);
> -		break;
> -	case VHOST_USER_SET_VRING_CALL:
> -		ret = vhost_user_set_vring_call(&dev, &msg);
> -		break;
> -
> -	case VHOST_USER_SET_VRING_ERR:
> -		ret = vhost_user_set_vring_err(&dev, &msg);
> -		break;
> -
> -	case VHOST_USER_GET_QUEUE_NUM:
> -		ret = vhost_user_get_queue_num(&dev, &msg);
> -		send_vhost_reply(fd, &msg);
> -		break;
> -
> -	case VHOST_USER_SET_VRING_ENABLE:
> -		ret = vhost_user_set_vring_enable(&dev, &msg);
> -		break;
> -	case VHOST_USER_SEND_RARP:
> -		ret = vhost_user_send_rarp(&dev, &msg);
> -		break;
> -
> -	case VHOST_USER_NET_SET_MTU:
> -		ret = vhost_user_net_set_mtu(&dev, &msg);
> -		break;
> -
> -	case VHOST_USER_SET_SLAVE_REQ_FD:
> -		ret = vhost_user_set_req_fd(&dev, &msg);
> -		break;
> -
> -	case VHOST_USER_IOTLB_MSG:
> -		ret = vhost_user_iotlb_msg(&dev, &msg);
> -		break;
> +	request = msg.request.master;
> +	if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) {
> +		if (!vhost_message_handlers[request])
> +			goto skip_to_post_handle;
> +		ret = vhost_message_handlers[request](&dev, &msg);
>   
> -	default:
> +		switch (ret) {
> +		case VH_RESULT_ERR:
> +			RTE_LOG(ERR, VHOST_CONFIG,
> +				"Processing %s failed.\n",
> +				vhost_message_str[request]);
> +		case VH_RESULT_OK:
> +			RTE_LOG(DEBUG, VHOST_CONFIG,
> +				"Processing %s succeeded.\n",
> +				vhost_message_str[request]);
> +			break;
> +		case VH_RESULT_REPLY:
> +			RTE_LOG(INFO, VHOST_CONFIG,
> +				"Processing %s succeeded and needs reply.\n",
> +				vhost_message_str[request]);
> +			send_vhost_reply(fd, &msg);
> +			break;
> +		}
> +	} else {
> +		RTE_LOG(ERR, VHOST_CONFIG,
> +			"Requested invalid message type %d.\n", request);
>   		ret = -1;
> -		break;
>   	}
>   
>   skip_to_post_handle:
>
Ilya Maximets Sept. 10, 2018, 4:09 p.m. UTC | #2 
Hi Maxime,
Thanks for pointing to this patch set. I missed it somehow.

This patch could not replace mine [1], because it does not improve
the message handling from the error handling point of view. But
it completely changes the code, so we need to negotiate the order
in which they will be applied or combine them somehow.

So, what's the plan? What do you think?

Few comments inline.

[1] http://patchwork.dpdk.org/patch/44168/

Best regards, Ilya Maximets.

On 10.09.2018 18:32, Maxime Coquelin wrote:
> 
> 
> On 07/19/2018 09:13 PM, Nikolay Nikolaev wrote:
>> Introduce vhost_message_handlers, which maps the message request
>> type to the message handler. Then replace the switch construct
>> with a map and call.
>>
>> Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
>> ---
>>   lib/librte_vhost/vhost_user.c |  143 +++++++++++++++--------------------------
>>   1 file changed, 54 insertions(+), 89 deletions(-)
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>> index 6b39d1e30..1b164df27 100644
>> --- a/lib/librte_vhost/vhost_user.c
>> +++ b/lib/librte_vhost/vhost_user.c
>> @@ -1466,6 +1466,34 @@ vhost_user_iotlb_msg(struct virtio_net **pdev, VhostUserMsg *msg)
>>       return VH_RESULT_OK;
>>   }
>>   +typedef int (*vhost_message_handler_t)(struct virtio_net **pdev, VhostUserMsg * msg);
>> +static vhost_message_handler_t vhost_message_handlers[VHOST_USER_MAX] = {
>> +    [VHOST_USER_NONE] = NULL,
>> +    [VHOST_USER_GET_FEATURES] = vhost_user_get_features,
>> +    [VHOST_USER_SET_FEATURES] = vhost_user_set_features,
>> +    [VHOST_USER_SET_OWNER] = vhost_user_set_owner,
>> +    [VHOST_USER_RESET_OWNER] = vhost_user_reset_owner,
>> +    [VHOST_USER_SET_MEM_TABLE] = vhost_user_set_mem_table,
>> +    [VHOST_USER_SET_LOG_BASE] = vhost_user_set_log_base,
>> +    [VHOST_USER_SET_LOG_FD] = vhost_user_set_log_fd,
>> +    [VHOST_USER_SET_VRING_NUM] = vhost_user_set_vring_num,
>> +    [VHOST_USER_SET_VRING_ADDR] = vhost_user_set_vring_addr,
>> +    [VHOST_USER_SET_VRING_BASE] = vhost_user_set_vring_base,
>> +    [VHOST_USER_GET_VRING_BASE] = vhost_user_get_vring_base,
>> +    [VHOST_USER_SET_VRING_KICK] = vhost_user_set_vring_kick,
>> +    [VHOST_USER_SET_VRING_CALL] = vhost_user_set_vring_call,
>> +    [VHOST_USER_SET_VRING_ERR] = vhost_user_set_vring_err,
>> +    [VHOST_USER_GET_PROTOCOL_FEATURES] = vhost_user_get_protocol_features,
>> +    [VHOST_USER_SET_PROTOCOL_FEATURES] = vhost_user_set_protocol_features,
>> +    [VHOST_USER_GET_QUEUE_NUM] = vhost_user_get_queue_num,
>> +    [VHOST_USER_SET_VRING_ENABLE] = vhost_user_set_vring_enable,
>> +    [VHOST_USER_SEND_RARP] = vhost_user_send_rarp,
>> +    [VHOST_USER_NET_SET_MTU] = vhost_user_net_set_mtu,
>> +    [VHOST_USER_SET_SLAVE_REQ_FD] = vhost_user_set_req_fd,
>> +    [VHOST_USER_IOTLB_MSG] = vhost_user_iotlb_msg,
>> +};
>> +
>> +
>>   /* return bytes# of read on success or negative val on failure. */
>>   static int
>>   read_vhost_message(int sockfd, VhostUserMsg *msg)
>> @@ -1618,6 +1646,7 @@ vhost_user_msg_handler(int vid, int fd)
>>       int ret;
>>       int unlock_required = 0;
>>       uint32_t skip_master = 0;
>> +    int request;
>>         dev = get_device(vid);
>>       if (dev == NULL)
>> @@ -1710,97 +1739,33 @@ vhost_user_msg_handler(int vid, int fd)
>>               goto skip_to_post_handle;
>>       }
>>   -    switch (msg.request.master) {
>> -    case VHOST_USER_GET_FEATURES:
>> -        ret = vhost_user_get_features(&dev, &msg);
>> -        send_vhost_reply(fd, &msg);
>> -        break;
>> -    case VHOST_USER_SET_FEATURES:
>> -        ret = vhost_user_set_features(&dev, &msg);
>> -        if (ret)
>> -            return -1;

You can not just remove this. Disconnection was triggered here
because the error is unrecoverable.
See 59fe5e17d930 ("vhost: propagate set features handling error")
for details.

>> -        break;
>> -
>> -    case VHOST_USER_GET_PROTOCOL_FEATURES:
>> -        ret = vhost_user_get_protocol_features(&dev, &msg);
>> -        send_vhost_reply(fd, &msg);
>> -        break;
>> -    case VHOST_USER_SET_PROTOCOL_FEATURES:
>> -        ret = vhost_user_set_protocol_features(&dev, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_SET_OWNER:
>> -        ret = vhost_user_set_owner(&dev, &msg);
>> -        break;
>> -    case VHOST_USER_RESET_OWNER:
>> -        ret = vhost_user_reset_owner(&dev, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_SET_MEM_TABLE:
>> -        ret = vhost_user_set_mem_table(&dev, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_SET_LOG_BASE:
>> -        ret = vhost_user_set_log_base(&dev, &msg);
>> -        send_vhost_reply(fd, &msg);
>> -        break;
>> -    case VHOST_USER_SET_LOG_FD:
>> -        ret = vhost_user_set_log_fd(&dev, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_SET_VRING_NUM:
>> -        ret = vhost_user_set_vring_num(&dev, &msg);
>> -        break;
>> -    case VHOST_USER_SET_VRING_ADDR:
>> -        ret = vhost_user_set_vring_addr(&dev, &msg);
>> -        break;
>> -    case VHOST_USER_SET_VRING_BASE:
>> -        ret = vhost_user_set_vring_base(&dev, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_GET_VRING_BASE:
>> -        ret = vhost_user_get_vring_base(&dev, &msg);
>> -        send_vhost_reply(fd, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_SET_VRING_KICK:
>> -        ret = vhost_user_set_vring_kick(&dev, &msg);
>> -        break;
>> -    case VHOST_USER_SET_VRING_CALL:
>> -        ret = vhost_user_set_vring_call(&dev, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_SET_VRING_ERR:
>> -        ret = vhost_user_set_vring_err(&dev, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_GET_QUEUE_NUM:
>> -        ret = vhost_user_get_queue_num(&dev, &msg);
>> -        send_vhost_reply(fd, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_SET_VRING_ENABLE:
>> -        ret = vhost_user_set_vring_enable(&dev, &msg);
>> -        break;
>> -    case VHOST_USER_SEND_RARP:
>> -        ret = vhost_user_send_rarp(&dev, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_NET_SET_MTU:
>> -        ret = vhost_user_net_set_mtu(&dev, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_SET_SLAVE_REQ_FD:
>> -        ret = vhost_user_set_req_fd(&dev, &msg);
>> -        break;
>> -
>> -    case VHOST_USER_IOTLB_MSG:
>> -        ret = vhost_user_iotlb_msg(&dev, &msg);
>> -        break;
>> +    request = msg.request.master;
>> +    if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) {
>> +        if (!vhost_message_handlers[request])
>> +            goto skip_to_post_handle;
>> +        ret = vhost_message_handlers[request](&dev, &msg);
>>   -    default:
>> +        switch (ret) {
>> +        case VH_RESULT_ERR:
>> +            RTE_LOG(ERR, VHOST_CONFIG,
>> +                "Processing %s failed.\n",
>> +                vhost_message_str[request]);

I guess 'break' is missing here. Isn't it?

>> +        case VH_RESULT_OK:
>> +            RTE_LOG(DEBUG, VHOST_CONFIG,
>> +                "Processing %s succeeded.\n",
>> +                vhost_message_str[request]);
>> +            break;
>> +        case VH_RESULT_REPLY:
>> +            RTE_LOG(INFO, VHOST_CONFIG,
>> +                "Processing %s succeeded and needs reply.\n",
>> +                vhost_message_str[request]);
>> +            send_vhost_reply(fd, &msg);
>> +            break;
>> +        }
>> +    } else {
>> +        RTE_LOG(ERR, VHOST_CONFIG,
>> +            "Requested invalid message type %d.\n", request);
>>           ret = -1;
>> -        break;
>>       }
>>     skip_to_post_handle:
>>
> 
>
Maxime Coquelin Sept. 10, 2018, 4:43 p.m. UTC | #3 
Hi Ilya,

On 09/10/2018 06:09 PM, Ilya Maximets wrote:
> Hi Maxime,
> Thanks for pointing to this patch set. I missed it somehow.
> 
> This patch could not replace mine [1], because it does not improve
> the message handling from the error handling point of view. But
> it completely changes the code, so we need to negotiate the order
> in which they will be applied or combine them somehow.

By complementary I meant both of your patches add similar things, like
vhost_user_set_vring_kick() becoming an int (even if Nikolay's version
doesn't return an error for now).

> 
> So, what's the plan? What do you think?

On one hand Nikolay's series was posted first, and it might be easier to
just apply your patch on top. On the other hand, your patch fixes an
issue, so it will be easier to backport it if it is applied first.

I guess we should go for your patch first to ease backporting.

I'd like Tiwei's feedback before applying anything, as he reviewed the
first versions of both yours and Nikolay's patches.

> Few comments inline.
> 
> [1] http://patchwork.dpdk.org/patch/44168/
> 
> Best regards, Ilya Maximets.
> 
> On 10.09.2018 18:32, Maxime Coquelin wrote:
>>
>>
>> On 07/19/2018 09:13 PM, Nikolay Nikolaev wrote:
>>> Introduce vhost_message_handlers, which maps the message request
>>> type to the message handler. Then replace the switch construct
>>> with a map and call.
>>>
>>> Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
>>> ---
>>>    lib/librte_vhost/vhost_user.c |  143 +++++++++++++++--------------------------
>>>    1 file changed, 54 insertions(+), 89 deletions(-)
>>
>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>
>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>>> index 6b39d1e30..1b164df27 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -1466,6 +1466,34 @@ vhost_user_iotlb_msg(struct virtio_net **pdev, VhostUserMsg *msg)
>>>        return VH_RESULT_OK;
>>>    }
>>>    +typedef int (*vhost_message_handler_t)(struct virtio_net **pdev, VhostUserMsg * msg);
>>> +static vhost_message_handler_t vhost_message_handlers[VHOST_USER_MAX] = {
>>> +    [VHOST_USER_NONE] = NULL,
>>> +    [VHOST_USER_GET_FEATURES] = vhost_user_get_features,
>>> +    [VHOST_USER_SET_FEATURES] = vhost_user_set_features,
>>> +    [VHOST_USER_SET_OWNER] = vhost_user_set_owner,
>>> +    [VHOST_USER_RESET_OWNER] = vhost_user_reset_owner,
>>> +    [VHOST_USER_SET_MEM_TABLE] = vhost_user_set_mem_table,
>>> +    [VHOST_USER_SET_LOG_BASE] = vhost_user_set_log_base,
>>> +    [VHOST_USER_SET_LOG_FD] = vhost_user_set_log_fd,
>>> +    [VHOST_USER_SET_VRING_NUM] = vhost_user_set_vring_num,
>>> +    [VHOST_USER_SET_VRING_ADDR] = vhost_user_set_vring_addr,
>>> +    [VHOST_USER_SET_VRING_BASE] = vhost_user_set_vring_base,
>>> +    [VHOST_USER_GET_VRING_BASE] = vhost_user_get_vring_base,
>>> +    [VHOST_USER_SET_VRING_KICK] = vhost_user_set_vring_kick,
>>> +    [VHOST_USER_SET_VRING_CALL] = vhost_user_set_vring_call,
>>> +    [VHOST_USER_SET_VRING_ERR] = vhost_user_set_vring_err,
>>> +    [VHOST_USER_GET_PROTOCOL_FEATURES] = vhost_user_get_protocol_features,
>>> +    [VHOST_USER_SET_PROTOCOL_FEATURES] = vhost_user_set_protocol_features,
>>> +    [VHOST_USER_GET_QUEUE_NUM] = vhost_user_get_queue_num,
>>> +    [VHOST_USER_SET_VRING_ENABLE] = vhost_user_set_vring_enable,
>>> +    [VHOST_USER_SEND_RARP] = vhost_user_send_rarp,
>>> +    [VHOST_USER_NET_SET_MTU] = vhost_user_net_set_mtu,
>>> +    [VHOST_USER_SET_SLAVE_REQ_FD] = vhost_user_set_req_fd,
>>> +    [VHOST_USER_IOTLB_MSG] = vhost_user_iotlb_msg,
>>> +};
>>> +
>>> +
>>>    /* return bytes# of read on success or negative val on failure. */
>>>    static int
>>>    read_vhost_message(int sockfd, VhostUserMsg *msg)
>>> @@ -1618,6 +1646,7 @@ vhost_user_msg_handler(int vid, int fd)
>>>        int ret;
>>>        int unlock_required = 0;
>>>        uint32_t skip_master = 0;
>>> +    int request;
>>>          dev = get_device(vid);
>>>        if (dev == NULL)
>>> @@ -1710,97 +1739,33 @@ vhost_user_msg_handler(int vid, int fd)
>>>                goto skip_to_post_handle;
>>>        }
>>>    -    switch (msg.request.master) {
>>> -    case VHOST_USER_GET_FEATURES:
>>> -        ret = vhost_user_get_features(&dev, &msg);
>>> -        send_vhost_reply(fd, &msg);
>>> -        break;
>>> -    case VHOST_USER_SET_FEATURES:
>>> -        ret = vhost_user_set_features(&dev, &msg);
>>> -        if (ret)
>>> -            return -1;
> 
> You can not just remove this. Disconnection was triggered here
> because the error is unrecoverable.
> See 59fe5e17d930 ("vhost: propagate set features handling error")
> for details.

Right, maybe adding another enum for unrecoverable errors would do the
trick.

>>> -        break;
>>> -
>>> -    case VHOST_USER_GET_PROTOCOL_FEATURES:
>>> -        ret = vhost_user_get_protocol_features(&dev, &msg);
>>> -        send_vhost_reply(fd, &msg);
>>> -        break;
>>> -    case VHOST_USER_SET_PROTOCOL_FEATURES:
>>> -        ret = vhost_user_set_protocol_features(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_OWNER:
>>> -        ret = vhost_user_set_owner(&dev, &msg);
>>> -        break;
>>> -    case VHOST_USER_RESET_OWNER:
>>> -        ret = vhost_user_reset_owner(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_MEM_TABLE:
>>> -        ret = vhost_user_set_mem_table(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_LOG_BASE:
>>> -        ret = vhost_user_set_log_base(&dev, &msg);
>>> -        send_vhost_reply(fd, &msg);
>>> -        break;
>>> -    case VHOST_USER_SET_LOG_FD:
>>> -        ret = vhost_user_set_log_fd(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_VRING_NUM:
>>> -        ret = vhost_user_set_vring_num(&dev, &msg);
>>> -        break;
>>> -    case VHOST_USER_SET_VRING_ADDR:
>>> -        ret = vhost_user_set_vring_addr(&dev, &msg);
>>> -        break;
>>> -    case VHOST_USER_SET_VRING_BASE:
>>> -        ret = vhost_user_set_vring_base(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_GET_VRING_BASE:
>>> -        ret = vhost_user_get_vring_base(&dev, &msg);
>>> -        send_vhost_reply(fd, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_VRING_KICK:
>>> -        ret = vhost_user_set_vring_kick(&dev, &msg);
>>> -        break;
>>> -    case VHOST_USER_SET_VRING_CALL:
>>> -        ret = vhost_user_set_vring_call(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_VRING_ERR:
>>> -        ret = vhost_user_set_vring_err(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_GET_QUEUE_NUM:
>>> -        ret = vhost_user_get_queue_num(&dev, &msg);
>>> -        send_vhost_reply(fd, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_VRING_ENABLE:
>>> -        ret = vhost_user_set_vring_enable(&dev, &msg);
>>> -        break;
>>> -    case VHOST_USER_SEND_RARP:
>>> -        ret = vhost_user_send_rarp(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_NET_SET_MTU:
>>> -        ret = vhost_user_net_set_mtu(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_SLAVE_REQ_FD:
>>> -        ret = vhost_user_set_req_fd(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_IOTLB_MSG:
>>> -        ret = vhost_user_iotlb_msg(&dev, &msg);
>>> -        break;
>>> +    request = msg.request.master;
>>> +    if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) {
>>> +        if (!vhost_message_handlers[request])
>>> +            goto skip_to_post_handle;
>>> +        ret = vhost_message_handlers[request](&dev, &msg);
>>>    -    default:
>>> +        switch (ret) {
>>> +        case VH_RESULT_ERR:
>>> +            RTE_LOG(ERR, VHOST_CONFIG,
>>> +                "Processing %s failed.\n",
>>> +                vhost_message_str[request]);
> 
> I guess 'break' is missing here. Isn't it?

Good catch.

> 
>>> +        case VH_RESULT_OK:
>>> +            RTE_LOG(DEBUG, VHOST_CONFIG,
>>> +                "Processing %s succeeded.\n",
>>> +                vhost_message_str[request]);
>>> +            break;
>>> +        case VH_RESULT_REPLY:
>>> +            RTE_LOG(INFO, VHOST_CONFIG,
>>> +                "Processing %s succeeded and needs reply.\n",
>>> +                vhost_message_str[request]);
>>> +            send_vhost_reply(fd, &msg);
>>> +            break;
>>> +        }
>>> +    } else {
>>> +        RTE_LOG(ERR, VHOST_CONFIG,
>>> +            "Requested invalid message type %d.\n", request);
>>>            ret = -1;
>>> -        break;
>>>        }
>>>      skip_to_post_handle:
>>>
>>
>>
Tiwei Bie Sept. 11, 2018, 1:51 a.m. UTC | #4 
On Mon, Sep 10, 2018 at 06:43:17PM +0200, Maxime Coquelin wrote:
> Hi Ilya,
> 
> On 09/10/2018 06:09 PM, Ilya Maximets wrote:
> > Hi Maxime,
> > Thanks for pointing to this patch set. I missed it somehow.
> > 
> > This patch could not replace mine [1], because it does not improve
> > the message handling from the error handling point of view. But
> > it completely changes the code, so we need to negotiate the order
> > in which they will be applied or combine them somehow.
> 
> By complementary I meant both of your patches add similar things, like
> vhost_user_set_vring_kick() becoming an int (even if Nikolay's version
> doesn't return an error for now).
> 
> > 
> > So, what's the plan? What do you think?
> 
> On one hand Nikolay's series was posted first, and it might be easier to
> just apply your patch on top. On the other hand, your patch fixes an
> issue, so it will be easier to backport it if it is applied first.
> 
> I guess we should go for your patch first to ease backporting.
> 
> I'd like Tiwei's feedback before applying anything, as he reviewed the
> first versions of both yours and Nikolay's patches.

I also incline to merge the fix patch first, as it needs
to be backported.

> 
> > Few comments inline.
> > 
> > [1] http://patchwork.dpdk.org/patch/44168/
[...]
diff mbox series

Patch

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 6b39d1e30..1b164df27 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1466,6 +1466,34 @@  vhost_user_iotlb_msg(struct virtio_net **pdev, VhostUserMsg *msg)
 	return VH_RESULT_OK;
 }
 
+typedef int (*vhost_message_handler_t)(struct virtio_net **pdev, VhostUserMsg * msg);
+static vhost_message_handler_t vhost_message_handlers[VHOST_USER_MAX] = {
+	[VHOST_USER_NONE] = NULL,
+	[VHOST_USER_GET_FEATURES] = vhost_user_get_features,
+	[VHOST_USER_SET_FEATURES] = vhost_user_set_features,
+	[VHOST_USER_SET_OWNER] = vhost_user_set_owner,
+	[VHOST_USER_RESET_OWNER] = vhost_user_reset_owner,
+	[VHOST_USER_SET_MEM_TABLE] = vhost_user_set_mem_table,
+	[VHOST_USER_SET_LOG_BASE] = vhost_user_set_log_base,
+	[VHOST_USER_SET_LOG_FD] = vhost_user_set_log_fd,
+	[VHOST_USER_SET_VRING_NUM] = vhost_user_set_vring_num,
+	[VHOST_USER_SET_VRING_ADDR] = vhost_user_set_vring_addr,
+	[VHOST_USER_SET_VRING_BASE] = vhost_user_set_vring_base,
+	[VHOST_USER_GET_VRING_BASE] = vhost_user_get_vring_base,
+	[VHOST_USER_SET_VRING_KICK] = vhost_user_set_vring_kick,
+	[VHOST_USER_SET_VRING_CALL] = vhost_user_set_vring_call,
+	[VHOST_USER_SET_VRING_ERR] = vhost_user_set_vring_err,
+	[VHOST_USER_GET_PROTOCOL_FEATURES] = vhost_user_get_protocol_features,
+	[VHOST_USER_SET_PROTOCOL_FEATURES] = vhost_user_set_protocol_features,
+	[VHOST_USER_GET_QUEUE_NUM] = vhost_user_get_queue_num,
+	[VHOST_USER_SET_VRING_ENABLE] = vhost_user_set_vring_enable,
+	[VHOST_USER_SEND_RARP] = vhost_user_send_rarp,
+	[VHOST_USER_NET_SET_MTU] = vhost_user_net_set_mtu,
+	[VHOST_USER_SET_SLAVE_REQ_FD] = vhost_user_set_req_fd,
+	[VHOST_USER_IOTLB_MSG] = vhost_user_iotlb_msg,
+};
+
+
 /* return bytes# of read on success or negative val on failure. */
 static int
 read_vhost_message(int sockfd, VhostUserMsg *msg)
@@ -1618,6 +1646,7 @@  vhost_user_msg_handler(int vid, int fd)
 	int ret;
 	int unlock_required = 0;
 	uint32_t skip_master = 0;
+	int request;
 
 	dev = get_device(vid);
 	if (dev == NULL)
@@ -1710,97 +1739,33 @@  vhost_user_msg_handler(int vid, int fd)
 			goto skip_to_post_handle;
 	}
 
-	switch (msg.request.master) {
-	case VHOST_USER_GET_FEATURES:
-		ret = vhost_user_get_features(&dev, &msg);
-		send_vhost_reply(fd, &msg);
-		break;
-	case VHOST_USER_SET_FEATURES:
-		ret = vhost_user_set_features(&dev, &msg);
-		if (ret)
-			return -1;
-		break;
-
-	case VHOST_USER_GET_PROTOCOL_FEATURES:
-		ret = vhost_user_get_protocol_features(&dev, &msg);
-		send_vhost_reply(fd, &msg);
-		break;
-	case VHOST_USER_SET_PROTOCOL_FEATURES:
-		ret = vhost_user_set_protocol_features(&dev, &msg);
-		break;
-
-	case VHOST_USER_SET_OWNER:
-		ret = vhost_user_set_owner(&dev, &msg);
-		break;
-	case VHOST_USER_RESET_OWNER:
-		ret = vhost_user_reset_owner(&dev, &msg);
-		break;
-
-	case VHOST_USER_SET_MEM_TABLE:
-		ret = vhost_user_set_mem_table(&dev, &msg);
-		break;
-
-	case VHOST_USER_SET_LOG_BASE:
-		ret = vhost_user_set_log_base(&dev, &msg);
-		send_vhost_reply(fd, &msg);
-		break;
-	case VHOST_USER_SET_LOG_FD:
-		ret = vhost_user_set_log_fd(&dev, &msg);
-		break;
-
-	case VHOST_USER_SET_VRING_NUM:
-		ret = vhost_user_set_vring_num(&dev, &msg);
-		break;
-	case VHOST_USER_SET_VRING_ADDR:
-		ret = vhost_user_set_vring_addr(&dev, &msg);
-		break;
-	case VHOST_USER_SET_VRING_BASE:
-		ret = vhost_user_set_vring_base(&dev, &msg);
-		break;
-
-	case VHOST_USER_GET_VRING_BASE:
-		ret = vhost_user_get_vring_base(&dev, &msg);
-		send_vhost_reply(fd, &msg);
-		break;
-
-	case VHOST_USER_SET_VRING_KICK:
-		ret = vhost_user_set_vring_kick(&dev, &msg);
-		break;
-	case VHOST_USER_SET_VRING_CALL:
-		ret = vhost_user_set_vring_call(&dev, &msg);
-		break;
-
-	case VHOST_USER_SET_VRING_ERR:
-		ret = vhost_user_set_vring_err(&dev, &msg);
-		break;
-
-	case VHOST_USER_GET_QUEUE_NUM:
-		ret = vhost_user_get_queue_num(&dev, &msg);
-		send_vhost_reply(fd, &msg);
-		break;
-
-	case VHOST_USER_SET_VRING_ENABLE:
-		ret = vhost_user_set_vring_enable(&dev, &msg);
-		break;
-	case VHOST_USER_SEND_RARP:
-		ret = vhost_user_send_rarp(&dev, &msg);
-		break;
-
-	case VHOST_USER_NET_SET_MTU:
-		ret = vhost_user_net_set_mtu(&dev, &msg);
-		break;
-
-	case VHOST_USER_SET_SLAVE_REQ_FD:
-		ret = vhost_user_set_req_fd(&dev, &msg);
-		break;
-
-	case VHOST_USER_IOTLB_MSG:
-		ret = vhost_user_iotlb_msg(&dev, &msg);
-		break;
+	request = msg.request.master;
+	if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) {
+		if (!vhost_message_handlers[request])
+			goto skip_to_post_handle;
+		ret = vhost_message_handlers[request](&dev, &msg);
 
-	default:
+		switch (ret) {
+		case VH_RESULT_ERR:
+			RTE_LOG(ERR, VHOST_CONFIG,
+				"Processing %s failed.\n",
+				vhost_message_str[request]);
+		case VH_RESULT_OK:
+			RTE_LOG(DEBUG, VHOST_CONFIG,
+				"Processing %s succeeded.\n",
+				vhost_message_str[request]);
+			break;
+		case VH_RESULT_REPLY:
+			RTE_LOG(INFO, VHOST_CONFIG,
+				"Processing %s succeeded and needs reply.\n",
+				vhost_message_str[request]);
+			send_vhost_reply(fd, &msg);
+			break;
+		}
+	} else {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"Requested invalid message type %d.\n", request);
 		ret = -1;
-		break;
 	}
 
 skip_to_post_handle: