diff mbox series

eal/linux: fix return after alarm registration failure

Message ID 20190626104056.26829-1-thomas@monjalon.net (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series eal/linux: fix return after alarm registration failure |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Thomas Monjalon June 26, 2019, 10:40 a.m. UTC 
  When adding an alarm, if an error happen when registering
the common alarm callback, it is not considered as a major failure.
The alarm is then inserted in the list.
However it was returning an error code after inserting the alarm.

The error code is reset to 0 so the behaviour and the return code
are consistent.
Other return code related lines are cleaned up for easier understanding.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_eal/linux/eal/eal_alarm.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

David Marchand June 26, 2019, 11:20 a.m. UTC | #1 
On Wed, Jun 26, 2019 at 12:41 PM Thomas Monjalon <thomas@monjalon.net>
wrote:

> When adding an alarm, if an error happen when registering
> the common alarm callback, it is not considered as a major failure.
> The alarm is then inserted in the list.
> However it was returning an error code after inserting the alarm.
>
> The error code is reset to 0 so the behaviour and the return code
> are consistent.
> Other return code related lines are cleaned up for easier understanding.
>
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  lib/librte_eal/linux/eal/eal_alarm.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_eal/linux/eal/eal_alarm.c
> b/lib/librte_eal/linux/eal/eal_alarm.c
> index 840ede780..d6d70e8c3 100644
> --- a/lib/librte_eal/linux/eal/eal_alarm.c
> +++ b/lib/librte_eal/linux/eal/eal_alarm.c
> @@ -137,9 +137,13 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback
> cb_fn, void *cb_arg)
>
>         rte_spinlock_lock(&alarm_list_lk);
>         if (!handler_registered) {
> -               ret |= rte_intr_callback_register(&intr_handle,
> +               ret = rte_intr_callback_register(&intr_handle,
>                                 eal_alarm_callback, NULL);
> -               handler_registered = (ret == 0) ? 1 : 0;
> +               if (ret == 0)
> +                       handler_registered = 1;
> +               else
> +                       /* not fatal, callback can be registered later */
> +                       ret = 0;
>         }
>
>         if (LIST_EMPTY(&alarm_list))
>

Well, then it means that you don't want to touch ret at all.
How about:
if (rte_intr_callback_register(&intr_handle,
                               eal_alarm_callback, NULL) == 0)
        handler_registered = 1;

?
Thomas Monjalon June 26, 2019, 11:36 a.m. UTC | #2 
26/06/2019 13:20, David Marchand:
> On Wed, Jun 26, 2019 at 12:41 PM Thomas Monjalon <thomas@monjalon.net>
> wrote:
> 
> > When adding an alarm, if an error happen when registering
> > the common alarm callback, it is not considered as a major failure.
> > The alarm is then inserted in the list.
> > However it was returning an error code after inserting the alarm.
> >
> > The error code is reset to 0 so the behaviour and the return code
> > are consistent.
> > Other return code related lines are cleaned up for easier understanding.
> >
[...]
> > --- a/lib/librte_eal/linux/eal/eal_alarm.c
> > +++ b/lib/librte_eal/linux/eal/eal_alarm.c
> >         if (!handler_registered) {
> > -               ret |= rte_intr_callback_register(&intr_handle,
> > +               ret = rte_intr_callback_register(&intr_handle,
> >                                 eal_alarm_callback, NULL);
> > -               handler_registered = (ret == 0) ? 1 : 0;
> > +               if (ret == 0)
> > +                       handler_registered = 1;
> > +               else
> > +                       /* not fatal, callback can be registered later */
> > +                       ret = 0;
> >         }
> 
> Well, then it means that you don't want to touch ret at all.
> How about:
> if (rte_intr_callback_register(&intr_handle,
>                                eal_alarm_callback, NULL) == 0)
>         handler_registered = 1;
> 
> ?

Too much simple :)

I think we try to avoid calling a function in a "if"
per coding style.
And my proposal has the benefit of offering a comment
about the non-fatal error.

After saying these arguments, I have to say I have no strong opinion :)
I'm fine either way.
David Marchand June 26, 2019, 11:39 a.m. UTC | #3 
On Wed, Jun 26, 2019 at 1:36 PM Thomas Monjalon <thomas@monjalon.net> wrote:

