Skip to content

script: add ifxname as the escaped string - #624

Merged
rsmarples merged 3 commits into
masterfrom
script
Jun 7, 2026
Merged

rsmarples merged 3 commits into
masterfrom
script

Conversation

@rsmarples

Copy link
Copy Markdown
Member

$interface is used for creating files.
$ifxname is used to reference the interace directly.

This is an important distinction for OS who can put path separators or invalid path characters into the interface name.

$interface is used for creating files.
$ifxname is used to reference the interace directly.

This is an important distinction for OS who can put
path separators or invalid path characters into the
interface name.
@coderabbitai

coderabbitai Bot commented Jun 7, 2026 •

Copy link
Copy Markdown

Review Change Stack

Walkthrough

make_env() now checks return values from print_string(...) when creating filesystem-safe interface=, string-safe ifxname=, and entries in interface_order=; failures cause an immediate abort via goto eexit. if_printoptions() now lists ifxname among environment params.

Changes

Interface name escaping error handling in make_env()

Layer / File(s) Summary
Interface and interface_order escaping with error handling
src/script.c
if_printoptions() adds ifxname. make_env() checks print_string(...) return values when escaping rt->rt_ifp->name for interface= (OT_ESCFILE) and ifxname= (OT_ESCSTRING), and when appending per-interface names to interface_order=; failures goto eexit to abort env generation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • NetworkConfiguration/dhcpcd#622: Both PRs update interface-name escaping by using print_string(..., OT_ESCFILE/OT_ESCSTRING, ...) with failure handling to safely embed $ifxname/ifname in generated outputs.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly addresses the main change: adding ifxname as an escaped string variable to the script environment.
Description check ✅ Passed The description clearly explains the purpose of the changes, distinguishing between $interface and $ifxname and the rationale for supporting escaped interface names.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch script

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Infer (1.2.0)
src/script.c

src/script.c:48:10: fatal error: 'config.h' file not found
48 | #include "config.h"
| ^~~~~~~~~~
1 error generated.
Error: the following clang command did not run successfully:
/opt/infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/bin/clang-18
@/tmp/coderabbit-infer/0c6852561872fc4a6c86b865b5825e49d6c159f6-864bd15102480e5f/tmp/clang_command_.tmp.77ca97.txt
++Contents of '/tmp/coderabbit-infer/0c6852561872fc4a6c86b865b5825e49d6c159f6-864bd15102480e5f/tmp/clang_command_.tmp.77ca97.txt':
"-cc1" "-load"
"/opt/infer-linux-x86_64-v1.2.0/lib/infer/infer/bin/../../facebook-clang-plugins/libtooling/build/FacebookClangPlugin.dylib"
"-add-plugin" "BiniouASTExporter" "-plugin-arg-BiniouASTExporter" "-"
"-plugin-arg-BiniouASTExporter" "PREPEND_CURRENT_DIR=1"
"-plugin-arg-BiniouASTExporter" "MAX_STRING_SIZE=65535" "-cc1" "-triple"
"x86_64-unknown-linux-gnu" "-emit-obj" "-mrelax-all" "-disable-free"
"-clear-ast-before-bac

... [truncated 658 characters] ...

"/opt/infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/lib/clang/18/include"
"-internal-isystem" "/usr/local/include" "-internal-isystem"
"/usr/lib/gcc/x86_64-linux-gnu/12/../../../../x86_64-linux-gnu/include"
"-internal-externc-isystem" "/usr/include/x86_64-linux-gnu"
"-internal-externc-isystem" "/include" "-internal-externc-isystem"
"/usr/include" "-Wno-ignored-optimization-argument" "-Wno-everything"
"-ferror-limit" "19" "-fgnuc-version=4.2.1" "-fskip-odr-check-in-gmf"
"-D__GCC_HAVE_DWARF2_CFI_ASM=1" "-o"
"/tmp/coderabbit-infer/864bd15102480e5f/file.o" "-x" "c" "src/script.c"
"-O0" "-fno-builtin" "-include"
"/opt/infer-linux-x86_64-v1.2.0/lib/infer/infer/bin/../lib/clang_wrappers/global_defines.h"
"-Wno-everything"

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

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/script.c (1)

62-64: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add ifxname to the exported script-parameter list.

make_env() now exports ifxname, but if_printoptions() won’t advertise it because if_params is missing the entry.

Suggested update
 static const char *const if_params[] = { "interface", "protocol", "reason",
 	"pid", "ifcarrier", "ifmetric", "ifwireless", "ifflags", "ssid",
-	"profile", "interface_order", NULL };
+	"profile", "ifxname", "interface_order", NULL };
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/script.c` around lines 62 - 64, The exported parameter list array
if_params is missing "ifxname", so if_printoptions won't advertise the new
variable even though make_env() exports it; update the static const char *const
if_params[] initializer to include "ifxname" among the other parameter names
(e.g., alongside "interface", "protocol", "reason", etc.) so the
script-parameter list and the existing functions if_printoptions and make_env()
remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/script.c`:
- Around line 340-342: Reformat the print_string invocation to satisfy
clang-format: rewrite the call to print_string(if_name, sizeof(if_name),
OT_ESCFILE, ifp->name, strlen(ifp->name)) so the arguments are laid out
according to project style (e.g. each argument on its own indented line or kept
on one line if it fits), ensure the closing parenthesis and semicolon are
aligned properly, and keep the existing variables and the goto eexit path
unchanged; update the call site referencing print_string, if_name,
sizeof(if_name), OT_ESCFILE, ifp->name, and strlen(ifp->name).

---

Outside diff comments:
In `@src/script.c`:
- Around line 62-64: The exported parameter list array if_params is missing
"ifxname", so if_printoptions won't advertise the new variable even though
make_env() exports it; update the static const char *const if_params[]
initializer to include "ifxname" among the other parameter names (e.g.,
alongside "interface", "protocol", "reason", etc.) so the script-parameter list
and the existing functions if_printoptions and make_env() remain consistent.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 53a7cf9a-0348-4c86-8286-f4225a099d6b

📥 Commits

Reviewing files that changed from the base of the PR and between fa4de4f and 4d4617e.

📒 Files selected for processing (1)
  • src/script.c

Comment thread src/script.c Resolved

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
src/script.c (1)

340-342: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix the clang-format violation in the print_string call.

This formatting issue was already flagged by CI. Per the pipeline failure, the arguments should be reformatted so that ifp->name appears on the same line as OT_ESCFILE,.

🔧 Suggested formatting fix
-		if (print_string(if_name, sizeof(if_name), OT_ESCFILE,
-			ifp->name, strlen(ifp->name)) == -1)
+		if (print_string(if_name, sizeof(if_name), OT_ESCFILE, ifp->name,
+		    strlen(ifp->name)) == -1)
 			goto eexit;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/script.c` around lines 340 - 342, The call to print_string is split
across lines causing a clang-format violation; reformat the argument list so the
OT_ESCFILE and the following argument ifp->name appear on the same line. Locate
the print_string invocation that passes if_name, sizeof(if_name), OT_ESCFILE and
ifp->name, and collapse the line break so it reads with OT_ESCFILE, ifp->name on
the same line (preserving the trailing strlen(ifp->name)) to satisfy the
formatter.

Source: Pipeline failures

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/script.c`:
- Around line 405-407: The call to print_string is misformatted; update the
invocation of print_string (the call using variables if_name, sizeof(if_name),
OT_ESCFILE, rt->rt_ifp->name, strlen(rt->rt_ifp->name)) to match the project's
multi-line formatting style used elsewhere (same pattern as lines 340-342) so
clang-format passes — break arguments across lines with the function name and
opening parenthesis on the first line and each argument on its own indented
line, keeping the trailing ) and the == -1 check aligned with the goto eexit;.
- Around line 345-347: Reformat the print_string call to match the existing
multi-line argument layout used on lines 340–342: break the arguments across
lines the same way (one argument per line aligned with the opening parenthesis)
and keep the same tokens (print_string, if_name, sizeof(if_name), OT_ESCSTRING,
ifp->name, strlen(ifp->name)); preserve the error check and the goto eexit
behavior but adjust whitespace/line breaks to conform to the project's
clang-format style.

