parseconf: accept literal hashes inside double quotes by user01010111 · Pull Request #3633 · networkupstools/nut

user01010111 · 2026-09-08T19:28:37Z

A description such as desc = "Rack #3", or a dummy-ups value such as outlet.3.desc: "Outlet #3", triggers a parse error with the original parser and loses the configured value. Accept a literal # inside double quotes. Unquoted hashes still start comments, and escaped characters still decode once inside or outside double quotes. Retain the historical error helper under #if 0 for future parser error handling.

Add portable regression coverage for all three parser entry points and a named NIT testcase checking exact text and decoded JSON through real localhost upsd, dummy-ups and upsc. The group handles sandbox preparation and cleanup. Each query reports its start and distinguishes watchdog expiry from an ordinary process failure, retaining both output streams in failure diagnostics. NIT_PARSECONF_TIMEOUT controls the overall per-query watchdog, defaulting to 60 seconds, independently of the connection timeout. Per-client debug settings and missing-Python/JSON skip/failure behaviour are preserved.

Update the configuration, network protocol and dummy-ups documentation, NEWS and UPGRADING. Explain how dummy-ups joins multiple value tokens and show the escaped-space alternative. The parser API, character filters, limits and incomplete-input behaviour remain unchanged. pconf_encode() still escapes hashes for older peers; default upsc output and driver code are unchanged. No protocol, ABI or driver version bump is needed. This remains independent of #3629.

Related: #607, #1306.

Validation of the original implementation, before this review follow-up:

Four quoted-hash fixtures failed on unmodified 2caa3c87501a1aba25a3070ad66757470bd1bf03 through line, file and character parsing. All 23 fixtures and boundary/encoder checks passed with the fix; the real NIT regression also failed before the fix.
Native macOS ARM64 and Linux ARM64 checks covered builds, compiler/language variants, sanitizers, parser integration and distribution/install paths. The original full Linux NIT run passed 31 groups with TLS required, and the original JSON checks passed on Python 2.6.9.

Validation of the review follow-up at fd8c0f65bcf311f89a62524a61a5d82be145f2a2:

Linux GCC/OpenSSL build with warnings and -Werror, the C parser test, and the helper self-test pass. The same two-second child succeeds with a five-second watchdog and fails with a one-second watchdog. Checks cover benign stderr, ordinary nonzero exit, captured output, invalid limits and the before/after timeout diagnostics.
The focused parser NIT group passes with plaintext and with TLS required: five passes, zero failures and zero skips in each run, including all 13 outlet fixtures. The TLS run verifies certificates and records TLS 1.3 with OpenSSL 3.6.4. Named-testcase counters, debug-setting isolation and missing-prerequisite paths also pass focused checks.
make stylecheck, source ASCII and whitespace checks pass. The real user-manual, dummy-ups manual and UPGRADING render, and the changed documents pass their spelling targets.
make distcheck-light with AM_DISTCHECK_CONFIGURE_FLAGS=--disable-spellcheck passes the distribution build, all 12 tests and install/uninstall checks. All six follow-up source files match the generated archive. The unmodified default spelling gate rejects kernel's and firmware's in unchanged NEWS and apcmicrolink documentation; that baseline dictionary failure is retained as a limitation.

The successful socket-dependent checks ran with localhost access enabled. Earlier isolated attempts were denied ordinary local sockets. Python 2 was unavailable for the follow-up runtime checks. No physical UPS hardware, live deployment or native NetBSD/NSS reproduction was exercised in the follow-up. The reported NetBSD delay still needs a native rerun; the improved watchdog diagnostics do not establish its underlying cause. EOF and multiline semantics remain a separate discussion.

AI assistance: the original implementation used OpenAI Codex with gpt-6-astra and gpt-daybreak-blue-latest. This review follow-up used OpenAI Codex with gpt-6-astra at high reasoning. The human contributor authorised the DCO sign-off and remains responsible for the contribution.

Accept quoted hashes in the shared parser while retaining unquoted comments and escaped output for older peers. Keep default upsc text output unchanged and document configuration, protocol and dummy-ups behavior. Add portable parser regressions and localhost text/JSON integration. Validated with C89/C99/C11 GCC and Clang builds, sanitizers, full NIT, package/install checks, documentation checks and distcheck-light. Related: networkupstools#607, networkupstools#1306, networkupstools#3629 AI assistance: OpenAI Codex with gpt-6-astra (x-high reasoning) and gpt-daybreak-blue-latest (high reasoning). The human contributor remains responsible for reviewing and submitting the change. Signed-off-by: user01010111 <12504630+user01010111@users.noreply.github.com>

github-actions · 2026-09-08T19:35:33Z

A ZIP file with standard source tarball and another tarball with pre-built docs for commit fd8c0f6 is temporarily available: NUT-tarballs-PR-3633.zip.

AppVeyorBot · 2026-09-08T20:53:53Z

✅ Build nut 2.8.5.5259-master completed (commit 744fe2319b by @user01010111)

artifacts
A 7-zip archive with built NUT for Windows debug binaries is temporarily available: NUT-for-Windows-x86_64-SNAPSHOT-2.8.5.5259-master.7z

AppVeyorBot · 2026-09-08T20:53:54Z

✅ Build nut 2.8.5.5259-master completed (commit 744fe2319b by @user01010111)

Probe the selected Python interpreter for json before preparing the parser fixtures. Report a skip in the default suite if the prerequisite is absent, while preserving failure for an explicitly requested parser test group. Keep the existing parser and JSON assertions unchanged. Jenkins exposed a Python 3.7 installation unable to import its standard json module. The worker still needs repair to restore this integration coverage. Validated the missing-module failure and skip paths, native macOS and Linux parser integration, Linux make check (13 tests), style/spelling checks, and default make distcheck-light. The missing module was simulated locally; the exact Jenkins worker and physical UPS hardware were not tested. Related: networkupstools#3633 AI assistance: OpenAI Codex with gpt-6-astra. The human contributor remains responsible for reviewing and submitting the change. Signed-off-by: user01010111 <12504630+user01010111@users.noreply.github.com>

AppVeyorBot · 2026-09-10T05:21:51Z

✅ Build nut 2.8.5.5270-master completed (commit 35cfb0fa6d by @user01010111)

artifacts
A 7-zip archive with built NUT for Windows debug binaries is temporarily available: NUT-for-Windows-x86_64-SNAPSHOT-2.8.5.5270-master.7z

jimklimov · 2026-09-11T07:25:28Z

Something not right in new test script (expecting empty stderr? Did not drill into it yet):

...
Fri Sep 11 02:26:48 UTC 2026 [INFO] [testcase_sandbox_start_drivers_after_upsd] Got output for UPS2:
OL BOOST
SSL handshake done successfully with client ::1
Init SSL with certificate database located at /var/shm/jenkins-nutci-netbsd-92-amd64/nut_nut_PR-3633/obj/tests/NIT/tmp/etc/cert/upsmon
Connecting in SSL to 'localhost' and look at certificate called 'NIT data server'
Intended to authenticate server localhost:35052 : SUCCESS
SSL handshake done successfully with server localhost
Connected to NUT server localhost in SSL
Fri Sep 11 02:26:58 UTC 2026 [INFO] [testcase_sandbox_start_drivers_after_upsd] Got JSON output for UPS2:
"OL BOOST"
Fri Sep 11 02:26:58 UTC 2026 [INFO] [testcase_sandbox_start_drivers_after_upsd] PASSED: Expected drivers are now responding via UPSD
Traceback (most recent call last):
  File "/var/shm/jenkins-nutci-netbsd-92-amd64/nut_nut_PR-3633/tests/NIT/parseconf-test.py", line 112, in <module>
    check(sys.argv[1], sys.argv[2])
    ~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/var/shm/jenkins-nutci-netbsd-92-amd64/nut_nut_PR-3633/tests/NIT/parseconf-test.py", line 78, in check
    plain = query(upsc, ups).splitlines()
            ~~~~~^^^^^^^^^^^
  File "/var/shm/jenkins-nutci-netbsd-92-amd64/nut_nut_PR-3633/tests/NIT/parseconf-test.py", line 64, in query
    raise AssertionError('upsc %r returned %d: %r' %
                         (args, process.returncode, error))
AssertionError: upsc ('dummy@127.0.0.1:35052',) returned -9: b'Init SSL with certificate database located at /var/shm/jenkins-nutci-netbsd-92-amd64/nut_nut_PR-3633/obj/tests/NIT/tmp/etc/cert/upsmon\n'
Fri Sep 11 02:27:56 UTC 2026 [INFO] Stopping test daemons
mainloop: Interrupted system call
Signal 15: exiting
Signal 15: exiting
Signal 15: exiting
Signal 15: exiting

================================
Fri Sep 11 02:27:56 UTC 2026 [INFO] OVERALL: PASSED=26 FAILED=1 SKIPPED=4
   1 [F] testgroup_sandbox_parseconf
   1 [S] testcase_sandbox_cppnit_simple_admin
   1 [S] testcase_sandbox_cppnit_upsmon_master
   1 [S] testcase_sandbox_cppnit_upsmon_primary
   1 [S] testcase_sandbox_cppnit_without_creds
Fri Sep 11 02:27:56 UTC 2026 [FATAL] Error: Some test scenarios failed!

UPDATE: Reading the script code, I think the fault was not due to parsing an unexpected "Init SSL..." as was my original theory, but process.communicate() for upsc returning -9 (as in kill -9?) and then reported in a somehow clumsy manner :)

It would help troubleshooting if the test case reported itself starting (and BTW testgroup_sandbox_parseconf should really be a sandbox preparation and a caller of test cases in that box - even if one case so far).

jimklimov · 2026-09-11T07:35:34Z

-/* set up the error reporting details */
-static void pconf_seterr(PCONF_CTX_t *ctx, const char *errmsg)
-{
-	snprintf(ctx->errmsg, PCONF_ERR_LEN, "%s", errmsg);
-
-	ctx->error = 1;
-}
-


I feel uneasy about dropping this method, it was with us for 20+ years. This drop also makes STATE_PARSEERR value un-used. There are a couple of other uses for ctx->errmsg at least (although those sanity-checkers probably don't want to change ctx->error value nor their return codes).

I'd rather hide these removed codes into #if 0 fences as an example how to revive this functionality should our parser report errors again, or find actual use for them in error handling. We should have some other cases, right?

One of these discussions about the parser mentioned handling multi-line strings where properly quoted, and qc_literal() confirms this is intentional (with removal of the actual newline character from the collected string). I first thought about reporting these as errors in a quoted context, but not sure about that now (the code probably meant to handle lines ending with a backslash+newline escape, no idea if it was about a simple or escaped newline inside the quotes too).

But we could certainly pconf_seterr() if there is an EOF while we are inside the quoted string context.

Not sure about other non-ASCII values (unprintable < 32 other than 10==newline, or locale-specific > 127, or potentially unicode) - it is probably a bit too late to start rejecting outright or handling them somehow specially :)

On the EOF and multiline point, fd8c0f65 preserves the existing behaviour. The current tests deliberately retain partial-input acceptance, and the character-at-a-time entry point has no EOF signal. I suggest agreeing EOF handling across the parser entry points in a separate follow-up, keeping multiline and character handling unchanged here.

Agreed. fd8c0f65 restores pconf_seterr() under #if 0, preserving the example alongside the existing STATE_PARSEERR handling. Active parser behaviour is unchanged by this follow-up.

jimklimov · 2026-09-11T07:38:48Z

+	outlet.1.desc: Outlet #1
+	outlet.2.desc: "Outlet \#2"
+	outlet.3.desc: "Outlet #3"
+	outlet.4.desc: Outlet \#4
+
+These values become `Outlet`, `Outlet #2`, `Outlet #3`, and `Outlet #4`,
+respectively. Older versions of the parser require `\#` even inside
+double quotes. Backslashes quote the next permitted character once, so
+`\n` becomes `n`, while `\\` becomes a single literal backslash.
+A backslash followed by a physical newline continues the logical line.
+An unescaped `=` outside double quotes is a separate token; quote or
+escape it when it is part of a value.
+