> 26/06/2019 13:20, David Marchand:
> > On Wed, Jun 26, 2019 at 12:41 PM Thomas Monjalon <thomas@monjalon.net>
> > wrote:
> >
> > > When adding an alarm, if an error happen when registering
> > > the common alarm callback, it is not considered as a major failure.
> > > The alarm is then inserted in the list.
> > > However it was returning an error code after inserting the alarm.
> > >
> > > The error code is reset to 0 so the behaviour and the return code
> > > are consistent.
> > > Other return code related lines are cleaned up for easier
> understanding.
> > >
> [...]
> > > --- a/lib/librte_eal/linux/eal/eal_alarm.c
> > > +++ b/lib/librte_eal/linux/eal/eal_alarm.c
> > >         if (!handler_registered) {
> > > -               ret |= rte_intr_callback_register(&intr_handle,
> > > +               ret = rte_intr_callback_register(&intr_handle,
> > >                                 eal_alarm_callback, NULL);
> > > -               handler_registered = (ret == 0) ? 1 : 0;
> > > +               if (ret == 0)
> > > +                       handler_registered = 1;
> > > +               else
> > > +                       /* not fatal, callback can be registered later
> */
> > > +                       ret = 0;
> > >         }
> >
> > Well, then it means that you don't want to touch ret at all.
> > How about:
> > if (rte_intr_callback_register(&intr_handle,
> >                                eal_alarm_callback, NULL) == 0)
> >         handler_registered = 1;
> >
> > ?
>
> Too much simple :)
>
> I think we try to avoid calling a function in a "if"
> per coding style.
> And my proposal has the benefit of offering a comment
> about the non-fatal error.
>

/* not fatal, callback can be registered later */
if (rte_intr_callback_register(&intr_handle,
                              eal_alarm_callback, NULL) == 0)
       handler_registered = 1;



> After saying these arguments, I have to say I have no strong opinion :)
> I'm fine either way.
>

Reviewed-by: David Marchand <david.marchand@redhat.com>
I won't insist either, if you feel like taking my proposal, you can keep
the reviewed-by token.
Anatoly Burakov June 26, 2019, 11:43 a.m. UTC | #4 
On 26-Jun-19 12:39 PM, David Marchand wrote:
> On Wed, Jun 26, 2019 at 1:36 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> 
>> 26/06/2019 13:20, David Marchand:
>>> On Wed, Jun 26, 2019 at 12:41 PM Thomas Monjalon <thomas@monjalon.net>
>>> wrote:
>>>
>>>> When adding an alarm, if an error happen when registering
>>>> the common alarm callback, it is not considered as a major failure.
>>>> The alarm is then inserted in the list.
>>>> However it was returning an error code after inserting the alarm.
>>>>
>>>> The error code is reset to 0 so the behaviour and the return code
>>>> are consistent.
>>>> Other return code related lines are cleaned up for easier
>> understanding.
>>>>
>> [...]
>>>> --- a/lib/librte_eal/linux/eal/eal_alarm.c
>>>> +++ b/lib/librte_eal/linux/eal/eal_alarm.c
>>>>          if (!handler_registered) {
>>>> -               ret |= rte_intr_callback_register(&intr_handle,
>>>> +               ret = rte_intr_callback_register(&intr_handle,
>>>>                                  eal_alarm_callback, NULL);
>>>> -               handler_registered = (ret == 0) ? 1 : 0;
>>>> +               if (ret == 0)
>>>> +                       handler_registered = 1;
>>>> +               else
>>>> +                       /* not fatal, callback can be registered later
>> */
>>>> +                       ret = 0;
>>>>          }
>>>
>>> Well, then it means that you don't want to touch ret at all.
>>> How about:
>>> if (rte_intr_callback_register(&intr_handle,
>>>                                 eal_alarm_callback, NULL) == 0)
>>>          handler_registered = 1;
>>>
>>> ?
>>
>> Too much simple :)
>>
>> I think we try to avoid calling a function in a "if"
>> per coding style.
>> And my proposal has the benefit of offering a comment
>> about the non-fatal error.
>>
> 
> /* not fatal, callback can be registered later */
> if (rte_intr_callback_register(&intr_handle,
>                                eal_alarm_callback, NULL) == 0)
>         handler_registered = 1;
> 