---

Duplicate comments:
In `@src/script.c`:
- Around line 340-342: The call to print_string is split across lines causing a
clang-format violation; reformat the argument list so the OT_ESCFILE and the
following argument ifp->name appear on the same line. Locate the print_string
invocation that passes if_name, sizeof(if_name), OT_ESCFILE and ifp->name, and
collapse the line break so it reads with OT_ESCFILE, ifp->name on the same line
(preserving the trailing strlen(ifp->name)) to satisfy the formatter.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5a012c79-6aec-4c99-86e5-d02a8817107b

📥 Commits

Reviewing files that changed from the base of the PR and between 4d4617e and 0c68525.

📒 Files selected for processing (1)
  • src/script.c

Comment thread src/script.c
Comment on lines +345 to +347
if (print_string(if_name, sizeof(if_name), OT_ESCSTRING,
ifp->name, strlen(ifp->name)) == -1)
goto eexit;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Apply consistent formatting to this print_string call.

This call follows the same formatting pattern as lines 340-342, which violates clang-format rules. For consistency with the project style and to avoid CI failures, apply the same fix here.

🔧 Suggested formatting fix
-		if (print_string(if_name, sizeof(if_name), OT_ESCSTRING,
-			ifp->name, strlen(ifp->name)) == -1)
+		if (print_string(if_name, sizeof(if_name), OT_ESCSTRING, ifp->name,
+		    strlen(ifp->name)) == -1)
 			goto eexit;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (print_string(if_name, sizeof(if_name), OT_ESCSTRING,
ifp->name, strlen(ifp->name)) == -1)
goto eexit;
if (print_string(if_name, sizeof(if_name), OT_ESCSTRING, ifp->name,
strlen(ifp->name)) == -1)
goto eexit;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/script.c` around lines 345 - 347, Reformat the print_string call to match
the existing multi-line argument layout used on lines 340–342: break the
arguments across lines the same way (one argument per line aligned with the
opening parenthesis) and keep the same tokens (print_string, if_name,
sizeof(if_name), OT_ESCSTRING, ifp->name, strlen(ifp->name)); preserve the error
check and the goto eexit behavior but adjust whitespace/line breaks to conform
to the project's clang-format style.

Comment thread src/script.c
Comment on lines +405 to +407
if (print_string(if_name, sizeof(if_name), OT_ESCFILE,
rt->rt_ifp->name, strlen(rt->rt_ifp->name)) == -1)
goto eexit;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Apply consistent formatting to this print_string call.

This call follows the same formatting pattern as lines 340-342, which violates clang-format rules. For consistency with the project style and to avoid CI failures, apply the same fix here.

🔧 Suggested formatting fix
-		if (print_string(if_name, sizeof(if_name), OT_ESCFILE,
-			rt->rt_ifp->name, strlen(rt->rt_ifp->name)) == -1)
+		if (print_string(if_name, sizeof(if_name), OT_ESCFILE, rt->rt_ifp->name,
+		    strlen(rt->rt_ifp->name)) == -1)
 			goto eexit;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (print_string(if_name, sizeof(if_name), OT_ESCFILE,
rt->rt_ifp->name, strlen(rt->rt_ifp->name)) == -1)
goto eexit;
if (print_string(if_name, sizeof(if_name), OT_ESCFILE, rt->rt_ifp->name,
strlen(rt->rt_ifp->name)) == -1)
goto eexit;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/script.c` around lines 405 - 407, The call to print_string is
misformatted; update the invocation of print_string (the call using variables
if_name, sizeof(if_name), OT_ESCFILE, rt->rt_ifp->name,
strlen(rt->rt_ifp->name)) to match the project's multi-line formatting style
used elsewhere (same pattern as lines 340-342) so clang-format passes — break
arguments across lines with the function name and opening parenthesis on the
first line and each argument on its own indented line, keeping the trailing )
and the == -1 check aligned with the goto eexit;.

@rsmarples
rsmarples merged commit 337720d into master Jun 7, 2026
6 checks passed
@rsmarples
rsmarples deleted the script branch June 7, 2026 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant