bug-inetutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Mats Erik Andersson
Subject: Re: [bug-inetutils] [patches 1,2,3] Making tftp/tftpd IPv6-capable.
Date: Mon, 6 Sep 2010 11:36:12 +0200
User-agent: Mutt/1.5.18 (2008-05-17)

Hello,

we have a clash of two objectives here. Support OpenBSD, or not?

söndag den  5 september 2010 klockan 23:05 skrev Guillem Jover detta:
> Hi!
> 
> 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 @@
> [...]
> > +/* A collection of helpers intended to handle IPv6 and IPv4 simultaneously
> > +   in a transparent manner. An underlying use of 'struct sockaddr_storage'
> > +   is conceiled as 'struct sockaddr' in the call from an application.
> > +
> > +   The helper function resolves and manipulates this as 'struct 
> > sockaddr_in'
> > +   or as 'struct sockaddr_in6', depending on contents.
> > +
> > +   Mats Erik Andersson, August 2010
> > +*/
> 
> These helper functions are not needed, and will hinder usage of other
> protocols transparently, one of the points of the new inet API is that
> it's transparent and does not need to care for the specific protocols
> explicitly. See below for detailed comments.
> 
> > +in_port_t
> > +get_port (struct sockaddr *sa)
> > +{
> > +  switch (sa->sa_family)
> > +    {
> > +      case AF_INET:
> > +   return ntohs (((struct sockaddr_in *) sa)->sin_port);
> > +      case AF_INET6:
> > +   return ntohs (((struct sockaddr_in6 *) sa)->sin6_port);
> > +      default:
> > +   return 0;
> > +    }
> > +}
> 
> > +void
> > +set_port (struct sockaddr *sa, in_port_t port)
> > +{
> > +  switch (sa->sa_family)
> > +    {
> > +      case AF_INET:
> > +   (((struct sockaddr_in *) sa)->sin_port) = htons (port);
> > +   break;
> > +      case AF_INET6:
> > +   (((struct sockaddr_in6 *) sa)->sin6_port) = htons (port);
> > +      default:
> > +   break;
> > +    }
> > +}
> 
> 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.

> 
> > +socklen_t
> > +get_socklen (struct sockaddr *sa)
> > +{
> > +  switch (sa->sa_family)
> > +    {
> > +      case AF_INET:
> > +   return sizeof (struct sockaddr_in);
> > +   break;
> > +      case AF_INET6:
> > +   return sizeof (struct sockaddr_in6);
> > +   break;
> > +      default:
> > +   return sizeof (*sa);
> > +   break;
> > +    }
> > +}
> 
> 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.

G. Jover has never programmed a socket operation in OpenBSD,
he is clearly restricted to GNU/Linux, GNU/FreeBSD, or GNU/Hurd.
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.

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

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

> 
> > @@ -270,24 +272,28 @@ main (int argc, char *argv[])
> >     exit (0);
> >        }
> >    }
> > -  from.sin_family = AF_INET;
> > +  /* The peer's address 'from' is valid at this point,
> > +   * and 'from.ss_family' contains the correct address
> > +   * family for any callback connection. */
> 
> Maybe place this just before the socket call?
> 
> >    alarm (0);
> >    close (0);
> >    close (1);
> > -  peer = socket (AF_INET, SOCK_DGRAM, 0);
> > +  peer = socket (from.ss_family, SOCK_DGRAM, 0);
> >    if (peer < 0)
> >      {
> >        syslog (LOG_ERR, "socket: %m\n");
> >        exit (1);
> >      }
> 
> 
> 
> 
> > diff --git a/src/tftp.c b/src/tftp.c
> > index f3b3760..e2cfe4a 100644
> > --- a/src/tftp.c
> > +++ b/src/tftp.c
> > @@ -277,26 +266,62 @@ char *hostname;
> >  static int
> >  resolve_name (char *name, int allow_null)
> >  {
> > -  struct hostent *hp = gethostbyname (name);
> > -  if (hp == NULL)
> > +  int rc;
> > +  struct sockaddr_storage ss;
> > +  struct addrinfo hints, *ai, *aiptr;
> > +
> > +  memset (&hints, '\0', sizeof (hints));
> 
> The structure is made of bytes, not chars, so probably better to use 0
> instead of '\0'. Same for the other ones.
> 
> > +  hints.ai_family = AF_UNSPEC;
> > +  hints.ai_socktype = SOCK_DGRAM;
> > +  hints.ai_flags = AI_CANONNAME;
> > +#ifdef AI_ADDRCONFIG
> > +  hints.ai_flags += AI_ADDRCONFIG;
> > +#endif
> 
> While this should work just fine, I think it'd be better to use a |=,
> as they are bit flags, and thus guarantees that even if AI_ADDRCONFIG
> would get added again by accident for example, no wrong results would
> happen.
> 

Observant!

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

> 
> >      case RESOLVE_NOT_RESOLVED:
> >        peeraddr.sin_family = AF_INET;
> >        peeraddr.sin_addr.s_addr = inet_addr (argv[1]);
> > @@ -348,9 +374,10 @@ setpeer (int argc, char *argv[])
> >       return;
> >     }
> >        hostname = xstrdup (argv[1]);
> > +#endif
> 
> regards,
> guillem

-- 
Mats Erik Andersson, fil. dr
<address@hidden>
2459 41E9 C420 3F6D F68B  2E88 F768 4541 F25B 5D41

Abonnerar på: debian-mentors, debian-devel-games, debian-perl,
              debian-ipv6, debian-qa



reply via email to

[Prev in Thread] Current Thread [Next in Thread]