Hello Samuel, Thanks for taking your time reviewing my code.
2017-09-27 0:44 GMT+02:00 Samuel Thibault <samuel.thiba...@gnu.org>: >> +error_t >> +lwip_S_io_write (struct sock_user * user, >> + char *data, >> + size_t datalen, >> + off_t offset, mach_msg_type_number_t * amount) >> +{ > ... >> + if (sockflags & O_NONBLOCK) >> + m.msg_flags |= MSG_DONTWAIT; >> + sent = lwip_sendmsg (user->sock->sockno, &m, 0); > > I'm wondering about thread-safety: I guess you enabled the unix arch to > get pthread mutex locks? Yes, lwip provides its own locking system that uses libpthread in Unix platforms. >> +static error_t >> +lwip_io_select_common (struct sock_user *user, >> + mach_port_t reply, >> + mach_msg_type_name_t reply_type, >> + struct timeval *tv, int *select_type) >> +{ > ... >> + timeout = tv ? tv->tv_sec * 1000 + tv->tv_usec / 1000 : -1; >> + ret = lwip_poll (&fdp, nfds, timeout); > > Better use lwip_ppoll to have better timeout resolution, and use struct > timespec instead of struct timeval. Unfortunately, lwip doesn't provide a lwip_ppoll() function. >> + if (addr6_prefix_len) >> + for (i = 0; i < LWIP_IPV6_NUM_ADDRESSES; i++) >> + *(addr6_prefix_len + i) = 64; > > Does lwip support only /64 IPv6 networks? Yes. >> +error_t >> +trivfs_goaway (struct trivfs_control *fsys, int flags) >> +{ >> + exit (0); >> +} > > The linuxish pfinet returns EBUSY if there are still sockets, it is a > useful thing to check. There are a few things I don't understand in pfinet's trivfs_goaway()[1]. 1.- Why inhibit pfinet_{cntl,protid}_portclasses[0] and not pfinet_{cntl,protid}_portclasses[1]. Wouldn't that lead to stop RPCs to /servers/socket/2 but still allow RPCs to /servers/socket/26? 2.- Why inhibit socketport_class and not addrpot_class? 3.- Why use ports_inhibit_class_rpcs() for inhibiting all classes, but then use ports_enable_class() just for socketport_class and ports_resume_class_rpcs() for all other classes? >> + if (pi) >> + { >> + ports_port_deref (pi); >> + >> + mig_routine_t routine; >> + if ((routine = lwip_io_server_routine (inp)) || >> + (routine = lwip_socket_server_routine (inp)) || >> + (routine = lwip_pfinet_server_routine (inp)) || >> + (routine = lwip_iioctl_server_routine (inp)) || >> + (routine = NULL, trivfs_demuxer (inp, outp))) > > In the linuxish pfinet, the startup protocol is also used, to nicely > close net channels before exiting, that's a useful thing to do. I have kind of the same question about S_startup_dosync()[2]. Why destroy only socketport_class ports and not addrport_class ones? >> + case 'A': >> + /* IPv6 address */ >> + if (arg) >> + { > >> + >> + for (i = 0; i < LWIP_IPV6_NUM_ADDRESSES; i++) >> + { >> + address6 = (ip6_addr_t *) & h->curint->addr6[i]; >> + >> + /* Is the slot free? */ >> + if (!ip6_addr_isany (address6)) >> + continue; >> + >> + /* Use the slot */ >> + if (ip6addr_aton (arg, address6) <= 0) >> + PERR (EINVAL, "Malformed address"); >> + >> + break; >> + } > > There should be some error thrown if we couldn't find a free slot. It was at [3] but didn't print the particular address. I changed it to do it. > Last but not least, did you try to run the glibc testsuite while running > this TCP/IP stack? It would probably find potential bugs. Also, the > perl testsuite is probably a nice thing to run. I need help here. I've been googling but haven't found any good reference to know what should I do exactly. Do you have any references? Everything else is done in my Github, I'll provide a patch when I finish all issues. ----------------------------- [1] http://git.savannah.gnu.org/cgit/hurd/hurd.git/tree/pfinet/main.c#n477 [2] http://git.savannah.gnu.org/cgit/hurd/hurd.git/tree/pfinet/main.c#n137 [3] https://github.com/jlledom/lwip-hurd/blob/master/lwip-util.c#L216