Re: [bug-inetutils] [PATCH] Implement IPv6 capability for the TFTP server.

[Alfred M. Szmidt]

   The previous thread

      "[patches 1,2,3] Making tftp/tftpd IPv6-capable."

   is hereby declared closed, and its code is declared dead.
   Well, it does return in a refined form!

Check!  I like this patch more too...

   I am not at all certain that my formulation of the changelog
   entry fulfill all your requirements, so do regard also that
   as a valid reason for patch rejection, besides any further
   nit picking in the C-code itself.

No worries, who ever is OK the patch or committing it has a that task.
Though I'll comment anyway, cause comments are what one learns from...

   +2010-09-12  Mats Erik Andersson <address@hidden>
   +
   +    * libinetutils/tftpsubs.c (synchnet): Upgrade the variable "from"
   +    to be "struct sockaddr_storage".

Linguistically, you changed the type of FROM to `struct
sockaddr_storage'; local variables should be in capital letters.  Thus
this should be something like:

(synchnet): Changed type of FROM to `struct sockaddr_storage'.

   +    * src/tftpd.c: Upgrade all "sockaddr_in" to "sockaddr_storage".

Same here, but this should something like (it is important to mention
what you changed):

(from): Changed ttype of variable to `struct sockaddr_storage'.

Or if you want something general:

Changed all occurences of `struct sockaddr_in' to `struct
sockaddr_stroage'.

   +    Accurate use of "fromlen" to track length of address structure.
   +    (tftp): Let logging display "IPv4", "IPv6, or "?".
   +    (verifyhost): New signature. Uses getnameinfo() as resolver.

When chaning types of functions, it is better to write:

(verifyhost): Change declaration to `...'.


   --- a/src/tftpd.c
   +++ b/src/tftpd.c
   @@ -101,7 +101,7 @@ 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;
   
    void tftp (struct tftphdr *, int);
   @@ -124,7 +124,7 @@ static int logging;
   
    static const char *errtomsg (int);
    static void nak (int);
   -static const char *verifyhost (struct sockaddr_in *);
   +static const char *verifyhost (struct sockaddr_storage *, socklen_t);
   
   @@ -172,7 +172,7 @@ main (int argc, char *argv[])
      int index;
      register struct tftphdr *tp;
      int on, n;
   -  struct sockaddr_in sin;
   +  struct sockaddr_storage sin;
   
      set_program_name (argv[0]);
      iu_argp_init ("tftpd", default_program_authors);
   @@ -268,24 +268,29 @@ main (int argc, char *argv[])
           exit (EXIT_SUCCESS);
          }
      }
   -  from.sin_family = AF_INET;
   +
      alarm (0);
      close (0);
      close (1);
   -  peer = socket (AF_INET, SOCK_DGRAM, 0);
   +
   +  /* The peer's address 'from' is valid at this point.
   +   * 'from.ss_family' contains the correct address
   +   * family for any callback connection, and 'fromlen'
   +   * is the length of the corresponding address structure. */
   +  peer = socket (from.ss_family, SOCK_DGRAM, 0);
      if (peer < 0)
        {
          syslog (LOG_ERR, "socket: %m\n");
          exit (EXIT_FAILURE);
        }
      memset (&sin, 0, sizeof (sin));
   -  sin.sin_family = AF_INET;
   -  if (bind (peer, (struct sockaddr *) &sin, sizeof (sin)) < 0)
   +  sin.ss_family = from.ss_family;
   +  if (bind (peer, (struct sockaddr *) &sin, fromlen) < 0)
        {
          syslog (LOG_ERR, "bind: %m\n");
          exit (EXIT_FAILURE);
        }
   -  if (connect (peer, (struct sockaddr *) &from, sizeof (from)) < 0)
   +  if (connect (peer, (struct sockaddr *) &from, fromlen) < 0)
        {
          syslog (LOG_ERR, "connect: %m\n");
          exit (EXIT_FAILURE);
   @@ -360,8 +365,10 @@ again:
      ecode = (*pf->f_validate) (&filename, tp->th_opcode);
      if (logging)
        {
   -      syslog (LOG_INFO, "%s: %s request for %s: %s",
   -          verifyhost (&from),
   +      syslog (LOG_INFO, "%s (%s): %s request for %s: %s",
   +          verifyhost (&from, fromlen),
   +          from.ss_family == AF_INET ? "IPv4"
   +            : (from.ss_family == AF_INET6 ? "IPv6" : "?"),
                 tp->th_opcode == WRQ ? "write" : "read",
                 filename, errtomsg (ecode));

Nicer to do:

switch (from.ss_family)
  {
  case AF_INET: family = "IPv4"; break;
  case AF_INET6: family = "IPv6"; break;
  default: family = "?"; break;
  }
syslog (LOG_INFO, ...", ..., family, ...);

        }
   @@ -737,16 +744,20 @@ nak (int error)
    }

    static const char *
   -verifyhost (struct sockaddr_in *fromp)
   +verifyhost (struct sockaddr_storage *fromp, socklen_t frlen)
    {
   -  struct hostent *hp;
   +  int rc;
   +  static char host[NI_MAXHOST];

Does the Hurd have NI_MAXHOST? I don't know.

Cheers.