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

  • From: Xin <eric.nevup@xxxxxxxxx>
  • To: mp+92480@xxxxxxxxxxxxxxxxxx
  • Date: Fri, 17 Feb 2012 16:49:34 -0000

On 14/02/12 22:34, Christof Mroz wrote:
> Review: Needs Fixing
>
> Implementation looks good overall, and splitting the code should make HIPL 
> more flexible for users.
> You may ignore my comments about code that was not newly written but just 
> moved, of course.
>
>> +int hip_accept(int fd, struct sockaddr *new_peer_hit, socklen_t *hit_len)
>> +{
>> +    int                     new_fd;
>> +    struct hip_fd_info     *fd_info     = NULL;
>> +    struct hip_fd_info     *fd_info_new = NULL;
>> +    struct sockaddr_storage ss          = { 0 };
>> +    socklen_t               ss_len      = sizeof(struct sockaddr_storage);
>> +    struct sockaddr_in6    *phit        = NULL;
>> +
>> +    if (*hit_len<  sizeof(struct sockaddr_in6)) {
>> +        return -1;
>> +    }
>> +
>> +    if ((fd_info = hip_socket_get_info(fd)) == NULL) {
>> +        HIP_ERROR("Fd %d is not a hip socket, exiting.\n", fd);
>> +        return -1;
>> +    }
>> +
>> +    new_fd = accept(fd, (struct sockaddr *)&ss,&ss_len);
>> +    if (new_fd<  0) {
>> +        HIP_PERROR("accept(): ");
>> +        return -1;
>> +    }
>> +
>> +    fd_info_new = create_new_fd_info(new_fd, fd_info->bound_port,
>> +                                     fd_info->family, fd_info->proto);
>> +
>> +    if (hip_await_bex(fd_info_new, (struct sockaddr *)&ss)<  0) {
>> +        HIP_ERROR("Base exchange not successful.\n");
>> +        return -1;
>> +    }
>> +
>> +    if (new_peer_hit) {
>> +        phit = (struct sockaddr_in6 *) new_peer_hit;
>> +        memset(phit, 0, *hit_len);
>> +        memcpy(&phit->sin6_addr,&fd_info_new->ha->hit_peer,
>> +               sizeof(struct in6_addr));
>> +        phit->sin6_port = get_port_from_saddr((struct sockaddr *)&ss);
>> +        *hit_len        = sizeof(struct sockaddr_in6);
>> +    }
>> +
>> +    return new_fd;
>> +}
> fd_info_new is not freed.

Those fd_infoS are maintained in a global list, so cannot be freed here. 
the function hipl_close (previously hip_close) handle the case of free 
fd_info.

>
>> +    /* check server&  client status */
>> +    for (i = 0; i<  TEST_HIPNC_TIMEOUT; i++) {
>> +        tv.tv_sec  = 1;
>> +        tv.tv_usec = 0;
>> +        select(0, NULL, NULL, NULL,&tv);
>> +        round = remain_cld;
>> +        for (j = 0; j<  round; j++) {
>> +            pid = waitpid(-1,&status, WNOHANG);
>> +            fail_if(pid>  0&&  status != 0, "hipnetcat failed");
>> +            if (pid>  0&&  status == 0) {
>> +                remain_cld--;
>> +                if (remain_cld == 0) {
>> +                    break;
>> +                }
>> +            }
>> +        }
>> +    }
>> +
>> +    fail_if(remain_cld>  0, "hipnetcat test timeout!");
>> +}
> I guess that the daemon and firewall store much (all?) of their state 
> statically, and this forces you to spawn separate processes even though 
> that's an ugly thing to do in a test (which should be self-contained without 
> side effects), correct?
>
> For completeness' sake I'd suggest an OpenGL-ish approach: simply provide two 
> functions that retrieve or store the global state, respectively. Only those 
> portions relevant to library users, of course.
> Then you can restructure the test as follows
>
> void *ctx[2];
>
> libhip_init();
> ctx[0] = libhip_get_context();
> libhip_init();
> ctx[1] = libhip_get_context();
>
> libhip_set_context(ctx[0]);
> send(...);
> libhip_set_context(ctx[1]);
> recv(...);
> check stuff
>
> libip_destroy_context(ctx[0]);
> libip_destroy_context(ctx[1]);
>
> Note that returning anything else than void* will break legacy libhip users 
> whenever the context struct changes.
> I can understand if you don't have time to waste on that interface, though :)
>

This test can verify both libhipl and hipnetcat. The hipnetcat cannot 
act as client and server at the same time (like normal netcat), that's 
why we have 2 processes.

Your advice gives me a good hint for the improvement of the libhipl API. 
There are still many things to explore, and my graduation relies on it :)


Thanks for your comments, other issues are fixed accordingly.

Xin

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

Other related posts: