Re: [PATCH 02/12] lib/librte_vhost: add vhost_user private info structure

[Yuanhan Liu <yliu@fridaylinux.org>]

On Mon, Nov 27, 2017 at 08:01:05PM +0000, Fan Zhang wrote:
> This patch adds a vhost_user_dev_priv structure and a vhost_user
> message handler function prototype to vhost_user. This allows
> different types of devices to add private information and their
> device-specific vhost-user message function handlers to
> virtio_net structure. The change to vhost_user_msg_handler is
> also added to call the device-specific message handler.
> 
> Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
> ---
>  lib/librte_vhost/vhost_user.c | 13 ++++++++++++-
>  lib/librte_vhost/vhost_user.h |  9 ++++++++-
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index f4c7ce4..c5ae85f 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1337,7 +1337,18 @@ vhost_user_msg_handler(int vid, int fd)
>  		break;
>  
>  	default:

Hi Fan,

Firstly, apologize for the very late review!

> -		ret = -1;
> +		if (!dev->private_data)
> +			ret = -1;
> +		else {
> +			struct vhost_user_dev_priv *priv = dev->private_data;
> +
> +			if (!priv->vhost_user_msg_handler)
> +				ret = -1;
> +			else {
> +				ret = (*priv->vhost_user_msg_handler)(dev,
> +						&msg, fd);

I think this is more complex than necessary. You could just add msg_handler
in the virtio_net struct and do:

		if (dev->msg_handler)
			ret = dev->msg_handler(dev, &msg, fd);
		else
			ret = -1;

> +			}
> +		}
>  		break;
>  
>  	}
> diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
> index 76d9fe2..990b40a 100644
> --- a/lib/librte_vhost/vhost_user.h
> +++ b/lib/librte_vhost/vhost_user.h
> @@ -81,7 +81,7 @@ typedef enum VhostUserRequest {
>  	VHOST_USER_NET_SET_MTU = 20,
>  	VHOST_USER_SET_SLAVE_REQ_FD = 21,
>  	VHOST_USER_IOTLB_MSG = 22,
> -	VHOST_USER_MAX
> +	VHOST_USER_MAX = 25

That's weird, we should list all VHOST_USER messages here.

	--yliu

>  } VhostUserRequest;
>  
>  typedef enum VhostUserSlaveRequest {
> @@ -137,6 +137,13 @@ typedef struct VhostUserMsg {
>  /* The version of the protocol we support */
>  #define VHOST_USER_VERSION    0x1
>  
> +typedef int (*msg_handler)(struct virtio_net *dev, struct VhostUserMsg *msg,
> +		int fd);
> +
> +struct vhost_user_dev_priv {
> +	msg_handler vhost_user_msg_handler;
> +	char data[0];
> +};
>  
>  /* vhost_user.c */
>  int vhost_user_msg_handler(int vid, int fd);
> -- 
> 2.9.5