Re: [dpdk-dev] [PATCH v4 01/19] vhost: fix messages error checks

[Tiwei Bie <tiwei.bie@intel.com>]

On Tue, Oct 09, 2018 at 02:20:47PM +0200, Maxime Coquelin wrote:
> On 10/09/2018 10:03 AM, Tiwei Bie wrote:
> > On Mon, Oct 08, 2018 at 05:25:39PM +0200, Maxime Coquelin wrote:
> > > Return of message handling has now changed to an enum that can
> > > take non-negative value that is not zero in case a reply is
> > > needed. But the code checking the variable afterwards has not
> > > been updated, leading to success messages handling being
> > > treated as errors.
> > > 
> > > Fixes: 2f270595c05d ("vhost: rework message handling as a callback array")
> > 
> > Fixline should be:
> > Fixes: 0bff510b5ea6 ("vhost: unify message handling function signature")
> > 
> > > 
> > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > > Acked-by: Ilya Maximets <i.maximets@samsung.com>
> > > ---
> > >   lib/librte_vhost/vhost_user.c | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> > > index 7ef3fb4a4..060b41893 100644
> > > --- a/lib/librte_vhost/vhost_user.c
> > > +++ b/lib/librte_vhost/vhost_user.c
> > > @@ -1783,7 +1783,7 @@ vhost_user_msg_handler(int vid, int fd)
> > >   	}
> > >   skip_to_post_handle:
> > > -	if (!ret && dev->extern_ops.post_msg_handle) {
> > > +	if (ret != VH_RESULT_ERR && dev->extern_ops.post_msg_handle) {
> > >   		uint32_t need_reply;
> > >   		ret = (*dev->extern_ops.post_msg_handle)(
> > > @@ -1800,10 +1800,10 @@ vhost_user_msg_handler(int vid, int fd)
> > >   		vhost_user_unlock_all_queue_pairs(dev);
> > >   	if (msg.flags & VHOST_USER_NEED_REPLY) {
> > > -		msg.payload.u64 = !!ret;
> > > +		msg.payload.u64 = ret == VH_RESULT_ERR;
> > >   		msg.size = sizeof(msg.payload.u64);
> > >   		send_vhost_reply(fd, &msg);
> > > -	} else if (ret) {
> > > +	} else if (ret == VH_RESULT_ERR) {
> > >   		RTE_LOG(ERR, VHOST_CONFIG,
> > >   			"vhost message handling failed.\n");
> > >   		return -1;
> > 
> > We also need to take care of the return value of .pre_msg_handle()
> > and .post_msg_handle(). Otherwise, errors could be ignored.
> 
> Right. What I plan to do is setting ret to VH_RESULT_ERR if
> pre/post_msg_handler() return is negative.
> 
> Does that sound reasonable to you?
> 
> I see that vhost-crypto does not respect the callbacks documentation wrt
> to return value, and we might want to change the callbacks documentation
> so that it returns an enum vh_result.

I think it would be better to change the callbacks documentation
to ask them to return enum vh_result as well.

> 
> > > -- 
> > > 2.17.1
> > >