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.