onsdag den 8 september 2010 klockan 06:09 skrev Guillem Jover detta:
> 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.
>
Interesting. I must check the code with this viewpoint in mind.
Clearly, the grand picture escaped me.
> > > > +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.
Point taken! Now I better follow your original motivation for claiming
the new library functions to be superfluous. I will certainly reread
the code with the purpose of retaining as much socket information as
is safely possible.
> > > > 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.
Would not recv(2) instead of recvfrom(2) involve a minute risk for spoofing?
That was my original reason for leaving all recvfrom() essentially untouched.
> > > > 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.
Admittedly, my patch did contain the intentional departure from the previous
code in one aspect here: where the old code did a full hostname resolution
for logging/reporting, I found it to be wasting sufficiently much time, that
I restricted the lockup to give a numerical result only. My plan was to discuss
this matter later, but it slipped my mind.
> > > > 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.
>
This is also my impression. I was simply cautious about its removal.
Your comments are truly appreciated, once I now understand where our
oppinions lead different observations.
Best regards,
Mats E A
