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

  • From: Christof Mroz <christof.mroz@xxxxxxxxxxxxxx>
  • To: mp+93782@xxxxxxxxxxxxxxxxxx
  • Date: Mon, 20 Feb 2012 20:03:17 -0000

Review: Needs Fixing

> === modified file 'lib/core/hostid.c'
> --- lib/core/hostid.c 2011-11-10 10:35:47 +0000
> +++ lib/core/hostid.c 2012-02-20 08:33:22 +0000
> @@ -743,11 +744,17 @@
>                  goto out_err;
>              }
>          } else if (!use_default) {
> +            char hi_file_dup[strlen(hi_file) + 1];
> +            strcpy(hi_file_dup, hi_file);
> +            if ((err = check_and_create_dir(dirname(hi_file_dup), 
> HIP_DIR_MODE))) {
> +                HIP_ERROR("Could not create direcory for path: %s\n", 
> hi_file);
> +                goto out_err;
> +            }

I investigated a bit and seems like PATH_MAX is 4096 chars on linux, which is 
probably OK even for stacks on embedded devices, but strdup() would be safer 
nevertheless.
My apologies for advising you to use the stack for that in the last mail (see 
also further below).

Probably unrelated: should this handle recursive mkdir()? 
check_and_create_dir() currently does not, as far as I see.

> === modified file 'lib/core/linkedlist.c'
> --- lib/core/linkedlist.c     2011-08-15 14:11:56 +0000
> +++ lib/core/linkedlist.c     2012-02-20 08:33:22 +0000
> @@ -298,6 +298,51 @@
>  /**
> + * Deletes the first node from list which has the same ptr given.
> + * If there is no match, does nothing.
> + *
> + * @param linkedlist   list to remoe an element from
> + * @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)
> +{
> +    struct hip_ll_node *curr;
> +    struct hip_ll_node *tmp;
> +
> +    /* match first list node */
> +    if (linkedlist != NULL && linkedlist->element_count > 0
> +        && 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 */
> +    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;
> +    }
> +}

Looks like both loops can be merged, at first glance (untested):

if(!linkedlist || linkedlist->element_count == 0) {
        return;
}

struct hip_ll_node **storage = &linkedlist->head;
struct hip_ll_node  *node    =  linkedlist->head;

while(node) {
        if(node->ptr == ptr) {
                *storage = node->next;
                free(node);
                node = *storage;

                linkedlist->elementcount -= 1;
                // could return here for micro optimization
        } else {
                storage = &node->next;
                node    =  node->next;
        }
}

> === renamed file 'hipd/init.c' => 'lib/hipdaemon/init.c'
> --- hipd/init.c       2012-01-18 21:21:26 +0000
> +++ lib/hipdaemon/init.c      2012-02-20 08:33:22 +0000
> @@ -1090,6 +1319,66 @@
> +    /* +2 because we need a slash after pwd and a NULL for termination */
> +    keypath_len = strlen(pwd->pw_dir) +
> +                  strlen(HIP_USER_DIR) +
> +                  strlen(DEFAULT_HOST_RSA_KEY_FILE_NAME) +
> +                  strlen(DEFAULT_PUB_HI_FILE_NAME_SUFFIX) + 2;
> +    char key_path[keypath_len];

As I stated above already: the path length could turn out to be absurdly long, 
so using malloc(), like you did before, was probably the best idea. Sorry again.
-- 
https://code.launchpad.net/~hipl-core/hipl/libhip/+merge/93782
Your team HIPL core team is subscribed to branch lp:hipl.

Other related posts: