clients: update clients to timeout in 10 secs, try SSL

[sshambar]

Many clients currently have no network timeout; connections can take several minutes to fail (or longer if multiple addresses are resolved).

Patch adds a 10 second timeout to all client connections. upsmon especially benefits from the patch as one unavailable ups could delay processing of other ups's, and also cause the watchdog to fire.

Many ups clients were also not attempting SSL, even ones that use authentication to perform actions. Patch adds flag to attempt (but not require) it.

[jimklimov]

Thanks. I presume you saw comments to PR #85 which tackled this matter before? At least, yours does use upscli_tryconnect() :)
I am not in favor of a hard-coded timeout like defined here. The define can be a default, but users should be able to override it (e.g. with a common CLI option and/or envvar, at least for clients without a config file?)

Also, this deserves entries in NEWS.adoc as well as UPGRADING.adoc (as a possible behavior change and cause for troubleshooting after NUT upgrades on a deployment).

[sshambar]

Thanks. I presume you saw comments to PR #85 which tackled this matter before?

No, I hadn't searched back that far :)... I just had a machine go offline and all my upsmon's starting failing the watchdog and restarting (which failed their heartbeats)... (and my checkmk agents started stalling on upsc) -- so I decided some kind of reasonable timeout would at least fix the "connect" issue.

At least, yours does use upscli_tryconnect() :) I am not in favor of a hard-coded timeout like defined here. The define can be a default, but users should be able to override it (e.g. with a common CLI option and/or envvar, at least for clients without a config file?).

Well, several of the clients use '-t seconds' for a command completion timeout... do you want me to apply that timeout to the connect part too?

Environment config would be possible, but that gets into a much more involved "one per client vs unified client default vs nutclient.so library default vs ???"

I didn't add "alarms" to the network transport part of the program (like upsmon has), but I guess that's possible too...

Also, this deserves entries in NEWS.adoc as well as UPGRADING.adoc (as a possible behavior change and cause for troubleshooting after NUT upgrades on a deployment).

OK, I'll put something in them once we've got a consensus on what we're doing :)

[jimklimov]

I think 't' for --timeout would be good. Indeed, it is used as such for nut-scanner, for similar cause in upsrw and upscmd. Just for "testing mode" in upsdrvctl - but drivers can have their own addvar("timeout"...) or similar, and at least some of the networked ones do (netxml-ups, snmp-ups...)

Alternately, maybe better, 'T' could be used almost everywhere (so far seems only used for thread-count constraint in nut-scanner which has t as timeout) to avoid overlap with the timeout of waiting for commands to respond (which logically differs from the timeout of server connection, might as well be by orders of magnitude).

As for defaulting, I suppose there can be a new method in upsclient.{c,h} to get_default_timeout(char*) into which you would feed the value seen in CLI (if any, else the variable for the argument is NULL), and which juggles the built-in default, or envvar, or explicit request, consistently and with a single implementation. Then the programs just assign (or not) the variable for argv[i] corresponding to a -t they see (or not), and after the CLI option loop is done - call this method and get an int (or a struct timeval right away?)

The same method can be called to handle config file options, if any are provided (so maybe calling it twice, or with two args either or both of which can be null). Two args would help establish priorities better (built-in, config file, envvar, CLI option).

[sshambar]