Can you please double-check and clarify whether "Outlet #4" (upon reading from such data in e.g. config files) becomes two tokens, while others would be one token. When reading from a .dev/.seq file in dummy-ups, do we actually inject only the first value token for the key token?

Maybe add an example with a backslash-escaped space as mentioned in config-notes.txt now?

Confirmed: unquoted Outlet \#4 produces two value tokens, Outlet and #4. dummy-ups joins all value tokens with spaces, so it stores Outlet #4; it does not discard the second token.

fd8c0f65 explains this and adds Outlet\ \#4 as the single-token equivalent, plus a real integration fixture for that form. The configuration notes also show escaped spaces in SHUTDOWNCMD. The updated examples render correctly and the parser integration checks pass.

jimklimov · 2026-09-11T07:42:12Z

 {
-	/* user is trying to break us */
-	if (ctx->ch == '#') {
-		pconf_seterr(ctx, "Unbalanced word due to unescaped # in quotes");
-		endofword(ctx);
-
-		/* this makes us drop all the way out of the caller */
-		return STATE_PARSEERR;
-	}
-


CC @aquette @arjendekorte @clepple : do you remember any rationale for rejecting an unescaped hash sign INSIDE double-quoted values (e.g. passwords, descriptions like "Outlet #4" etc.)?

How here a "user is trying to break us" ?

@user01010111 : if we do go through with this, it is a significant enough change to mention also in UPGRADING.adoc.

I cannot establish the original rationale from the code alone, so I would welcome the historical context. The change permits literal hashes inside double quotes while preserving unquoted comments. pconf_encode() continues escaping hashes for compatibility with older peers. I have left this question open for the maintainers who know the history.

Agreed. fd8c0f65 adds an UPGRADING.adoc entry under 2.8.5 to 2.8.6. It explains literal hashes inside double quotes, unchanged unquoted comments, continued \# support and the need to retain escaped forms when sharing configuration with older versions. Encoded protocol output still escapes hashes for older peers. The PR description is updated accordingly.

jimklimov · 2026-09-11T08:08:47Z

-/* set up the error reporting details */
-static void pconf_seterr(PCONF_CTX_t *ctx, const char *errmsg)
-{
-	snprintf(ctx->errmsg, PCONF_ERR_LEN, "%s", errmsg);
-
-	ctx->error = 1;
-}
-


One of these discussions about the parser mentioned handling multi-line strings where properly quoted, and qc_literal() confirms this is intentional (with removal of the actual newline character from the collected string). I first thought about reporting these as errors in a quoted context, but not sure about that now (the code probably meant to handle lines ending with a backslash+newline escape, no idea if it was about a simple or escaped newline inside the quotes too).

But we could certainly pconf_seterr() if there is an EOF while we are inside the quoted string context.

Not sure about other non-ASCII values (unprintable < 32 other than 10==newline, or locale-specific > 127, or potentially unicode) - it is probably a bit too late to start rejecting outright or handling them somehow specially :)

jimklimov · 2026-09-11T08:21:48Z

+#!/usr/bin/env python
+# Exact values through configuration, dummy-ups, upsd and upsc.


Similarly to changes in some other PR of this bunch, makes sense to add a comment here that:

normally the caller (nit.sh) selects which $PYTHON interpreter runs this script, so we do not bother with .in templating

expected to be, and should remain, compatible with Py2 and Py3

Added beside the shebang in fd8c0f65: nit.sh selects $PYTHON, and the script must remain compatible with Python 2.6+ and Python 3. The watchdog continues using the older-compatible timer and subprocess APIs. Python 3 helper/integration checks pass.

I also built an isolated CPython 2.6.9 runtime and ran the published helper self-test plus the real parser integration group with that interpreter. The self-test passes; plaintext and certificate-verified TLS each pass with five passes, zero failures and zero skips, covering all 13 fixtures. The TLS run negotiated TLS 1.3. No source changes were needed.

Validation assistance: OpenAI Codex (gpt-6-astra, high reasoning).

Report query starts, watchdog expiry and captured process output. Allow a bounded per-query NIT_PARSECONF_TIMEOUT, defaulting to 60 seconds, and preserve per-client debug settings in the named parser testcase. Retain the parser error helper under #if 0. Clarify dummy-ups token joining, escaped spaces, Python compatibility and the quoted-hash upgrade behaviour. Keep EOF, multiline and active parser semantics unchanged. AI assistance: OpenAI Codex with gpt-6-astra. Signed-off-by: user01010111 <12504630+user01010111@users.noreply.github.com>

user01010111 · 2026-09-13T09:57:05Z

Following up on the NetBSD test result, fd8c0f65 moves execution and result reporting into testcase_sandbox_parseconf, with the group handling sandbox preparation and cleanup. Queries now announce and flush their start, distinguish watchdog expiry from ordinary failure, and retain stdout/stderr in failure diagnostics. NIT_PARSECONF_TIMEOUT provides a bounded overall query limit, defaulting to 60 seconds; the per-client debug setting is preserved.

The old ten-second watchdog can produce the reported -9, but the log does not prove it caused this particular kill or identify the NetBSD/NSS delay. The updated helper checks pass, and the real parser group passes locally with TLS 1.3 and certificate verification: five passes, zero failures and zero skips. A native NetBSD rerun is still needed.

The distribution build/install/uninstall check and all 12 tests also pass with spellcheck disabled; the default gate rejects existing kernel's/firmware's entries in unchanged files. Changed-document spelling checks pass. The PR description records this exception and separates the original platform checks from the follow-up validation.

Review and implementation assistance: OpenAI Codex (gpt-6-astra).

AppVeyorBot · 2026-09-13T10:45:19Z

✅ Build nut 2.8.5.5281-master completed (commit 06bf6b5834 by @user01010111)

artifacts
A 7-zip archive with built NUT for Windows debug binaries is temporarily available: NUT-for-Windows-x86_64-SNAPSHOT-2.8.5.5281-master.7z

AppVeyorBot · 2026-09-13T10:45:20Z

✅ Build nut 2.8.5.5281-master completed (commit 06bf6b5834 by @user01010111)

user01010111 mentioned this pull request Sep 8, 2026

PyNUTClient: decode NUT values and escape authentication arguments #3629

Open

8 tasks

jimklimov reviewed Sep 11, 2026

View reviewed changes

jimklimov requested changes Sep 11, 2026

View reviewed changes

jimklimov added this to NUT QA Sep 11, 2026

jimklimov added this to the 2.8.6 milestone Sep 11, 2026

jimklimov moved this to In Progress in NUT QA Sep 11, 2026

jimklimov added documentation C-str Issues and PRs about C/C++ methods, headers and data types dealing with strings and memory blocks AI For good or bad, machine tools are upon us. Humans are still the responsible ones. labels Sep 11, 2026

user01010111 requested a review from jimklimov September 14, 2026 02:18

		#!/usr/bin/env python
		# Exact values through configuration, dummy-ups, upsd and upsc.

Uh oh!

Uh oh!

Conversation

user01010111 commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AppVeyorBot commented Sep 8, 2026

Uh oh!

AppVeyorBot commented Sep 8, 2026

Uh oh!

AppVeyorBot commented Sep 10, 2026

Uh oh!

jimklimov commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

user01010111 Sep 13, 2026 • 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!

user01010111 commented Sep 13, 2026

Uh oh!

AppVeyorBot commented Sep 13, 2026

Uh oh!

AppVeyorBot commented Sep 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

user01010111 commented Sep 8, 2026 •

edited

Loading

github-actions Bot commented Sep 8, 2026 •

edited

Loading

jimklimov commented Sep 11, 2026 •

edited

Loading

user01010111 Sep 13, 2026 •

edited

Loading