Re: [PATCH] LwIP translator

[Samuel Thibault]

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