[v4,01/19] vhost: fix messages error checks
Checks
Commit Message
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
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
>
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
>>
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
> > >
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
>>>>
@@ -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;