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

[Arjen de Korte]

> 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.

> 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).

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.

> 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.

Best regards, Arjen