[Nut-upsdev] Default NUT PORT (errrr...)

[Peter Selinger]

Arjen de Korte wrote:
> 
> 
> > I am not convinced this is the best solution. You said you wanted to
> > avoid runtime conversions, but for the 99% of users that use IPv4, you
> > just introduced such a conversion in upsd.c, line 204. The string
> > constant manufactured by the preprocessor is converted back to an
> > integer there.
> 
> I think for the vast majority of users, the HAVE_IPV6 flag will be set and
> this code will not be used at all anymore, so this is probably not much of
> an issue.
> 
> > The use of getaddrinfo() is not in itself a good reason to make the
> > port a string type. That function is probably not portable anyway.
> 
> I doubt the latter. It is defined by POSIX, there are several RFC's for it
> (RFC 2553, RFC 3493) and it seems to be in widespread use. In contrast,
> the functions gethostbyaddr() and gethostbyname() (the ones we used so
> far) have been marked obsolescent in POSIX 1003.1-2001 already. We should
> probably be conservative in what flags we pass to it however, since there
> have been some additions lately (AI_ADDRCONFIG, AI_ALL, and AI_V4MAPPED
> are available since glibc 2.3.3, AI_NUMERICSERV is available since glibc
> 2.3.4), that probably not have made it to every platform. When we restrict
> ourselves to AI_PASSIVE, we should be fine here.

Ah, right. I didn't know it was in POSIX. 
 
> > Perhaps one good reason for making the port a string is to allow
> > symbolic names, such as "http" instead of "80". I don't know how
> > useful this will be in NUT - would anybody want to run a NUT server on
> > a port reserved for some other protocol?
> 
> I think it is a good idea because it matches with the way the data is read
> from upsd.conf (as a string) and later on used by getaddrinfo(). Unlike
> what you noted, I think the future is in getaddrinfo() and that we should
> aim to cater for that one. With both the listening address and -port as
> strings, that would mean zero conversions between parsing of the
> configuration file and the use in setuptcp(). For systems not supporting
> (or with broken getaddrinfo() for instance), we must provide conversion
> functions. I don't care much about the symbolic names, that's nice to
> have, but not really important. If someone wants to run NUT on a different
> port, the're probably not going to confuse themselves even more by using a
> symbolic name (I wouldn't recommend that anyway).
> 
> > In this case, --with-port should also take a string, and therefore
> > PORT should be #defined as a string. I don't think the interface in
> > upsclient.c needs to be changed at all; all that's needed is for
> > upscli_splitname() to call getservbyname(3) to generate a numeric
> > value.
> 
> The only (minor) drawback here, is that we have a conversion from string
> to int in all cases (whether or not we have HAVE_IPV6 defined). Referring
> to the above, I do think it is cleaner to change in the API the port from
> an int to a string value in upsclient.c in upscli_connect(). The interface
> would be more straightforward that way since everything is passed as a
> string (which is probably closer to what a client program reads as command
> line arguments).

Sure, I agree. Or rather, I agree that it doesn't matter much, once
you don't want the symbolic port names. 
 
> I do think however, that the upscli_splitname() doesn't really belong in
> the API. How a client addresses parsing a upsname / hostname / portnumber
> triplet is up to the client, not of the API. Also, setting defaults in
> absence of some required values (hostname, portnumber), is something a
> client should handle, not the API.

I like the idea that NUT defines some standard string notation for a
ups-address, and that this is made available to clients. Some
specialized clients may want to handle their own parsing, but in
general it is less confusing if all clients use the same notation, not
to mention it's easier to extend the notation in the future. 
 
> > How about defining a function to convert a string to a port number, as
> > follows:
> >
> > #include <netdb.h>
> > #include <stdlib.h>
> > #include <netinet/in.h>
> >
> > int strtoport(char *s) {
> >    int port;
> >    char *endptr;
> >    struct servent *service;
> >
> >    port = strtol(s, &endptr, 10);
> >    if (endptr != s && *endptr == '\0') {
> >       return port;
> >    }
> >    service = getservbyname(s, NULL);  /* specify proto = tcp or not? */
> >    if (service) {
> >       return ntohs(service->s_port);
> >    }
> >    return 0;  /* invalid port */
> > }
> >
> > This returns for example
> > strtoport("1234") == 1234
> > strtoport("http") == 80
> >
> > Coupling this with the following patch I think is all that is needed
> > to enable symbolic port names throughout.  The only remaining problem
> > is where to put the function strtoport; it would have to be somewhere
> > both upsd and upsclient can use it.
> >
> > What do you think? -- Peter
> 
> I think this is an awful lot of code for a function that is essentially
> not more than atoi(PORT). I was not aiming for support of symbolic names,
> but rather for a more straightforward way of dealing with the PORT value.
> Other than that, I have no objections to it.

Yup, if we don't want the symbolic names, there is not point to
this. 

-- Peter