Re: [PATCH] LwIP translator

[Joan Lledó]

Hello Samuel,

Thanks for taking your time reviewing my code.

2017-09-27 0:44 GMT+02:00 Samuel Thibault <samuel.thibault@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