I prefer the original. It's more explicit and conveys the intention 
better. Did i break the tie? :)
Thomas Monjalon June 26, 2019, 11:55 a.m. UTC | #5 
26/06/2019 13:43, Burakov, Anatoly:
> On 26-Jun-19 12:39 PM, David Marchand wrote:
> > On Wed, Jun 26, 2019 at 1:36 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > 
> >> 26/06/2019 13:20, David Marchand:
> >>> On Wed, Jun 26, 2019 at 12:41 PM Thomas Monjalon <thomas@monjalon.net>
> >>> wrote:
> >>>
> >>>> When adding an alarm, if an error happen when registering
> >>>> the common alarm callback, it is not considered as a major failure.
> >>>> The alarm is then inserted in the list.
> >>>> However it was returning an error code after inserting the alarm.
> >>>>
> >>>> The error code is reset to 0 so the behaviour and the return code
> >>>> are consistent.
> >>>> Other return code related lines are cleaned up for easier
> >> understanding.
> >>>>
> >> [...]
> >>>> --- a/lib/librte_eal/linux/eal/eal_alarm.c
> >>>> +++ b/lib/librte_eal/linux/eal/eal_alarm.c
> >>>>          if (!handler_registered) {
> >>>> -               ret |= rte_intr_callback_register(&intr_handle,
> >>>> +               ret = rte_intr_callback_register(&intr_handle,
> >>>>                                  eal_alarm_callback, NULL);
> >>>> -               handler_registered = (ret == 0) ? 1 : 0;
> >>>> +               if (ret == 0)
> >>>> +                       handler_registered = 1;
> >>>> +               else
> >>>> +                       /* not fatal, callback can be registered later
> >> */
> >>>> +                       ret = 0;
> >>>>          }
> >>>
> >>> Well, then it means that you don't want to touch ret at all.
> >>> How about:
> >>> if (rte_intr_callback_register(&intr_handle,
> >>>                                 eal_alarm_callback, NULL) == 0)
> >>>          handler_registered = 1;
> >>>
> >>> ?
> >>
> >> Too much simple :)
> >>
> >> I think we try to avoid calling a function in a "if"
> >> per coding style.
> >> And my proposal has the benefit of offering a comment
> >> about the non-fatal error.
> >>
> > 
> > /* not fatal, callback can be registered later */
> > if (rte_intr_callback_register(&intr_handle,
> >                                eal_alarm_callback, NULL) == 0)
> >         handler_registered = 1;
> > 
> 
> I prefer the original. It's more explicit and conveys the intention 
> better. Did i break the tie? :)

I was going to send a v2 with David's suggestion.
Now I'm confused.
Bruce Richardson June 26, 2019, 12:36 p.m. UTC | #6 
On Wed, Jun 26, 2019 at 01:55:53PM +0200, Thomas Monjalon wrote:
> 26/06/2019 13:43, Burakov, Anatoly:
> > On 26-Jun-19 12:39 PM, David Marchand wrote:
> > > On Wed, Jun 26, 2019 at 1:36 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > 
> > >> 26/06/2019 13:20, David Marchand:
> > >>> On Wed, Jun 26, 2019 at 12:41 PM Thomas Monjalon <thomas@monjalon.net>
> > >>> wrote:
> > >>>
> > >>>> When adding an alarm, if an error happen when registering
> > >>>> the common alarm callback, it is not considered as a major failure.
> > >>>> The alarm is then inserted in the list.
> > >>>> However it was returning an error code after inserting the alarm.
> > >>>>
> > >>>> The error code is reset to 0 so the behaviour and the return code
> > >>>> are consistent.
> > >>>> Other return code related lines are cleaned up for easier
> > >> understanding.
> > >>>>
> > >> [...]
> > >>>> --- a/lib/librte_eal/linux/eal/eal_alarm.c
> > >>>> +++ b/lib/librte_eal/linux/eal/eal_alarm.c
> > >>>>          if (!handler_registered) {
> > >>>> -               ret |= rte_intr_callback_register(&intr_handle,
> > >>>> +               ret = rte_intr_callback_register(&intr_handle,
> > >>>>                                  eal_alarm_callback, NULL);
> > >>>> -               handler_registered = (ret == 0) ? 1 : 0;
> > >>>> +               if (ret == 0)
> > >>>> +                       handler_registered = 1;
> > >>>> +               else
> > >>>> +                       /* not fatal, callback can be registered later
> > >> */
> > >>>> +                       ret = 0;
> > >>>>          }
> > >>>
> > >>> Well, then it means that you don't want to touch ret at all.
> > >>> How about:
> > >>> if (rte_intr_callback_register(&intr_handle,
> > >>>                                 eal_alarm_callback, NULL) == 0)
> > >>>          handler_registered = 1;
> > >>>
> > >>> ?
> > >>
> > >> Too much simple :)
> > >>
> > >> I think we try to avoid calling a function in a "if"
> > >> per coding style.
> > >> And my proposal has the benefit of offering a comment
> > >> about the non-fatal error.
> > >>
> > > 
> > > /* not fatal, callback can be registered later */
> > > if (rte_intr_callback_register(&intr_handle,
> > >                                eal_alarm_callback, NULL) == 0)
> > >         handler_registered = 1;
> > > 
> > 
> > I prefer the original. It's more explicit and conveys the intention 
> > better. Did i break the tie? :)
> 
> I was going to send a v2 with David's suggestion.
> Now I'm confused.
> 
I always tend to prefer shorter versions, so +1 for v2 (does that make it a
v3? :-) )

/Bruce
Anatoly Burakov June 26, 2019, 12:52 p.m. UTC | #7 
On 26-Jun-19 1:36 PM, Bruce Richardson wrote:
> On Wed, Jun 26, 2019 at 01:55:53PM +0200, Thomas Monjalon wrote:
>> 26/06/2019 13:43, Burakov, Anatoly:
>>> On 26-Jun-19 12:39 PM, David Marchand wrote:
>>>> On Wed, Jun 26, 2019 at 1:36 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>>>>
>>>>> 26/06/2019 13:20, David Marchand:
>>>>>> On Wed, Jun 26, 2019 at 12:41 PM Thomas Monjalon <thomas@monjalon.net>
>>>>>> wrote:
>>>>>>
>>>>>>> When adding an alarm, if an error happen when registering
>>>>>>> the common alarm callback, it is not considered as a major failure.
>>>>>>> The alarm is then inserted in the list.
>>>>>>> However it was returning an error code after inserting the alarm.
>>>>>>>
>>>>>>> The error code is reset to 0 so the behaviour and the return code
>>>>>>> are consistent.
>>>>>>> Other return code related lines are cleaned up for easier
>>>>> understanding.
>>>>>>>
>>>>> [...]
>>>>>>> --- a/lib/librte_eal/linux/eal/eal_alarm.c
>>>>>>> +++ b/lib/librte_eal/linux/eal/eal_alarm.c
>>>>>>>           if (!handler_registered) {
>>>>>>> -               ret |= rte_intr_callback_register(&intr_handle,
>>>>>>> +               ret = rte_intr_callback_register(&intr_handle,
>>>>>>>                                   eal_alarm_callback, NULL);
>>>>>>> -               handler_registered = (ret == 0) ? 1 : 0;
>>>>>>> +               if (ret == 0)
>>>>>>> +                       handler_registered = 1;
>>>>>>> +               else
>>>>>>> +                       /* not fatal, callback can be registered later
>>>>> */
>>>>>>> +                       ret = 0;
>>>>>>>           }
>>>>>>
>>>>>> Well, then it means that you don't want to touch ret at all.
>>>>>> How about:
>>>>>> if (rte_intr_callback_register(&intr_handle,
>>>>>>                                  eal_alarm_callback, NULL) == 0)
>>>>>>           handler_registered = 1;
>>>>>>
>>>>>> ?
>>>>>
>>>>> Too much simple :)
>>>>>
>>>>> I think we try to avoid calling a function in a "if"
>>>>> per coding style.
>>>>> And my proposal has the benefit of offering a comment
>>>>> about the non-fatal error.
>>>>>
>>>>
>>>> /* not fatal, callback can be registered later */
>>>> if (rte_intr_callback_register(&intr_handle,
>>>>                                 eal_alarm_callback, NULL) == 0)
>>>>          handler_registered = 1;
>>>>
>>>
>>> I prefer the original. It's more explicit and conveys the intention
>>> better. Did i break the tie? :)
>>
>> I was going to send a v2 with David's suggestion.
>> Now I'm confused.
>>
> I always tend to prefer shorter versions, so +1 for v2 (does that make it a
> v3? :-) )
> 
> /Bruce
> 

OK, but then the suggested comment needs to be fixed. It makes it seem 
like registering the handler is the "non fatal" part. Perhaps something 
like:

/* failed register is not a fatal error - callback can be registered 
later */
Thomas Monjalon June 26, 2019, 1:20 p.m. UTC | #8 
26/06/2019 14:52, Burakov, Anatoly:
> On 26-Jun-19 1:36 PM, Bruce Richardson wrote:
> > On Wed, Jun 26, 2019 at 01:55:53PM +0200, Thomas Monjalon wrote:
> >> 26/06/2019 13:43, Burakov, Anatoly:
> >>> On 26-Jun-19 12:39 PM, David Marchand wrote:
> >>>> On Wed, Jun 26, 2019 at 1:36 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >>>>
> >>>>> 26/06/2019 13:20, David Marchand:
> >>>>>> On Wed, Jun 26, 2019 at 12:41 PM Thomas Monjalon <thomas@monjalon.net>
> >>>>>> wrote:
> >>>>>>
> >>>>>>> When adding an alarm, if an error happen when registering
> >>>>>>> the common alarm callback, it is not considered as a major failure.
> >>>>>>> The alarm is then inserted in the list.
> >>>>>>> However it was returning an error code after inserting the alarm.
> >>>>>>>
> >>>>>>> The error code is reset to 0 so the behaviour and the return code
> >>>>>>> are consistent.
> >>>>>>> Other return code related lines are cleaned up for easier
> >>>>> understanding.
> >>>>>>>
> >>>>> [...]
> >>>>>>> --- a/lib/librte_eal/linux/eal/eal_alarm.c
> >>>>>>> +++ b/lib/librte_eal/linux/eal/eal_alarm.c
> >>>>>>>           if (!handler_registered) {
> >>>>>>> -               ret |= rte_intr_callback_register(&intr_handle,
> >>>>>>> +               ret = rte_intr_callback_register(&intr_handle,
> >>>>>>>                                   eal_alarm_callback, NULL);
> >>>>>>> -               handler_registered = (ret == 0) ? 1 : 0;
> >>>>>>> +               if (ret == 0)
> >>>>>>> +                       handler_registered = 1;
> >>>>>>> +               else
> >>>>>>> +                       /* not fatal, callback can be registered later
> >>>>> */
> >>>>>>> +                       ret = 0;
> >>>>>>>           }
> >>>>>>
> >>>>>> Well, then it means that you don't want to touch ret at all.
> >>>>>> How about:
> >>>>>> if (rte_intr_callback_register(&intr_handle,
> >>>>>>                                  eal_alarm_callback, NULL) == 0)
> >>>>>>           handler_registered = 1;
> >>>>>>
> >>>>>> ?
> >>>>>
> >>>>> Too much simple :)
> >>>>>
> >>>>> I think we try to avoid calling a function in a "if"
> >>>>> per coding style.
> >>>>> And my proposal has the benefit of offering a comment
> >>>>> about the non-fatal error.
> >>>>>
> >>>>
> >>>> /* not fatal, callback can be registered later */
> >>>> if (rte_intr_callback_register(&intr_handle,
> >>>>                                 eal_alarm_callback, NULL) == 0)
> >>>>          handler_registered = 1;
> >>>>
> >>>
> >>> I prefer the original. It's more explicit and conveys the intention
> >>> better. Did i break the tie? :)
> >>
> >> I was going to send a v2 with David's suggestion.
> >> Now I'm confused.
> >>
> > I always tend to prefer shorter versions, so +1 for v2 (does that make it a
> > v3? :-) )
> > 
> > /Bruce
> > 
> 
> OK, but then the suggested comment needs to be fixed. It makes it seem 
> like registering the handler is the "non fatal" part. Perhaps something 
> like:
> 
> /* failed register is not a fatal error - callback can be registered 
> later */

Of course! I had prepared this:
/* registration can fail, callback can be registered later */
diff mbox series

Patch

diff --git a/lib/librte_eal/linux/eal/eal_alarm.c b/lib/librte_eal/linux/eal/eal_alarm.c
index 840ede780..d6d70e8c3 100644
--- a/lib/librte_eal/linux/eal/eal_alarm.c
+++ b/lib/librte_eal/linux/eal/eal_alarm.c
@@ -137,9 +137,13 @@  rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
 
 	rte_spinlock_lock(&alarm_list_lk);
 	if (!handler_registered) {
-		ret |= rte_intr_callback_register(&intr_handle,
+		ret = rte_intr_callback_register(&intr_handle,
 				eal_alarm_callback, NULL);
-		handler_registered = (ret == 0) ? 1 : 0;
+		if (ret == 0)
+			handler_registered = 1;
+		else
+			/* not fatal, callback can be registered later */
+			ret = 0;
 	}
 
 	if (LIST_EMPTY(&alarm_list))