Re: [B.A.T.M.A.N.] [RFC 04/23] batman-adv: ELP - creating neighbor structures

Lew Pitcher
2014-02-11T16:11:27Z

[-- Attachment #1: Type: text/plain, Size: 2691 bytes --]

On Tuesday 11 February 2014 11:02:11 Antonio Quartulli wrote:
> On 11/02/14 16:32, Andrew Lunn wrote:
> > On Tue, Feb 11, 2014 at 01:48:04PM +0100, Antonio Quartulli wrote:
> >> From: Linus Luessing <linus.luessing@web.de>
> >>
> >> Initially developed by Linus during a 6 months trainee study
> >> period in Ascom (Switzerland) AG.
> >>
> >> Signed-off-by: Linus Luessing <linus.luessing@web.de>
> >> Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
> >> Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
> >> ---
> >> bat_v.c | 18 +++++-
> >> bat_v_elp.c | 204
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >> bat_v_elp.h | 6 ++
> >> main.h | 2 +
> >> types.h | 53 ++++++++++++++++
> >> 5 files changed, 281 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/bat_v.c b/bat_v.c
> >> index 7247d7f..bed5e00 100644
> >> --- a/bat_v.c
> >> +++ b/bat_v.c
> >> @@ -61,5 +61,21 @@ static struct batadv_algo_ops batadv_batman_v
__read_mostly = {
> >>
> >> int __init batadv_v_init(void)
> >> {
> >> - return batadv_algo_register(&batadv_batman_v);
> >> + int ret;
> >> +
> >> + /* batman v echo location protocol packet */
> >> + ret = batadv_recv_handler_register(BATADV_ELP,
> >> + batadv_v_elp_packet_recv);
> >> + if (ret < 0)
> >> + goto elp_unregister;
> >> +
> >> + ret = batadv_algo_register(&batadv_batman_v);
> >> +
> >> + return ret;
> >> +
> >> +elp_unregister:
> >> + if (ret < 0)
> >> + batadv_recv_handler_unregister(BATADV_ELP);
> >
> > No need to check ret here. If we are here, it has to be < 0.
> >
> > It also seems odd to me you are unregistering the handler when the
> > registration of the handler fails!
> >
> > I suspect the first if (ret < 0) should be followed by a plain return
> > ret; and there should be a second test for the return value of
> > batadv_algo_register() which should goto the label and unregister the
> > handler.
>
> I totally agree with what you said.
> We should jump to elp_unregister if batadv_algo_register() fails.

Sorry to break in here, but the (ex) professional programmer in me just /has/
to comment.

Why not just
int __init batadv_v_init(void)
{
- return batadv_algo_register(&batadv_batman_v);
+ int ret;
+
+ /* batman v echo location protocol packet */
+ ret = batadv_recv_handler_register(BATADV_ELP,
+ batadv_v_elp_packet_recv);
+
+ if (ret >= 0)
+ ret = batadv_algo_register(&batadv_batman_v);
+ else
+ batadv_recv_handler_unregister(BATADV_ELP);
+
+ return ret;
?
--
Lew Pitcher
"In Skills, We Trust"
PGP public key available upon request

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 227 bytes --]