[hipl-dev] Re: [Merge] lp:~stefan.goetz/hipl/esp-destination-addresses into lp:hipl
- From: Stefan Götz <stefan.goetz@xxxxxxxxxxxxxxxxx>
- To: mp+55259@xxxxxxxxxxxxxxxxxx
- Date: Tue, 29 Mar 2011 23:31:29 -0000
Hi Christof!
Thanks so much for testing - I really appreciate it!
Wasn't there a plan about some automated testbed? What happened to
that? It'd be fairly cool if one could inject one's branches into such
a testbed for this kind of testing...
>> + while (node) {
>> + struct esp_address *const esp_addr = node->ptr;
>
> const struct esp_address *const
ok
>> + if ((esp_addr = malloc(sizeof(*esp_addr)))) {
>> + esp_addr->dst_addr = *addr;
>> esp_addr->update_id = NULL;
>> + if (upd_id == NULL || (esp_addr->update_id =
>> malloc(sizeof(*esp_addr->update_id)))) {
>> + *esp_addr->update_id = *upd_id;
>
> Looks like if upd_id == NULL, the branch will be entered and a NULL
> dereference triggered.
> Also, the return value of malloc() is not checked for NULL.
ok - obviously, I got the logic wrong there.
> Did you find out why the update ID is dynamically allocated in the first
> place?
Yes: the update_id for a given destination address may be known or
not. Its value range occupies 32 bits and no magic value is reserved
to indicate the 'unknown' state. Thus, a VCP (very clever person)
decided to encode the 'unknown' state as a NULL pointer. If the
pointer is not NULL, it points to the actual known update ID.
I planned to replace this memory-leak infested mess in a different branch.
> Code duplication can be avoided via
>
> while (addr = hip_ll_del_first(...)) { ... }
Doh - of course!
>> + HIP_ASSERT(esp_info);
>> + HIP_ASSERT(locator);
>> + HIP_ASSERT(seq);
>> + HIP_ASSERT(tuple);
>> + HIP_ASSERT(esp_info->new_spi == esp_info->old_spi);
>
> That last assertion looks odd: could this be handled gracefully? I don't know
> what the RFC says on this though.
The way I understand the code, this is indeed an assertion about code
consistency, i.e. no outsider can trigger this assertion, only someone
who messes up the code in an unintended way.
>> + HIP_IFEL((new_esp = calloc(1, sizeof(*new_esp))) == NULL, -1,
>> + "Allocating esp_tuple object failed");
>> new_esp->spi = ntohl(esp_info->new_spi);
>> new_esp->tuple = tuple;
>> + hip_ll_init(&new_esp->dst_addresses);
>
> Is this still the state of the art, with your newly added initializer macro?
Yes - the initializer macro is only for static initializations.
>> + for (unsigned idx = 0; idx < addresses_in_locator; idx += 1) {
>
> Now I'm even more confused about the C99 thing... Please, please tell me that
> this declaration is allowed :)
Diego just gave it his blessing :)
>> + struct esp_address *const esp_address =
>> + malloc(sizeof(*esp_address));
>> + HIP_IFEL(esp_address == NULL, -1,
>> + "Allocating esp_address object for address %i failed",
>> + idx);
>> + esp_address->dst_addr = addresses[idx].address;
>> + HIP_IFEL(hip_ll_add_first(&new_esp->dst_addresses, esp_address)
>> + != 0, -1,
>> + "Appending esp_address object %i to list of
>> destination addresses in ESP tuple failed",
>> + idx);
>> + HIP_IFEL((esp_address->update_id =
>> + malloc(sizeof(*esp_address->update_id))) == NULL,
>> -1,
>> + "Allocating update_id object for address %i failed",
>> + idx);
>> + *esp_address->update_id = seq->update_id;
>> }
>> +
>> + return new_esp;
>> }
>> - return new_esp;
>> +
>> +out_err:
>> + free_esp_tuple(new_esp);
>> + return NULL;
>> }
>
> The esp_address allocated inside the loop is not deallocated if
> hip_ll_add_first() fails.
Good catch. Will fix it.
> It's also easier to follow IMO if you deallocated it explicitly and not
> relied on free_esp_tuple() to clean up a half-initialized item.
I agree. Will do.
Thanks for the review!
Stefan
--
https://code.launchpad.net/~stefan.goetz/hipl/esp-destination-addresses/+merge/55259
Your team HIPL core team is subscribed to branch lp:hipl.
Other related posts:
