[dpdk-dev] [PATCH v2 01/10] lib/librte_vhost: add vhost user private info structure

[Zhang, Roy Fan]

Hi Maxime,

Thanks for the review.

> 
> I think this include isn't needed looking at the rest of the patch.

I agree. I will remove this line here.

> >   #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);
> >
> 
> I think the wording is a bit misleading, I'm fine with having a private_data
> pointer, but it should only be used by the external backend.
> 
> Maybe what you need here is a new API to be to register a callback for the
> external backend to handle specific requests.

That's exactly what I need. 
Shall I rework the code like this?

/* vhost.h */
struct virtio_net {
	....
	void *extern_data; /*<< private data for external backend */
	
}

/* vhost_user.h */
typedef int (*msg_handler)(struct virtio_net *dev, struct VhostUserMsg *msg,
		int fd);

struct vhost_user_dev_extern {
	msg_handler post_vhost_user_msg_handler;
	char data[0];
};

int 
vhost_user_register_call_back(struct virtio_net *dev, msg_handler post_msg_handler);

> 
> Also, it might be interesting for the external backend to register callbacks for
> existing requests. For example .pre_vhost_user_msg_handler
> and .post_vhost_user_msg_handler. Doing so, the external backend could
> for example catch beforehand any change that could affect resources being
> used. Tomasz, Pawel, do you think that could help for the issue you reported?
> 


I think it is definitely a good idea. However there will be a problem. As vhost_crypto does not require pre_vhost_user_msg_handler I think it may not appropriate to add pre_vhost_user_msg_handler in this patchset. 

Thanks a million Maxime.

> Thanks,
> Maxime