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

[Lew Pitcher <lew.pitcher@digitalfreehold.ca>]

[-- 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 --]