Re: [dpdk-dev] [PATCH v2] vhost: add external message handling callbacks to the public API

["Stojaczyk, Dariusz" <dariusz.stojaczyk@intel.com>]

> -----Original Message-----
> From: Bie, Tiwei
> Sent: Tuesday, January 15, 2019 12:22 PM
> To: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>
> Cc: dev@dpdk.org; Wang, Zhihong <zhihong.wang@intel.com>; Liang,
> Cunming <cunming.liang@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Harris, James R
> <james.r.harris@intel.com>; Liu, Changpeng <changpeng.liu@intel.com>
> Subject: Re: [PATCH v2] vhost: add external message handling callbacks to
> the public API
> 
> On Mon, Jan 14, 2019 at 05:28:29AM +0100, Darek Stojaczyk wrote:
> > External message callbacks are used e.g. by vhost crypto
> > to parse crypto-specific vhost-user messages.
> >
> > We are now publishing the API to register those callbacks,
> > so that other backends outside of DPDK can use them as well.
> >
> > Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> > ---
> >  lib/librte_vhost/rte_vhost.h | 66
> ++++++++++++++++++++++++++++++++++++
> >  lib/librte_vhost/vhost.c     | 13 +++++++
> >  lib/librte_vhost/vhost.h     | 54 ++---------------------------
> >  3 files changed, 81 insertions(+), 52 deletions(-)
> >
> > diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
> > index d280ac420..a11f9ca04 100644
> > --- a/lib/librte_vhost/rte_vhost.h
> > +++ b/lib/librte_vhost/rte_vhost.h
> > @@ -111,6 +111,56 @@ struct rte_vhost_vring {
> >  	uint16_t		size;
> >  };
> >
> > +/* The possible results of a message handling function */
> 
> Better to change /* to /**, so doxygen can generate doc for it.

Ack

> 
> > +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,
> > +};
> 
> Maybe better to prefix with rte_ and RTE_?
> How about rte_vhost_result and RTE_VHOST_RESULT_*?

Ack

> 
> > +
> [...]
> >
> > +/**
> > + * Register external message handling callbacks
> > + *
> > + * @param vid
> > + *  vhost device ID
> > + * @param ops
> > + *  virtio external callbacks to register
> > + * @param ctx
> > + *  additional context passed to the callbacks
> > + * @return
> > + *  0 on success, -1 on failure
> > + */
> > +int __rte_experimental
> > +rte_vhost_extern_callback_register(int vid,
> > +		struct rte_vhost_user_extern_ops const * const ops, void
> *ctx);
> 
> This symbol also needs to be added to the .map file
> for shared library.

Ack

> 
> For the rest,
> Reviewed-by: Tiwei Bie <tiwei.bie@intel.com>

Thanks,
D.