fstab-generator: support for x-systemd.onfailure#42440
Conversation
Claude review of PR #42440 (8d535a7)This is a small, well-scoped change adding Must fix
Suggestions
Nits
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (isempty(opts)) | ||
| return 0; | ||
|
|
||
| r = fstab_filter_options(opts, |
There was a problem hiding this comment.
Claude: nit: The fstab_filter_options() call (lines 437-442) and the log_error_errno() message (lines 444-446) are each spread over multiple lines, whereas every sibling helper writes them as single lines (e.g. r = fstab_filter_options(opts, filter, NULL, NULL, &paths, NULL); and return log_error_errno(r, "Failed to parse options for '%s': %m", where);). Collapsing them to one-liners would match the file's style.
nikhilar2410
force-pushed
the
x-systemd-onfailure
branch
from
50c65ce to
1f09717
Compare
| STRV_FOREACH(u, units) { | ||
| if (isempty(*u)) | ||
| continue; | ||
|
|
||
| fprintf(f, "OnFailure=%s\n", *u); |
There was a problem hiding this comment.
Claude: suggestion: The x-systemd.onfailure= value is written verbatim with no escaping. When PID1 loads the generated unit, OnFailure= is parsed by config_parse_unit_deps(), which runs each token through unit_name_printf() and therefore expands %-specifiers. The path-writing sibling write_mounts_for() escapes its values via specifier_escape_strv() for exactly this reason. As written, an fstab value containing a literal % (e.g. x-systemd.onfailure=foo-%n.service) will be reinterpreted rather than emitted literally, so the resulting dependency can differ from what the administrator typed. Consider running the values through specifier_escape_strv() before emitting them so the written value stays literal. Low impact (fstab is admin-controlled), hence a suggestion.
nikhilar2410
force-pushed
the
x-systemd-onfailure
branch
from
1f09717 to
8ffaeda
Compare
| } | ||
| return 0; |
There was a problem hiding this comment.
Claude: nit: Missing blank line before the final return 0;. The sibling helpers in this file (write_dependency(), write_mounts_for()) separate the emit/loop block from the trailing return 0; with a blank line; matching that here keeps the helpers consistent.
| /dev/sdx16 /mnt/nofail auto nofail 0 0 | ||
| /dev/sdx17 /mnt/wantedby-automount auto x-systemd.wanted-by=foo.service,x-systemd.automount 0 0 | ||
| /dev/sdx18 /mnt/onfailure auto x-systemd.onfailure=foo.service 0 0 | ||
| /dev/sdx19 /mnt/onfailure-multi auto x-systemd.onfailure=foo.service,x-systemd.onfailure=bar.service 0 0 |
There was a problem hiding this comment.
Claude: suggestion: The generator explicitly handles an empty x-systemd.onfailure= value (the if (isempty(*u)) continue; branch in write_on_failure()), but no test fixture exercises it. Consider adding an fstab entry such as x-systemd.onfailure=foo.service,x-systemd.onfailure= so the empty-value skip path is covered and the expected unit confirms only the non-empty OnFailure= is emitted.
| /dev/sdx16 /mnt/nofail auto nofail 0 0 | ||
| /dev/sdx17 /mnt/wantedby-automount auto x-systemd.wanted-by=foo.service,x-systemd.automount 0 0 | ||
| /dev/sdx18 /mnt/onfailure auto x-systemd.onfailure=foo.service 0 0 | ||
| /dev/sdx19 /mnt/onfailure-multi auto x-systemd.onfailure=foo.service,x-systemd.onfailure=bar.service 0 0 |
There was a problem hiding this comment.
Claude: suggestion: The man page entry documents that x-systemd.onfailure= "always applies to the generated mount unit only, regardless of whether x-systemd.automount has been specified", but no fixture combines x-systemd.onfailure= with x-systemd.automount. Consider adding such an entry and a .automount expected unit to confirm the documented behavior (OnFailure= lands on the .mount unit, not the .automount unit).
Add support for the x-systemd.onfailure fstab option in fstab-generator. This allows generated mount units to include the systemd OnFailure= dependency specified through /etc/fstab. The implementation parses x-systemd.onfailure entries using fstab_filter_options() and emits corresponding OnFailure= directives into generated mount units. Example: /dev/sda1 /data ext4 defaults,x-systemd.onfailure=emergency.target 0 2 generates: OnFailure=emergency.target This makes it possible to attach recovery targets or services to mount failures directly from fstab configuration. Add documentation and generator tests. Signed-off-by: Nikhil R <[email protected]>
nikhilar2410
force-pushed
the
x-systemd-onfailure
branch
from
8ffaeda to
8d535a7
Compare
github-actions
Bot
added
the
please-review
| <varlistentry> | ||
| <term><option>x-systemd.onfailure=</option></term> | ||
|
|
||
| <listitem><para>In the generated mount unit, configures an |
There was a problem hiding this comment.
Claude: nit: Claude: This entry refers to the "generated mount unit" (here, and again at line 296), but every sibling x-systemd.* option on this page says "created mount unit" (e.g. x-systemd.wanted-by=, x-systemd.wants-mounts-for=, and the equivalent automount caveat at lines 248-250). Use "created mount unit" for consistency.
| <term><option>x-systemd.onfailure=</option></term> | ||
|
|
||
| <listitem><para>In the generated mount unit, configures an | ||
| <varname>OnFailure=</varname> dependency. The argument should be a unit name. |
There was a problem hiding this comment.
Claude: nit: Claude: This line is indented with a hard tab (followed by a space), whereas the rest of systemd.mount.xml, including the surrounding option entries, uses spaces only. Re-indent with spaces to match the file's style.
2 participants
Add support for the x-systemd.onfailure= fstab option in systemd-fstab-generator.
This allows configuring OnFailure= dependencies for generated mount units directly through /etc/fstab.
For example:
/dev/sda1 /data ext4 defaults,x-systemd.onfailure=emergency.target 0 2
results in the generated mount unit containing:
[Unit]
OnFailure=emergency.target
This makes it possible to attach recovery targets or services to mount failures directly from fstab configuration.
Add documentation and generator tests.