Alternately, maybe better, 'T' could be used almost everywhere (so far seems only used for thread-count

'W' appears available for all the clients; matching the ping -W option, it could support a float that can set fractional seconds.

As for defaulting, I suppose there can be a new method in upsclient.{c,h} to get_default_timeout(char*) into which you would feed the value seen in CLI (if any, else the variable for the argument is NULL), and which juggles the built-in default, or envvar, or explicit request, consistently and with a single implementation. Then the programs just assign (or not) the variable for argv[i] corresponding to a -t they see (or not), and after the CLI option loop is done - call this method and get an int (or a struct timeval right away?)

So you're thinking something like:

  void get_default_timeout(const char *opt_val, const char *config_val, struct timeval *tm);

which can be called multiple times, and if opt_val/config_val are non-NULL they "update" the internal timeout, and later calls to get_default_timeout(NULL, NULL, &tv) will return the latest value?

[sshambar]

Actually, would it not be simpler to just have upscli_connect() just honor the default timeout? Then it's as easy as:

upsclient.h:
int upscli_set_default_timeout(const char *str); // <str> may be NULL, returns -1 on parse/value error
int upscli_get_default_timeout(struct timeval *tv); // -1 if no default set

<prog>.c:
#define UPSCLI_DEFAULT_TIMEOUT "10"
upscli_set_default_timeout(argval ? argval : UPSCLI_DEFAULT_TIMEOUT);
upscli_connect(...) // as before

The timeout is unset at startup, so current library users don't see a change.
Calls to upscli_set_default_timeout() set/update the timeout (NULL reverts to no default).

All nut client tools could have the new option, always call upscli_set_default_timeout() and get network timeouts with no further updates. The timeout could also apply to all network traffic (not just connect) and make clients easier to maintain. Most clients would never even call upscli_get_default_timeout()...

UPSCLI_DEFAULT_TIMEOUT could be in a common nut tool header somewhere, but isn't really in the library (which has no default)

Option ==> UPSCLI_DEFAULT_TIMEOUT is the new global default, but can be turned off with upscli_set_default_timeout(NULL)

[sshambar]

As a trial, I added the api above and updated upsc (only) to use it with option -W (option -W 0 sets no timeout)

[jimklimov]

So you're thinking something like:

void get_default_timeout(const char *opt_val, const char *config_val, struct timeval *tm);

which can be called multiple times, and if opt_val/config_val are non-NULL they "update" the internal timeout, and later calls to get_default_timeout(NULL, NULL, &tv) will return the latest value?

Well, my idea here was that this method selects the best default timeval from given sources that may be present or absent, and the program then keeps the variable and uses it for upscli_tryconn. Comparing to your later comments, potentially this allows several such variables, rather than one hidden in the library as an initially-set-never-changed value. That could be useful e.g. in upsmon, maybe upslog or other clients that deal with several UPSes simultaneously...

The idea with (argval ? argval : UPSCLI_DEFAULT_TIMEOUT) seems neat. Perhaps this can be used with an UPSCLI_DEFAULT_TIMEOUT defined in the actual library (upsclient.h) and if some tools think they want another default - they can pass their own macro this way.

It may be even easier on processing (logic at least) somewhat, the method picks the best preferred non-NULL string and parses it into a number. MAYBE if the parsing fails, it takes the next best-liked string pointer and so on. Although then the argval being the last tried option with an invalid content would need a hard-coded number still (e.g. 0 for indefinite timeout like now?)

upscli_connect(...) // as before

This method, per #85 discussion, should connect until the system reports a failure (e.g. TCP/IP stack timeout). If we have a soft timeout to consider, consumer code should (be changed to) use upscli_tryconnect().

[sshambar]

Well, my idea here was that this method selects the best default timeval from given sources that may be present or absent, and the program then keeps the variable and uses it for upscli_tryconn. Comparing to your later comments, potentially this allows several such variables, rather than one hidden in the library as an initially-set-never-changed value.

So I guess I'm unsure what the xxx_default_timeout() function does then... if the client passes in the "sources" (command option, default, config file in future?), is the function just a wrapper for strtoul() -> timeval.tv_sec? -- and clients store it locally and pass it to upscli_tryconnect()?... also, would clients then handle their own "post-connect" timeouts with alarm() or similar (as upsmon currently does)?

I guess I thought it would be nice to have a function that changed the timeouts for all network functions so that each client didn't have to "re-invent the wheel" with alarms or such (and not calling the function just keeps things the way they are now...)

That could be useful e.g. in upsmon, maybe upslog or other clients that deal with several UPSes simultaneously...

So you think upsmon/upslog might need different network timeouts for different UPSs? That's flexible, but i wonder how many people would need different timeouts for each UPS... of course, just calling the xxx_default_timeout() before handling each UPS's network traffic could support that -- perhaps a more clearly named upscli_set_net_timeout()?

The idea with (argval ? argval : UPSCLI_DEFAULT_TIMEOUT) seems neat. Perhaps this can be used with an UPSCLI_DEFAULT_TIMEOUT defined in the actual library (upsclient.h) and if some tools think they want another default - they can pass their own macro this way.

Yeah, I couldn't quite figure out a good name though, since it's only the "DEFAULT" if you actually call the function :D

It may be even easier on processing (logic at least) somewhat, the method picks the best preferred non-NULL string and parses it into a number. MAYBE if the parsing fails, it takes the next best-liked string pointer and so on. Although then the argval being the last tried option with an invalid content would need a hard-coded number still (e.g. 0 for indefinite timeout like now?)

Wouldn't it be useful to have a parse error (on a parameter value for example) actually fail the command with a useful error message rather than a warning and ignoring it?

upscli_connect(...) // as before

This method, per #85 discussion, should connect until the system reports a failure (e.g. TCP/IP stack timeout). If we have a soft timeout to consider, consumer code should (be changed to) use upscli_tryconnect().

But that would still require clients to (a) pass the timeout value about, (b) apply it to alarms() etc to interrupt other network functions... upscli_tryconnect() is useful for explicit timeouts, but a nice function to set the "default" timeout would make the interface easier to use...

[sshambar]

BTW, it's worth noting that the upscli_readline(...) just calls upscli_readline_timeout(...,DEFAULT_NETWORK_TIMEOUT) (defined in common.h, and is 5).

Oddly, upscli_sendline(...) calls upscli_sendline_timeout(...,0)

[jimklimov]

Regarding naming changes (hey, ain't known as one of the hardest problems in IT for nothing) - hopefully my work for this pass is done... if CI agrees.

[jimklimov]

Hm, that IP address printing misfired, will look at it later... or you can :)

[AppVeyorBot]

❌ Build nut 2.8.2.2930-master failed (commit 454fc01cf8 by @jimklimov)

[jimklimov]

Reports from NetBSD worker would be fixed in #2870
Windows just exceeded the hour...