Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Propagate IMMUTABLE flag on socket protocol (driver=>upsd); use it in netset; sanity-check if (defaulted) NUMBER values are not in fact STRINGs #2549

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

jimklimov
Copy link
Member

@jimklimov jimklimov commented Jul 23, 2024

Addresses various sides of issue #266 and should help hunt down cases where we do not provide a NUMBER/STRING flag for the readings.

Currently this PR does not enforce the STRING:N type but logs messages to that effect to help with further troubleshooting. One problem was about picking the aux value (e.g. allowed string length) at a point far away from code that should have defined the variable type in the first place. Another concern is about propagating the guessed STRING type "upwards", so that netset can set it safely. Although if it lacks any type flag in the first place, most of those checks are (before and still) bypassed and any value should be settable.

Possibly the better place for this sanity-checking logic of newly posted data vs. flags of a state entry is in core state.c code, to transplant pieces of both netset.c::set_var() and netget.c::get_type() to restrict type usage that should conform to declared flags.

Another aspect is bringing IMMUTABLE flag presence into consideration when trying to set values over NUT protocol (it is not exposed in GET TYPE of the networked protocol to avoid changing the RFC).

Screenshot with a driver that has override.battery.charge=10 in ups.conf:

:; (echo 'GET TYPE dummy battery.charge'; sleep 1; echo 'GET TYPE dummy driver.version'; sleep 1) | ( telnet localhost 3493 ; sleep 1 )
Trying 127.0.0.1...
Connected to localhost.localdomain.
Escape character is '^]'.
TYPE dummy battery.charge NUMBER
TYPE dummy driver.version NUMBER
Connection closed by foreign host.

### Server side
Jul 23 02:19:08 pve nut-server[3327143]: 1448.475949        [D2] Connect from 127.0.0.1
Jul 23 02:19:08 pve nut-server[3327143]: 1448.475982        [D6] Entering check_command: GET
Jul 23 02:19:08 pve nut-server[3327143]: 1448.475984        [D6] check_command: Calling command handler for GET
Jul 23 02:19:08 pve nut-server[3327143]: 1448.475988        [D3] get_type: UPS[dummy] variable battery.charge is IMMUTABLE
Jul 23 02:19:08 pve nut-server[3327143]: 1448.475991        [D3] get_type: assuming UPS[dummy] variable battery.charge is a NUMBER
Jul 23 02:19:08 pve nut-server[3327143]: 1448.476013        [D2] write: [destfd=8] [len=33] [TYPE dummy battery.charge NUMBER]
Jul 23 02:19:09 pve nut-server[3327143]: 1449.475430        [D6] Entering check_command: GET
Jul 23 02:19:09 pve nut-server[3327143]: 1449.475441        [D6] check_command: Calling command handler for GET
Jul 23 02:19:09 pve nut-server[3327143]: 1449.475447        [D3] get_type: assuming UPS[dummy] variable driver.version is a NUMBER
Jul 23 02:19:09 pve nut-server[3327143]: 1449.475450        [D3] get_type: UPS[dummy] variable driver.version is a NUMBER but not (exclusively) a double: 2.8.2-729-g336cfcef0
Jul 23 02:19:09 pve nut-server[3327143]: 1449.475455        [D4] get_type: val=0.000000 len=20: Invalid argument
Jul 23 02:19:09 pve nut-server[3327143]: 1449.475458        [D3] get_type: UPS[dummy] variable driver.version is a NUMBER but not (exclusively) a long int: 2.8.2-729-g336cfcef0
Jul 23 02:19:09 pve nut-server[3327143]: 1449.475460        [D4] get_type: val=0 len=20: Invalid argument
Jul 23 02:19:09 pve nut-server[3327143]: 1449.475463        get_type: UPS[dummy] variable driver.version is flagged as a NUMBER but is not by contents (please report as a bug to NUT project)): 2.8.2-729-g336cfcef0
Jul 23 02:19:09 pve nut-server[3327143]: 1449.475484        [D2] write: [destfd=8] [len=33] [TYPE dummy driver.version NUMBER]
Jul 23 02:19:10 pve nut-server[3327143]: 1450.117526        [D6] Entering check_command: GET
Jul 23 02:19:10 pve nut-server[3327143]: 1450.117536        [D6] check_command: Calling command handler for GET
Jul 23 02:19:10 pve nut-server[3327143]: 1450.117555        [D2] write: [destfd=6] [len=27] [VAR eco650 ups.status "OL"]
Jul 23 02:19:10 pve nut-server[3327143]: 1450.476642        [D2] Disconnect 127.0.0.1 (no data available)
Jul 23 02:19:10 pve nut-server[3327143]: 1450.476657        [D2] Disconnect from 127.0.0.1

Currently planning this PR to hang around for reviews and discussion until the next release after an upcoming one; some less questionable changes may be cherry-picked earlier.

This sort of change, or something inspired by it to ensure proper data type mapping with flags (maybe a table based on docs/nut-names.txt to auto-flag device readings by pattern name? maybe from a loadable file, DMF or parseconf'ed?), would also be useful for efforts like #2525/#2524 which can culminate in reporting of JSON from NUT clients (web or not) -- and then best with a specific reportable schema.

@jimklimov jimklimov added enhancement NUT protocols documentation-protocol Submitted vendor-provided or user-discovered protocol information, or similar data (measurements...) C-str Issues and PRs about C/C++ methods, headers and data types dealing with strings and memory blocks labels Jul 23, 2024
@jimklimov jimklimov added this to the 2.8.4 milestone Jul 23, 2024
@jimklimov jimklimov marked this pull request as draft July 23, 2024 07:47
@jimklimov
Copy link
Member Author

Rebased over proposed PR #2552 which covers the noisy but mostly cosmetic changes, to group logical-change commits later in the stack.

@jimklimov jimklimov marked this pull request as ready for review July 23, 2024 07:53
…e as float or long int [networkupstools#266]

If we assume a value without flags is by default a NUMBER but is not,
re-default it to STRING. If it is also flagged explicitly as a NUMBER
but is not, yell a warning for upstream to fix the code. Eventually
this should be a reason to report protocol error, but not now.

Signed-off-by: Jim Klimov <[email protected]>
…MMUTABLE flag propagation on socket protocol (and fix float=>double)

Signed-off-by: Jim Klimov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-str Issues and PRs about C/C++ methods, headers and data types dealing with strings and memory blocks documentation-protocol Submitted vendor-provided or user-discovered protocol information, or similar data (measurements...) enhancement NUT protocols
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant