fix: escape control characters in IfcfgUtil.ValueEscape

[suraj-cmd]

Problem

IfcfgUtil.ValueEscape() in library/network_connections.py guards its
ANSI-C quoting branch with ord(c) < ord(c), which is always False. The
escaping path is therefore unreachable and control characters are copied
verbatim into the $'...' string.

ValueEscape() is called from ifcfg_to_content(), which writes one
KEY=VALUE per line. A value containing a newline breaks that structure
in the generated ifcfg file.

There is a second defect behind the first: the escape emits
"\\" + str(ord(c)), a decimal code point, but ANSI-C quoting reads
\nnn as octal. Fixing only the comparison would make the branch
reachable and produce the wrong character — newline (10) would emit
\10, which bash decodes as octal 10 = backspace.

Fix

Compare against ord(" ") and emit %03o. Uses % formatting to stay
Python 2.6-compatible per the constraint on library/ code.

Tests

Four unit tests added covering the ANSI-C path, the double-quoting path
and the unquoted path.

Without the fix:

FAILED TestIfcfgUtilValueEscape::test_control_char_escaped_as_octal
  AssertionError: "$'line1\nline2'" != "$'line1\\012line2'"
  + $'line1\012line2'
  - $'line1
  - line2'

FAILED TestIfcfgUtilValueEscape::test_control_char_with_quote_and_backslash
  AssertionError: "$'a\n\\'b\\\\c'" != "$'a\\012\\'b\\\\c'"
  + $'a\012\'b\\c'
  - $'a
  - \'b\\c'

3 failed, 2 passed, 152 deselected

With the fix:

test_control_char_escaped_as_octal PASSED
test_control_char_with_quote_and_backslash PASSED
test_double_quoting_path_is_unchanged PASSED
test_plain_value_is_not_quoted PASSED

4 passed, 153 deselected

Full suite on macOS: 153 passed, 3 skipped, 1 failed. The single
failure is TestSysUtils::test_link_read_permaddress, which needs a
Linux SIOCETHTOOL ioctl and fails identically on an unpatched
checkout — it appears in the "without the fix" run above for that
reason. black --check and flake8 are clean on both changed files.

Verified the octal form round-trips: printf '%s' $'line1\012line2'
produces a real newline.

Signed-off-by: Suraj Patil surajpatil522@gmail.com

Summary by CodeRabbit

• Bug Fixes

  • Corrected escaping of control characters in network configuration values.
  • Control characters are now represented using three-digit octal notation.
  • Preserved proper handling of quotes, backslashes, and already-quoted paths.

• Tests

  • Added coverage for plain values, control-character escaping, embedded special characters, and shell-style paths.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d80ad049-6786-42b0-9761-367f5d34ae7b

📥 Commits

Reviewing files that changed from the base of the PR and between dce6742 and 8417a35.

📒 Files selected for processing (1)

• library/network_connections.py

🚧 Files skipped from review as they are similar to previous changes (1)

• library/network_connections.py

---

📝 Walkthrough

Walkthrough

IfcfgUtil.ValueEscape now documents its Bash quoting behavior and emits three-digit octal escapes for control characters. Unit tests cover plain values, control characters, quote and backslash escaping, and quoted paths.

Changes

Value escaping

Layer / File(s)	Summary
Control-character escaping and validation library/network_connections.py, tests/unit/test_network_connections.py	IfcfgUtil.ValueEscape documents ANSI-C and double-quote escaping rules. It detects characters below the space character and escapes them with three-digit octal notation. Unit tests cover plain values, control characters, embedded quotes and backslashes, and unchanged double-quoted paths.

Mergeability Score: ⚪ Minimal · up to 8417a

This change correctly escapes control characters in generated ifcfg values and adds focused coverage. No actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description Format	⚠️ Warning	The description is a bug-fix PR but uses Problem/Fix/Tests headings and omits required Cause:, Consequences:, Fix:, and Result: sections; Signed-off-by: is present.	Rewrite the description with Cause:, Consequences:, Fix:, Result:, and Signed-off-by: sections, using the repository template format.

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title follows Conventional Commits format and accurately describes the control-character escaping fix.
Description check	✅ Passed	The description clearly explains the problem, fix, tests, and unrelated platform failure, although it does not use the template headings.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 46.01%. Comparing base (1b57520) to head (8417a35).
⚠️ Report is 121 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #894      +/-   ##
==========================================
+ Coverage   43.11%   46.01%   +2.89%     
==========================================
  Files          12       13       +1     
  Lines        3124     3277     +153     
==========================================
+ Hits         1347     1508     +161     
+ Misses       1777     1769       -8     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[richm]

@suraj-cmd where is the documentation for escaping - which characters must be escaped, and how to escape them?

[suraj-cmd]

The rules come from shell quoting, since ifcfg files are shell syntax. I've
added a docstring to ValueEscape citing both sections so the reference lives
next to the code.

ANSI-C quoting, $'...' — Bash Reference Manual 3.1.2.4
https://www.gnu.org/software/bash/manual/html_node/ANSI_002dC-Quoting.html
\nnn the eight-bit character whose value is the octal value nnn
(one to three octal digits)
\ backslash
' single quote

Double quoting, "..." — Bash Reference Manual 3.1.2.3
https://www.gnu.org/software/bash/manual/html_node/Double-Quotes.html
$ ` \ " retain their special meaning and need a preceding backslash

Both branches in the existing code already match those sets, so "which
characters" was never wrong — the control-character branch just never ran,
because of the ord(c) < ord(c) comparison.

On octal vs decimal:

$ printf '%s\n' $'A\012B' | od -c
0000000   A  \n   B          <- octal 012 = newline

$ printf '%s\n' $'A\10B' | od -c
0000000   A  \b   B          <- \10 read as octal 8 = backspace

So the old str(ord(c)) would have written \10 for a newline and the shell would
have decoded a backspace.

One thing I'd like your view on: NetworkManager's ifcfg-rh reader (shvar.c) is
not a shell — does it honour $'...' ANSI-C quoting? The existing code already
assumes it does, and this patch doesn't change that, but it seemed worth
asking.

[richm]

@bengal can you answer this question? ^^^

One thing I'd like your view on: NetworkManager's ifcfg-rh reader (shvar.c) is
not a shell — does it honour $'...' ANSI-C quoting? The existing code already
assumes it does, and this patch doesn't change that, but it seemed worth
asking.