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.