Re: [bug-inetutils] [patches 1,2,3] Making tftp/tftpd IPv6-capable.

[Guillem Jover]

Hi!

On Mon, 2010-09-06 at 11:36:12 +0200, Mats Erik Andersson wrote:
> we have a clash of two objectives here. Support OpenBSD, or not?

Well, I'm not a GNU maintainer for inetutils, so my opinion is
non-authoritative, but I'd say portability is important, more so to
the BSDs.

> söndag den  5 september 2010 klockan 23:05 skrev Guillem Jover detta:
> > On Thu, 2010-08-19 at 10:39:04 +0200, Mats Erik Andersson wrote:
> > > diff --git a/libinetutils/sockaddr_aux.c b/libinetutils/sockaddr_aux.c
> > > new file mode 100644
> > > index 0000000..f882f6e
> > > --- /dev/null
> > > +++ b/libinetutils/sockaddr_aux.c
> > > @@ -0,0 +1,98 @@
> > > +get_port (struct sockaddr *sa)
> > > +set_port (struct sockaddr *sa, in_port_t port)
> > 
> > These two functions can be completely avoided if the port is passed as
> > the service name in the getaddrinfo calls in the code, it will also
> > substantially reduce the current code.
> 
> Not all uses are related to getaddrinfo(3), getnameinfo(3) calls.
> Both functions are there to extract port information for printing.
> Their use in tftp/tftpd mirror exactly the old port manipulations.
> If you indend a complete rewrite of the code, be my guest.

I might not be looking at the right place, but I don't see where it
might use the ports for printing, the only places where it does print
something is in command usage, but then there we know the port already
as it's a string argument.

> > > +get_socklen (struct sockaddr *sa)
> > 
> > This one is also unneeded, you should just use sockaddr_storage for
> > storage as it's guaranteed to be able to store any socket address,
> > then sizeof of the sockaddr_storage type or variable should just
> > work fine.

> This function is absolutely necessary to get support for OpenBSD.
> I had to redo the whole patch set once I did observe this fact.
> The frasing "should work fine" is utterly inapplicable here.

Oh right, all BSD expect the socklen_t argument to match exactly with
the sockaddr length of the specific protocol.

Anyway, the function is still not needed, you always know the size of
the sockaddr. Either from getaddrinfo's ai_addrlen in struct addrinfo
or from recvfrom's addrlen argument, for example.


> > > diff --git a/libinetutils/tftpsubs.c b/libinetutils/tftpsubs.c
> > > index 6eb0a09..371764b 100644
> > > --- a/libinetutils/tftpsubs.c
> > > +++ b/libinetutils/tftpsubs.c
> > > @@ -287,7 +287,7 @@ synchnet (int f)
> > >  {
> > >    int i, j = 0;
> > >    char rbuf[PKTSIZE];
> > > -  struct sockaddr_in from;
> > > +  struct sockaddr_storage from;
> > >    socklen_t fromlen;
> > >  
> > >    while (1)
> > 
> > Shouldn't this hunk be in the next patch instead of this one anyway?
> 
> It went there since it is independent of any changes to src/tftp.c
> and tftpd.c, and since the change is correct also for IPv4-only
> settings.

Checking the actual source, it seems to be unneeded, as it's not using
from nor fromlen at all, so switching recvfrom() to recv() should do it
too.


> > > diff --git a/src/tftpd.c b/src/tftpd.c
> > > index f343f8a..892c709 100644
> > > --- a/src/tftpd.c
> > > +++ b/src/tftpd.c
> > > @@ -103,8 +104,9 @@ static int maxtimeout = 5 * TIMEOUT;
> > >  #define PKTSIZE  SEGSIZE+4
> > >  static char buf[PKTSIZE];
> > >  static char ackbuf[PKTSIZE];
> > > -static struct sockaddr_in from;
> > > +static struct sockaddr_storage from;
> > >  static socklen_t fromlen;
> > > +static char host[INET6_ADDRSTRLEN];
> > 
> > Probably better to reduce the scope of host and place it inside
> > verifyhost which is the only user (although keeping it static), which
> > implies it will be non-reentrant, but that's currently the case anyway.
> > Also use NI_MAXHOST instead of INET6_ADDRSTRLEN from <netdb.h>.
> 
> It is a numerical resolution. Right?

Actually no, it should be a host resolution, which degrades to a
numeric address in case the resolving fails, which is what getnameinfo
will do by default if NI_NUMERICHOST is not passed to it. Which will map
to what gethostbyaddr + inet_ntoa were doing previously.


> > > diff --git a/src/tftp.c b/src/tftp.c
> > > index f3b3760..e2cfe4a 100644
> > > --- a/src/tftp.c
> > > +++ b/src/tftp.c
> > > @@ -338,6 +363,7 @@ setpeer (int argc, char *argv[])
> > >      case RESOLVE_FAIL:
> > >        return;
> > >  
> > > +#if 0
> > 
> > ??
> 
> I inactivate old code, which I am not certain whether it
> need be reinserted.

Right, sorry I was not clear, I meant that this should either remove
the code block or replace with new code.

Looking at setpeer and resolve_name, it seems it's just a way to
gracefully handle unresolvable hosts w/o terminating the program, so
changing the RESOLVE_NOT_RESOLVED case to just:

        connected = 0;
        printf ("%s: unknown host\n", argv[1]);
        return;

should preserve the intent of the previous code, as getaddrinfo will
handle hostname and addresses in dotted format.

regards,
guillem