[hipl-dev] Re: [Merge] lp:~hipl-core/hipl/libhip into lp:hipl

  • From: Stefan Götz <stefan.goetz@xxxxxx>
  • To: mp+93782@xxxxxxxxxxxxxxxxxx
  • Date: Sun, 26 Feb 2012 08:25:22 -0000

Review: Needs Fixing

 review needs-fixing

Sorry, Stefan the bean counter is back...
>  /**
> + * Deletes the first node from list which has the same ptr given.

well, maybe "which contains the payload pointer ptr"?

> + * If there is no match, does nothing.
> + *
> + * @param linkedlist   list to remoe an element from

remo[v]e

> + * @param ptr          pointer by which to identify the node
> + * @param free_element a function pointer to a function for freeing the 
> memory
> + *                     allocated for an element at a node or NULL if the 
> element
> + *                     itself is not to be freed.
> + */
> +void hip_ll_del_by_ptr(struct hip_ll *linkedlist, void *ptr,
> +                       free_elem_fn free_element)

const correctness: struct hip_ll *const linkedlist, const void *const ptr

To separate memory management from list management, I would find it a cleaner
API to not pass in a free function pointer but to return a void* pointer. If ptr
was found in the list and removed, the function should then return ptr itself.
If ptr was not found in the list, the function should return NULL. In any case,
the caller can free the memory for ptr (if they intend to do so) by calling
free(hip_ll_del_by_ptr(list, ptr)); This separates concerns and is still very
easy to use.

> +{
> +    struct hip_ll_node *curr;
> +    struct hip_ll_node *tmp;
> +
> +    /* match first list node */
> +    if (linkedlist != NULL && linkedlist->element_count > 0

the check for linkedlist != NULL should also cover the second part of this
function. I'd suggest adding an assertion for it.

> +        && linkedlist->head->ptr == ptr) {
> +        tmp              = linkedlist->head;
> +        linkedlist->head = tmp->next;
> +        linkedlist->element_count--;
> +        if (free_element != NULL) {
> +            free_element(tmp->ptr);
> +        }
> +        free(tmp);
> +        return;
> +    }
> +
> +    /* match the rest list */

rest [of ]the list?

> +    tmp = linkedlist->head;
> +    for (curr = tmp->next; curr != NULL; curr = curr->next) {
> +        if (curr->ptr == ptr) {
> +            tmp->next = curr->next;
> +            linkedlist->element_count--;
> +            if (free_element != NULL) {
> +                free_element(curr->ptr);
> +            }
> +            free(curr);
> +            return;
> +        }
> +        tmp = tmp->next;
> +    }
> +}

Why not go with Christof's version? It looked much simpler to me.

> +void hip_ll_del_by_ptr(struct hip_ll *linkedlist, void *ptr,
> +                       free_elem_fn free_element);

const correctness

>  /**
> + * Read a control message over TCP socket.

What exactly does this function do? What happens after the message is read? Is
it returned to the caller? Sent to another processing function? /dev/null? The
CIA? Paradise?

> + * @param  sockfd         a socket file descriptor
> + * @param  ctx            a pointer to the packet context
> + * @return                -1 in case of an error, 0 otherwise.
> + */
> +int hip_read_control_msg_tcp(int sockfd, struct hip_packet_context *ctx)

const correctness

> +int hip_read_control_msg_tcp(int sockfd, struct hip_packet_context *ctx);

const correctness


> --- hipd/hadb.c       2012-02-17 10:45:47 +0000
> +++ lib/hipdaemon/hadb.c      2012-02-20 08:33:22 +0000
> @@ -616,7 +616,12 @@
>  
>      if (hip_select_source_address(&peer_map.our_addr, &peer_map.peer_addr)) {
>          HIP_ERROR("Cannot find source address\n");
> -        return -1;
> +        if (hipl_is_libhip_mode()) {
> +            memset(&peer_map.our_addr, 0, sizeof(peer_map.our_addr));

How about 'peer_map.our_addr = INVALID_ADDR' with a suitable definition (and
probably a better name for) INVALID_ADDR? It would reveal the intention of this
code more clearly

> +            HIP_DEBUG("Using ANY for source address\n");
> +        } else {
> +            return -1;
> +        }

I couldn't finish reviewing, maybe later. In any case, const correctness for all
further functions, please :)

Stefan


-- 
https://code.launchpad.net/~hipl-core/hipl/libhip/+merge/93782
Your team HIPL core team is subscribed to branch lp:hipl.

Other related posts: