Skip to content

bug: If MountPoint.options is an empty string, an fstab is entry is corrupt #37

Merged
bfjelds merged 5 commits into
mainfrom
user/bfjelds/empty-mount-options-as-default
Jul 18, 2025
Merged

bug: If MountPoint.options is an empty string, an fstab is entry is corrupt #37
bfjelds merged 5 commits into
mainfrom
user/bfjelds/empty-mount-options-as-default

Conversation

@bfjelds
Copy link
Copy Markdown
Member

@bfjelds bfjelds commented Jul 18, 2025

🔍 Description

When creating fstab entries, we try to ensure that "defaults" is used if a mount point has empty options:

        // If the options are empty, use "defaults" as the default
        let options = if self.options.is_empty() {
            "defaults".into()
        } else {
            self.options.join(",")
        };

We don't quite get this correct, as self.options is created from MountOptions.to_*_vec() into a Vec. Currently, at runtime self.options is a vector with one empty string element ([""]).

In this case, we create an fstab entry that is missing the options parameter, which means that the OS treats the subsequent parameter we create (0) as the options.

Proposed solution is to change MountOptions.to_*_vec() to filter empty strings.

@bfjelds bfjelds requested a review from a team as a code owner July 18, 2025 21:27
Comment thread osutils/src/tabfile.rs Outdated
Comment thread trident_api/src/config/host/storage/filesystem.rs Outdated
@bfjelds bfjelds changed the title bug: fix empty check; add test bug: If MountPoint.options is an empty string, an fstab is entry is corrupt Jul 18, 2025
@bfjelds bfjelds merged commit 4801de4 into main Jul 18, 2025
59 checks passed
alejandro-microsoft pushed a commit that referenced this pull request Aug 4, 2025
…orrupt (#37)

* fix empty check; add test

* update comment

* change at vec creation

* trim

* clippy
@bfjelds bfjelds deleted the user/bfjelds/empty-mount-options-as-default branch August 13, 2025 19:58
bfjelds added a commit that referenced this pull request May 25, 2026
Fixes from frhuelsz code review:

- grub_cfg: skip $variable references during partition extraction (#30)
- users: use * lock marker instead of ! for AZL UsePAM=no (#31)
- users: fsync + parent dir sync in atomic_write_file (#33)
- users: propagate unexpected errors from check_user_exists (#36)
- users: explicitly lock new users when no password set (#43)
- users: skip set_primary_group for new users (useradd -g suffices) (#46)
- users: named constants for shadow/passwd field indices (#41)
- users: named constants for SSH dir/file permissions (#52)
- config: add deny_unknown_fields to remaining serde structs (#38)
- config: doc comment on PasswordType intentional reduction (#37)
- services: error on non-UTF-8 root path instead of silent fallback (#51)
- modules: document intentional Disable fidelity fix vs Go (#50)
- selinux: doc comment clarifying non-overlapping call paths (#45)
- lib: update OsModifierContext doc for MOS config path (#54)
- lib: add caller invariant doc on modify_os for UKI (#32)
- lib: add BootTarget enum note for future UKI-awareness (#53)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
frhuelsz added a commit that referenced this pull request Jun 2, 2026
The installed dependency tree has long required Node 20+ (the entire
Docusaurus 3.9 stack declares 'engines.node: >=20.0', and the CI
workflow .github/workflows/deploy-website.yaml already pins
node-version: 20.x), but the website package itself still advertised
'engines.node: >=18.0'.

The recent serialize-javascript 6 -> 7 bump (commit decdb0c,
resolving Dependabot alerts #37 and #90) makes the strictness
explicit: serialize-javascript@7 declares 'engines.node: >=20.0.0'.

Align the website's engines field with what the deps and CI already
require so npm install gives a consistent signal to contributors and
to lockfile metadata.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.

2 participants