Re: [PATCH] LwIP translator

[Samuel Thibault]

Hello,

Joan Lledó, on mer. 18 oct. 2017 09:24:22 +0200, wrote:
> >> +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.

Ah, I saw it in lwipv6, I thought it was generally available.

That being said, it'd be better to make the code already use struct
timespec, converting to coarser resolution only where is has to be done
(the lwip_poll call).

> >> +      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.

Ok :/

> >> +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?

That's probably just a bug, yes.

> 2.- Why inhibit socketport_class and not addrpot_class?

I guess that's because it's not "too bad" for the addrport RPCs to
break, and they are usually used very transiently while a socket is
still active.  But for safety it is probably better to inhibit it.
I guess the cases where there wouldn't be any socket left but some
addrport left is rare enough that we don't care.

> 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?

I have to say I don't know :)
I don't know if that was really tested actually. From reading the source
it seems it should really be replaced by ports_resume_class_rpcs.

> >> +  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?

I guess it's the same answer as above: we can as well destroy them too
for nicer handling.

Samuel