Hi!
On Tue, 2010-09-07 at 18:05:37 +0200, Ludovic Courtès wrote:
> The attached patch makes ‘tftpd’ IPv6-compatible. As a side effect, it
> makes it usable (!) on current GNU/Linux when a client connects via an
> address other than the loopback address (FROM is always AF_INET6 on my
> machine.)
>
> The patch assumes that IPv6 support is available. Besides, it’d be
> better to import Gnulib’s ‘inet_ntop’ modules.
Mats was already working on this, see also my review on that thread.
I'll comment on this one anyway, as I've seen some problems.
> From 3be7f44305103b39a0e1bc79a9960feb5d142485 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= <l...@gnu.org>
> Date: Tue, 7 Sep 2010 17:53:23 +0200
> Subject: [PATCH] Make `tftpd' IPv6-compatible.
> diff --git a/src/tftpd.c b/src/tftpd.c
> index 70602fa..719db81 100644
> --- a/src/tftpd.c
> +++ b/src/tftpd.c
> @@ -124,7 +124,8 @@ static int logging;
>
> static const char *errtomsg (int);
> static void nak (int);
> -static const char *verifyhost (struct sockaddr_in *);
> +static char *verify_host (struct sockaddr_storage *, socklen_t,
> + char *, size_t);
Why remove the constness? the returned string is not supposed to be
modified nor freed.
> @@ -268,18 +269,19 @@ main (int argc, char *argv[])
> memset (&sin, 0, sizeof (sin));
> - sin.sin_family = AF_INET;
> + sin.ss_family = from.ss_family;
> if (bind (peer, (struct sockaddr *) &sin, sizeof (sin)) < 0)
The socklen_t argument should be fromlen so that this works on the
BSDs.
> @@ -360,8 +362,10 @@ again:
> ecode = (*pf->f_validate) (&filename, tp->th_opcode);
> if (logging)
> {
> + char node[1024];
This should be NI_MAXHOST instead.
> syslog (LOG_INFO, "%s: %s request for %s: %s",
> - verifyhost (&from),
> + verify_host (&from, sizeof from,
Here fromlen too.
> + node, sizeof node),
> tp->th_opcode == WRQ ? "write" : "read",
> filename, errtomsg (ecode));
> }
> @@ -736,17 +740,29 @@ nak (int error)
> syslog (LOG_ERR, "nak: %m\n");
> }
>
> -static const char *
> -verifyhost (struct sockaddr_in *fromp)
> +static char *
> +verify_host (struct sockaddr_storage *addr, socklen_t addr_len,
> + char *host_and_service, size_t len)
> {
> - struct hostent *hp;
> -
> - hp = gethostbyaddr ((char *) &fromp->sin_addr, sizeof (fromp->sin_addr),
> - fromp->sin_family);
> - if (hp)
> - return hp->h_name;
> + int err;
> + char host[512], service[256];
Here NI_MAXHOST and NI_MAXSERV instead of magic values.
> +
> + err = getnameinfo ((struct sockaddr *) addr, addr_len,
> + host, sizeof host,
> + service, sizeof service,
> + 0);
> +
> + if (err)
> + inet_ntop (addr->ss_family,
> + addr->ss_family == AF_INET
> + ? (void *) &((struct sockaddr_in *) addr)->sin_addr
> + : (void *) &((struct sockaddr_in6 *) addr)->sin6_addr,
> + host_and_service, len);
The inet_ntop call should not be needed, getnameinfo should be able to
handle numeric addresses just fine.
> else
> - return inet_ntoa (fromp->sin_addr);
> + snprintf (host_and_service, len, "%s%s%s",
> + host, *service ? "/" : "", service);
> +
> + return host_and_service;
> }
regards,
guillem
