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
<mats.anders...@gisladisker.se>
2459 41E9 C420 3F6D F68B 2E88 F768 4541 F25B 5D41
Abonnerar på: debian-mentors, debian-devel-games, debian-perl,
debian-ipv6, debian-qa
