INSTCMD and SET VAR status tracking implementation #659

aquette · 2019-02-04T13:43:27Z

This implements the instant commands (instcmd) and variables settings (setvar) status tracking, to get the actual execution status from the driver. Support in upscmd and upsrw is also included.

Other pull requests will address the remaining bits of implementation, such as libnutclient, and bindings

Ref: #656

Signed-off-by: Arnaud Quette [email protected]

This commit implements the instant commands (instcmd) and variables settings (setvar) status tracking, to get the actual execution status from the driver Signed-off-by: Arnaud Quette <[email protected]>

Signed-off-by: Arnaud Quette <[email protected]>

Following Daniele Pezzini thorough review, sanitize and improve the code, and also complete documentation Signed-off-by: Arnaud Quette <[email protected]>

implement support for status tracking in upscmd and upsrw Signed-off-by: Arnaud Quette <[email protected]>

aquette · 2019-02-04T16:34:08Z

Note on the [DO NOT MERGE YET] / WIP:
I've forgotten that I really want to finalize decision on the UUID implementation...

Signed-off-by: Arnaud Quette <[email protected]>

There is no need to condition CMDSET_STATUS commands received by upsd from the driver, on cmdset_status_enabled, since what matters is the presence of a list and id Signed-off-by: Arnaud Quette <[email protected]>

Signed-off-by: Arnaud Quette <[email protected]>

replaced the initial rxi/uuid4 library used by a cross platform version. Though more basic, it is enough for our needs, at least for now Signed-off-by: Arnaud Quette <[email protected]>

zykh

thanks for addressing those issues, @aquette... here a few more, for your pleasure

clients/upscmd.c

docs/man/upscmd.txt

drivers/dstate.c

server/upsd.c

server/upsd.h

Signed-off-by: Arnaud Quette <[email protected]>

Add upscli_sendline_timeout and upscli_readline_timeout, beside from the classic blocking versions. Also make a common define for timeout, and use it in upsclient and nut-scanner Signed-off-by: Arnaud Quette <[email protected]>

Signed-off-by: Arnaud Quette <[email protected]>

clepple

I should have added this first comment to the review as well: https://github.com/networkupstools/nut/pull/659/files/87f59e7c009efe0fc2da9fba33d11ab31a61ffa8..a619455687f98d3dc2b2c83628ca4fa14cf39076#r255510222

Somewhat cosmetic changes, but I figure since these files are being changed...

clients/upscmd.c

clients/upsrw.c

Rename the default timeout used in network operations by upsclient and nut-scanner to be more specific: from DEFAULT_TIMEOUT to DEFAULT_NETWORK_TIMEOUT. Plus, make the default timeout used when retrieving the result of an INSTCMD/SET VAR with TRACKING enabled a common #define'd value (DEFAULT_TRACKING_TIMEOUT), and use it also to publish the default value of the -t option in the help messages of upscmd and upsrw. As suggested by Charles Lepple.

Also, remove some nesting in tracking_del().

To keep things simple, at least for now, only generate manpages and not html pages (and, as such, don't even think of using our linkman AsciiDoc macro with those functions, to avoid dead links).

- addition of upscli_{read,send}line_timeout(), - upscli_cleanup() -> upcli_cleanup(void) Note: only increase 'current' and not 'age', because the upscli_cleanup() change could (potentially) make it not compatible with previous versions.

zykh · 2019-02-12T02:32:34Z

Thanks @clepple, @aquette.

I've merged the tracking branch into the cmdset_status one and addressed those remaining issues (hopefully in a reasonable way... let me know if this is not the case).

clepple

@zykh thanks for implementing the defaults commit.
@aquette looks good overall.

clepple · 2019-02-12T02:50:38Z

The only confusing thing is that now upscmd against dummy-ups in dummy/file mode fails when request tracking is on - the driver publishes a list of commands from the file, but does not allow them to "execute". Testing with upsrw worked better against dummy-ups, where I could suspend the driver to simulate high load.

zykh · 2019-02-13T04:39:51Z

hmm... right, dummy-ups's instcmd() always returns STAT_INSTCMD_UNKNOWN for the lone supported command (load.off): should we make it succeed? (actually implementing configurable instant commands seems worth a new issue)
We should also fix the (up until now ignored) return codes of the various drivers that were not fully ported to the new (in 2007) return codes for setvar() and instcmd() (e.g. setvar() of usbhid-ups and ... *cough* *cough* ... nutdrv_qx, which inherited it from usbhid-ups)...
Ah.. perhaps we should also warn that not only the server, but also the drivers need to support the TRACKING command, for the -w option of upscmd / upsrw to work as expected.

I'll do that, later today.

aquette · 2019-02-13T07:27:44Z

hi fellows,
agreed for the need to complete drivers, that's a due thing.
as for dummy-ups and instcmd/setvar, I've a plan to address that cleanly too (not yet actually thought about the how), but it's indeed another ticket.

aquette · 2019-02-13T07:30:06Z

FYI, travis failed on git fetch. relaunched to get the green...

…r -w

zykh · 2019-02-14T00:33:40Z

I was all set to update the various drivers, but stumbled upon this...

Drivers that already use all the STAT_* return codes have slightly different (and contradictory) behaviours, also in conflict with the documentation of STAT_* values in upshandler.h.

So, first, let's find a common field.

setvar

event	STAT_SET value returned
unknown var	STAT_SET_INVALID
var is not writable	STAT_SET_INVALID
invalid value for var	STAT_SET_INVALID
tried to set the var, but failed	STAT_SET_FAILED
other unspecified errors	STAT_SET_UNKNOWN
successfully set the var	STAT_SET_HANDLED

instcmd

event	STAT_INSTCMD value returned
unknown/invalid command	STAT_INSTCMD_INVALID
invalid optional param for command	STAT_INSTCMD_INVALID
tried the command, but it failed	STAT_SET_FAILED
other unspecified errors	STAT_INSTCMD_UNKNOWN
successfully set the var	STAT_INSTCMD_HANDLED

Does this seem reasonable to you?

aquette · 2019-02-14T07:18:57Z

For instcmd and my last patch on eaton marlin and hpe mib, I assumed that STAT_INSTCMD_INVALID is "invalid optional or missing mandatory param for command". As an example, load.off jas a default value but can be provided one. Load.off.delay has a mandatory param.
As a general note, I was also thinking about extending the return codes list, but that needed an audit of all drivers. So you @zykh have better visibility than me

jimklimov

Generally ok, some nits and sanity-check challenges commented...

jimklimov · 2019-02-22T09:00:25Z

clients/Makefile.am

 # libupsclient version information
 # http://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html
-libupsclient_la_LDFLAGS = -version-info 4:0:0
+libupsclient_la_LDFLAGS = -version-info 5:0:0


Note to self (and others): this impacts library filename and packaging/dependencies.

docs/man/upscli_readline.txt

docs/man/upscli_sendline.txt

docs/nut-names.txt

docs/nut.dict

jimklimov · 2019-02-22T09:37:29Z

clients/upscmd.c

+	if (
+		!tracking_enabled ||
+		/* sanity check on the size: "OK TRACKING " + UUID4_LEN */
+		strlen(buf) != (UUID4_LEN - 1 + strlen("OK TRACKING "))


From my reading of standards and docs in UUID-related codebases, at least two correct spellings of an UUID value are possible: the hexadecimal string broken by dashes in correct positions, or just the all-hex string (shorter).

Our updated header defines UUID4_LEN as "(Minimum) Size that a string must have to hold a UUID4", catering for the dashed spelling.

What if the user passes a valid non-dashed version (so strlen(buf) would be shorter than expected here)?
Should we convert one to another (if all hexes and length==X, then add dashes)?

At the very least, I think we should avoid surprises by documenting that the current code requires the particular spelling of UUID string.

Not sure it's worth: the intent is not to have real UUID v4, but more something (a key) unique enough to avoid collisions. Whatever the user passes has to be valid, and was supposed to have been provided before. If he passes random string, or non dashed ID or whatever, this will be truncated and discarded.

server/conf.c

jimklimov · 2019-02-23T16:14:23Z

@zykh @aquette (@clepple ?) : I see you've had quite a discussion above about updating other drivers to this new paradigm, fixing dummy-ups and error code handling... is this planned as part of this PR (or, does lack of those fixes block merging this codebase "as is" into the master?)

If yes, maybe we should mark this PR as a DNMY (Do not merge yet) for the time being, to avoid an eager mis-click? ;)

jimklimov · 2019-02-23T18:03:20Z

Resolved merge conflicts vs. master, that arose as other small PRs got merged.

aquette · 2019-02-25T07:38:41Z

hi all
back from vacations today, I was expecting that this PR would have been merged :D
I'll check and address your comments @jimklimov, thx.
@zykh any news on the drivers update front?
@jimklimov the followups are planned for separate PRs (including the libnutclient update related to the present PR)

Signed-off-by: Arnaud Quette <[email protected]>

aquette · 2019-02-26T09:19:40Z

all, I'm now going to merge this PR, to unblock followups (esp. the libnutclient implementation of this from @boricj )
@zykh : if you feel that are remainders for the drivers, let's track it into a separate ticket.
thanks to all for your comments and help on this topic!

* INSTCMD and SET VAR status tracking implementation This commit implements the instant commands (instcmd) and variables settings (setvar) status tracking, to get the actual execution status from the driver Signed-off-by: Arnaud Quette <[email protected]> * Missing Revision history Signed-off-by: Arnaud Quette <[email protected]> * Fix nut-names.txt modification that should not be here Signed-off-by: Arnaud Quette <[email protected]> * Augeas support: add CMDSETSTATUSDELAY for upsd.conf Signed-off-by: Arnaud Quette <[email protected]> * INSTCMD and SET VAR status tracking completion Following Daniele Pezzini thorough review, sanitize and improve the code, and also complete documentation Signed-off-by: Arnaud Quette <[email protected]> * INSTCMD and SET VAR status tracking completion implement support for status tracking in upscmd and upsrw Signed-off-by: Arnaud Quette <[email protected]> * upscmd/upsrw: delay retries for status tracking Signed-off-by: Arnaud Quette <[email protected]> * Remove comment There is no need to condition CMDSET_STATUS commands received by upsd from the driver, on cmdset_status_enabled, since what matters is the presence of a list and id Signed-off-by: Arnaud Quette <[email protected]> * Complete comment Signed-off-by: Arnaud Quette <[email protected]> * UUID v4 implementation replaced the initial rxi/uuid4 library used by a cross platform version. Though more basic, it is enough for our needs, at least for now Signed-off-by: Arnaud Quette <[email protected]> * status_info should be static Signed-off-by: Arnaud Quette <[email protected]> * No need for else, since fatalx is called before Signed-off-by: Arnaud Quette <[email protected]> * Suppress \n from debug output Signed-off-by: Arnaud Quette <[email protected]> * Group sanity checks Signed-off-by: Arnaud Quette <[email protected]> * Get rid of dynamic memory allocation Signed-off-by: Arnaud Quette <[email protected]> * Improve and enforce the use of UUID4_LEN Signed-off-by: Arnaud Quette <[email protected]> * Comment on the size of dest for nut_uuid_v4() Signed-off-by: Arnaud Quette <[email protected]> * Move structures and defines to more appropriate places Signed-off-by: Arnaud Quette <[email protected]> * Add functions with timeout Add upscli_sendline_timeout and upscli_readline_timeout, beside from the classic blocking versions. Also make a common define for timeout, and use it in upsclient and nut-scanner Signed-off-by: Arnaud Quette <[email protected]> * upscmd/upsrw: add a timeout option Signed-off-by: Arnaud Quette <[email protected]> * Basic homebrew UUID v4 implementation Signed-off-by: Arnaud Quette <[email protected]> * Prefer to use static buffer for UUID Signed-off-by: Arnaud Quette <[email protected]> * log actual result of instcmd / setvar Signed-off-by: Arnaud Quette <[email protected]> * Fix tracking ID reporting due to static memory changes Signed-off-by: Arnaud Quette <[email protected]> * upsclient: use unsigned int for timeouts Also, explicit that upscli_cleanup() takes no argument. * upscmd/upsrw: use unsigned int for timeout + our str_to_uint() for it Also, slightly reword the help message for `-t` (timeout) option, in order to clarify which is the unit (seconds) used for the provided value. * upscmd/upsrw: don't sleep after receiving a non-PENDING CMDSET_STATUS Also, remove some nesting in do_cmd() and do_set(). * net-protocol: clarify the format of GET CMDSET_STATUS + <status_id> <status_id> is not optional to get the status of a command/setvar with CMDSET_STATUS, so drop the [square brackets] from it. Also drop "quotes" in SET CMDSET_STATUS's <value>, since it's expected to be a single word. Plus, fix markup of INSTMCD's <cmdparam> parameter. * sock-protocol: align case and markup of command parameters * dstate: fix handling of INSTCMD's optional parameters We should still support the old `INSTCMD <cmdname> [<cmdparam>]` format, and not only consider <cmdparam> when also `STATUS_ID <id>` is provided. Also, fix the format of our sock-protocol commands mentioned in comments, and add function names to debug info. * common: document the recently added things * upsd: in INSTCMD/SET handlers, also accept NULL for status_id Plus: - use a simpler approach to test if status_id is not empty, - align the way the SET command is built to the one used for the INSTCMD command. * upsd: drop unnecessary/unused global * upsd: move sanity checks of cmdset_status_get() after declaration of vars Also, drop some nesting in that function. * Move from CMDSET_STATUS / STATUS_ID to TRACKING This is just a big, big rename, no code changes. * upsd: refine the tracking API Add a couple of functions to change in a predictable way the value of the general enablement of tracking and make it visible only inside upsd.c. Also, move the tracking type (tracking_t) and the list of items inside upsd.c. * net-protocol: also return TRACKING between OK and <id>, for INSTCMD/SET VAR * dstate: *really* fix handling of INSTCMD's optional parameters Erroring out on `INSTCMD <cmdname>` doesn't seem like a good idea... * common: massage default timeouts Rename the default timeout used in network operations by upsclient and nut-scanner to be more specific: from DEFAULT_TIMEOUT to DEFAULT_NETWORK_TIMEOUT. Plus, make the default timeout used when retrieving the result of an INSTCMD/SET VAR with TRACKING enabled a common #define'd value (DEFAULT_TRACKING_TIMEOUT), and use it also to publish the default value of the -t option in the help messages of upscmd and upsrw. As suggested by Charles Lepple. * upsd: ignore case of UUID4 in tracking API Also, remove some nesting in tracking_del(). * libupsclient: generate manpages for upscli_{read,send}line_timeout() To keep things simple, at least for now, only generate manpages and not html pages (and, as such, don't even think of using our linkman AsciiDoc macro with those functions, to avoid dead links). * libupsclient: bump version as per recent changes - addition of upscli_{read,send}line_timeout(), - upscli_cleanup() -> upcli_cleanup(void) Note: only increase 'current' and not 'age', because the upscli_cleanup() change could (potentially) make it not compatible with previous versions. * upscmd/upsrw: warn that also the drivers need to support TRACKING, for -w * Fix typo and spelling Signed-off-by: Arnaud Quette <[email protected]> * Add check around atoi() conversion Signed-off-by: Arnaud Quette <[email protected]>

aquette added 5 commits February 4, 2019 09:02

INSTCMD and SET VAR status tracking implementation

72241ad

This commit implements the instant commands (instcmd) and variables settings (setvar) status tracking, to get the actual execution status from the driver Signed-off-by: Arnaud Quette <[email protected]>

Missing Revision history

b7c0a9a

Signed-off-by: Arnaud Quette <[email protected]>

Fix nut-names.txt modification that should not be here

97c51c9

Signed-off-by: Arnaud Quette <[email protected]>

Augeas support: add CMDSETSTATUSDELAY for upsd.conf

bf46107

Signed-off-by: Arnaud Quette <[email protected]>

INSTCMD and SET VAR status tracking completion

a348995

Following Daniele Pezzini thorough review, sanitize and improve the code, and also complete documentation Signed-off-by: Arnaud Quette <[email protected]>

aquette requested review from clepple, jimklimov and zykh February 4, 2019 13:43

INSTCMD and SET VAR status tracking completion

2a171dd

implement support for status tracking in upscmd and upsrw Signed-off-by: Arnaud Quette <[email protected]>

aquette added the WIP label Feb 4, 2019

aquette changed the title ~~INSTCMD and SET VAR status tracking implementation~~ [DO NOT MERGE YET] INSTCMD and SET VAR status tracking implementation Feb 4, 2019

aquette added 4 commits February 4, 2019 18:00

upscmd/upsrw: delay retries for status tracking

f802054

Signed-off-by: Arnaud Quette <[email protected]>

Remove comment

0f3a01b

There is no need to condition CMDSET_STATUS commands received by upsd from the driver, on cmdset_status_enabled, since what matters is the presence of a list and id Signed-off-by: Arnaud Quette <[email protected]>

Complete comment

f67ab8d

Signed-off-by: Arnaud Quette <[email protected]>

UUID v4 implementation

3104343

replaced the initial rxi/uuid4 library used by a cross platform version. Though more basic, it is enough for our needs, at least for now Signed-off-by: Arnaud Quette <[email protected]>

aquette removed the WIP label Feb 4, 2019

aquette changed the title ~~[DO NOT MERGE YET] INSTCMD and SET VAR status tracking implementation~~ INSTCMD and SET VAR status tracking implementation Feb 4, 2019

zykh requested changes Feb 6, 2019

View reviewed changes

aquette added 11 commits February 6, 2019 08:32

status_info should be static

f3ac852

Signed-off-by: Arnaud Quette <[email protected]>

No need for else, since fatalx is called before

fe6c478

Signed-off-by: Arnaud Quette <[email protected]>

Suppress \n from debug output

d30e0cb

Signed-off-by: Arnaud Quette <[email protected]>

Group sanity checks

6cdbe93

Signed-off-by: Arnaud Quette <[email protected]>

Get rid of dynamic memory allocation

823e968

Signed-off-by: Arnaud Quette <[email protected]>

Improve and enforce the use of UUID4_LEN

4d36b83

Signed-off-by: Arnaud Quette <[email protected]>

Comment on the size of dest for nut_uuid_v4()

aef9ea7

Signed-off-by: Arnaud Quette <[email protected]>

Move structures and defines to more appropriate places

25eca74

Signed-off-by: Arnaud Quette <[email protected]>

Add functions with timeout

2e9c27c

Add upscli_sendline_timeout and upscli_readline_timeout, beside from the classic blocking versions. Also make a common define for timeout, and use it in upsclient and nut-scanner Signed-off-by: Arnaud Quette <[email protected]>

upscmd/upsrw: add a timeout option

f91255d

Signed-off-by: Arnaud Quette <[email protected]>

Basic homebrew UUID v4 implementation

1473496

Signed-off-by: Arnaud Quette <[email protected]>

clepple reviewed Feb 11, 2019

View reviewed changes

clients/upscmd.c Outdated Show resolved Hide resolved

clients/upscmd.c Outdated Show resolved Hide resolved

clients/upsrw.c Outdated Show resolved Hide resolved

zykh added 4 commits February 11, 2019 23:23

upsd: ignore case of UUID4 in tracking API

787d584

Also, remove some nesting in tracking_del().

libupsclient: generate manpages for upscli_{read,send}line_timeout()

a31e5e4

To keep things simple, at least for now, only generate manpages and not html pages (and, as such, don't even think of using our linkman AsciiDoc macro with those functions, to avoid dead links).

libupsclient: bump version as per recent changes

663dd9f

- addition of upscli_{read,send}line_timeout(), - upscli_cleanup() -> upcli_cleanup(void) Note: only increase 'current' and not 'age', because the upscli_cleanup() change could (potentially) make it not compatible with previous versions.

clepple approved these changes Feb 12, 2019

View reviewed changes

upscmd/upsrw: warn that also the drivers need to support TRACKING, fo…

c0c478b

…r -w

jimklimov approved these changes Feb 23, 2019

View reviewed changes

Merge branch 'master' into cmdset_status

3948f09

aquette added 2 commits February 26, 2019 09:52

Fix typo and spelling

155e795

Signed-off-by: Arnaud Quette <[email protected]>

Add check around atoi() conversion

9a40084

Signed-off-by: Arnaud Quette <[email protected]>

aquette merged commit d5e2902 into master Feb 26, 2019

aquette mentioned this pull request Mar 1, 2019

Use timeout in connect function, #85

Closed

jimklimov deleted the cmdset_status branch March 29, 2022 12:13

jimklimov added the NUT protocols label Apr 1, 2022

jimklimov added this to the 2.8.0 milestone Mar 24, 2025

jimklimov mentioned this pull request Mar 24, 2025

Found some tech debt for INSTCMD handling from v2.8.0-v2.8.2 #2860

Merged

jimklimov mentioned this pull request Apr 14, 2025

docs/man/upscmd.txt, docs/man/upsrw.txt: clarify "OK" vs. waiting for result #2901

Merged

Uh oh!

INSTCMD and SET VAR status tracking implementation #659

INSTCMD and SET VAR status tracking implementation #659

Uh oh!

Conversation

aquette commented Feb 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aquette commented Feb 4, 2019

Uh oh!

zykh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

clepple left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zykh commented Feb 12, 2019

Uh oh!

clepple left a comment

Choose a reason for hiding this comment

Uh oh!

clepple commented Feb 12, 2019

Uh oh!

zykh commented Feb 13, 2019

Uh oh!

aquette commented Feb 13, 2019

Uh oh!

aquette commented Feb 13, 2019

Uh oh!

zykh commented Feb 14, 2019

Uh oh!

aquette commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimklimov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimklimov Feb 22, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jimklimov Feb 22, 2019

Choose a reason for hiding this comment

Uh oh!

aquette Feb 26, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jimklimov commented Feb 23, 2019

Uh oh!

jimklimov commented Feb 23, 2019

Uh oh!

aquette commented Feb 25, 2019

Uh oh!

aquette commented Feb 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

aquette commented Feb 4, 2019 •

edited

Loading

aquette commented Feb 14, 2019 •

edited

Loading

jimklimov left a comment •

edited

Loading