diff mbox series

[v4,01/19] vhost: fix messages error checks

Message ID 20181008152557.14275-2-maxime.coquelin@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series vhost: add postcopy live-migration support |

Checks

Context Check Description
ci/Intel-compilation success Compilation OK

Commit Message

Maxime Coquelin Oct. 8, 2018, 3:25 p.m. UTC 
  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")

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(-)

Comments

Tiwei Bie Oct. 9, 2018, 8:03 a.m. UTC | #1 
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.

> -- 
> 2.17.1
>
Maxime Coquelin Oct. 9, 2018, 12:20 p.m. UTC | #2 
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.

>> -- 
>> 2.17.1
>>
Tiwei Bie Oct. 9, 2018, 12:54 p.m. UTC | #3 
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
> > >
Maxime Coquelin Oct. 9, 2018, 5:53 p.m. UTC | #4 
On 10/09/2018 02:54 PM, Tiwei Bie wrote:
> 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.

Yes, that's what I meant.

So I'm adding to this patch the fix to the callback return type to enum
vh_result, and convert vhost_crypto to it.

Cheers,
Maxime
>>
>>>> -- 
>>>> 2.17.1
>>>>
diff mbox series

Patch

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;