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

Antonio Quartulli
2014-02-11T16:26:23Z

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

On 11/02/14 17:11, Lew Pitcher wrote:
> 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.

Everybody is welcome! :)

>
> 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;
> ?
>

Actually I already fixed it like this:

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)
+ return ret;
+
+ ret = batadv_algo_register(&batadv_batman_v);
+ if (ret < 0)
+ batadv_recv_handler_unregister(BATADV_ELP);
+
+ return ret;

I think it is easier to read, no?
Anyway, this chunk is changed later b patch 7/23 because we add the OGM2
registration.

Cheers,

--
Antonio Quartulli

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]