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 <tho...@monjalon.net> wrote:
26/06/2019 13:20, David Marchand:
On Wed, Jun 26, 2019 at 12:41 PM Thomas Monjalon <tho...@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 */
--
Thanks,
Anatoly
