Hello, Joan Lledó, on mer. 06 sept. 2017 10:09:36 +0200, wrote: > --- /dev/null > +++ b/lwip/iioctl-ops.c
> +/* Get the interface from its name */ > +static struct netif * > +get_if (char *name) ... > + > + netif = netif_list; > + while (netif != 0) > + { > + if (strcmp (netif_get_state (netif)->devname, ifname) == 0) > + break; > + > + netif = netif->next; > + } Rather use a for loop: for (netif = netif_list; netif != 0; netif = netif->next) that's much more trustable :) > +/* Get some sockaddr type of info. */ > +static kern_return_t > +siocgifXaddr (struct sock_user *user, > + ifname_t ifnam, sockaddr_t * addr, enum siocgif_type type) > +{ > + error_t err = 0; > + struct sockaddr_in *sin = (struct sockaddr_in *) addr; > + struct netif *netif; > + size_t buflen = sizeof (struct sockaddr); > + uint32_t addrs[4]; > + err = lwip_getsockname (user->sock->sockno, addr, (socklen_t *) & buflen); Here, perhaps comment that we are only interested in checking the address family, thus only sizeof (struct sockaddr) > +/* 100 SIOCGIFINDEX -- Get index number of a network interface. */ > +error_t > +lwip_S_iioctl_siocgifindex (struct sock_user * user, > + ifname_t ifnam, > + int *index) > +{ > + error_t err = 0; > + struct netif *netif; > + int i; > + > + if (!user) > + return EOPNOTSUPP; > + > + i = 1; /* The first index must be 1 */ > + netif = netif_list; > + while (netif != 0) > + { > + if (strcmp (netif_get_state (netif)->devname, ifnam) == 0) > + { > + *index = i; > + break; > + } > + > + netif = netif->next; > + i++; Similarly, it makes the reader more confident to read a for loop here. > +/* 101 SIOCGIFNAME -- Get name of a network interface from index number. */ > +error_t > +lwip_S_iioctl_siocgifname (struct sock_user * user, > + ifname_t ifnam, > + int *index) > +{ And there again :) > diff --git a/lwip/io-ops.c b/lwip/io-ops.c > new file mode 100644 > index 00000000..2553ca33 > --- /dev/null > +++ b/lwip/io-ops.c > +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? Also, why using lwip_sendmsg instead of just lwip_send? That'd avoid the construction of m etc. In the linuxish pfinet case, it's just because there is no recv method :) > +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. > diff --git a/lwip/lwip-util.c b/lwip/lwip-util.c > new file mode 100644 > index 00000000..2996632e > --- /dev/null > +++ b/lwip/lwip-util.c > + /* Freed in the module terminate callback */ > + ifc->devname = malloc (strlen (name) + 1); > + if (ifc->devname) > + { > + memset (ifc->devname, 0, strlen (name) + 1); > + strncpy (ifc->devname, name, strlen (name)); > + } Err, rather just use strdup (name)? :) > + 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? > --- /dev/null > +++ b/lwip/main.c > +#ifndef _GNU_SOURCE > +#define _GNU_SOURCE > +#endif _GNU_SOURCE has to be defined *before* including headers. And we always define _GNU_SOURCE while building hurd translators, this it *is* the GNU system, so we use the GNU standard :) > +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. > + 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. > diff --git a/lwip/options.c b/lwip/options.c > new file mode 100644 > index 00000000..b7b34fd8 > --- /dev/null > +++ b/lwip/options.c > @@ -0,0 +1,346 @@ > + 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. > + netif = netif_list; > + while (netif != 0) > + { Again, for loop :) > --- /dev/null > +++ b/lwip/pfinet-ops.c > +static void > +dev_ifconf (struct ifconf *ifc) > +{ > + struct netif *netif; > + struct ifreq ifr; > + struct sockaddr_in *saddr; > + char *buf; > + int len; > + > + buf = (char *) ifc->ifc_req; > + len = ifc->ifc_len; > + saddr = (struct sockaddr_in *) &ifr.ifr_addr; > + netif = netif_list; > + while (netif != 0) > + { Ditto. > + if (ifc->ifc_req != 0) > + { > + /* Get the data */ > + if (len < (int) sizeof (struct ifreq)) > + break; > + > + memset (&ifr, 0, sizeof (struct ifreq)); > + > + memcpy (ifr.ifr_name, netif_get_state (netif)->devname, > + strlen (netif_get_state (netif)->devname)); > + saddr->sin_len = sizeof (struct sockaddr_in); > + saddr->sin_family = AF_INET; > + saddr->sin_addr.s_addr = netif_ip4_addr (netif)->addr; > + > + memcpy (buf, &ifr, sizeof (struct ifreq)); Why filling ifr before copying to the buffer? You could use a struct ifreq *ifr = buf; struct sockaddr_in *saddr = (struct sockaddr_in *) &ifr->ifr_addr; > diff --git a/lwip/port-objs.c b/lwip/port-objs.c > new file mode 100644 > index 00000000..e60c6808 > --- /dev/null > +++ b/lwip/port-objs.c > +struct sock_user * > +make_sock_user (struct socket *sock, int isroot, int noinstall, int consume) > +{ > + error_t err; > + struct sock_user *user; > + > + assert (sock->refcnt != 0); Btw, use assert_backtrace everywhere you can, it's much more useful than just assert :) > + if (noinstall) > + err = ports_create_port_noinstall (socketport_class, lwip_bucket, > + sizeof (struct sock_user), &user); > + else > + err = ports_create_port (socketport_class, lwip_bucket, > + sizeof (struct sock_user), &user); > + if (err) > + return 0; > + > + if (!consume) > + ++sock->refcnt; This kind of reference counting needs locking. You can probably use refcount.h to avoid having to add locking. > diff --git a/lwip/port/netif/hurdethif.c b/lwip/port/netif/hurdethif.c > new file mode 100644 > index 00000000..b17aa84a > --- /dev/null > +++ b/lwip/port/netif/hurdethif.c > +error_t > +hurdethif_device_init (struct netif * netif) > +{ > + ethif = malloc (sizeof (hurdethif)); > + if (!ethif) > + { > + LWIP_DEBUGF (NETIF_DEBUG, ("hurdethif_init: out of memory\n")); > + return ERR_MEM; > + } > + memset (ethif, 0, sizeof (hurdethif)); Better use calloc in such cases, to let glibc automatically optimize whenever possible. > + /* Maximum transfer unit: MSS + IP header size + TCP header size */ > + netif->mtu = TCP_MSS + 0x28; Better use 20 + 20 in such case instead of 0x28. > --- /dev/null > +++ b/lwip/socket-ops.c > + struct sock_user *newuser; > + union > + { > + struct sockaddr_storage storage; > + struct sockaddr sa; > + } addr = You don't need to do this, just use sockaddr_storage, and cast the address of addr to (struct sockaddr *), it is defined in a way that makes the cast valid. 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. Apart from that, it looks good to me :) Samuel