Skip to content

nixos-generate-config: account for mount points & devices with spaces & tabs in the name #50234

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 3, 2019
Merged

nixos-generate-config: account for mount points & devices with spaces & tabs in the name #50234

merged 1 commit into from
Feb 3, 2019
+5 −1

Conversation

aanderse
Copy link
Member

@aanderse aanderse commented Nov 11, 2018 •
edited
Loading

Motivation for this change

nixos-generate-config doesn't properly reproduce my system because I have a file system mount which has a space in it.

Running cat /etc/fstab | grep Home returns my existing mount: data/Home\040Videos /srv/Home\040Videos zfs defaults 0 0
Now if I run nixos-generate-config --show-hardware-config | grep Home then I get no results.

Running cat /proc/self/mountinfo | grep Home shows me that a space is escaped by \040 as such: 287 27 0:51 / /srv/Home\040Videos rw,relatime shared:158 - zfs data/Home\040Videos rw,xattr,noacl

See #49354

Note that I tested this on my system which uses zfs and everything seems fine, but I did not test against any other options, such as BTRFS, or disks with labels.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

Sorry, something went wrong.

@aanderse aanderse requested a review from infinisil as a code owner November 11, 2018 14:58
@GrahamcOfBorg GrahamcOfBorg added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Nov 11, 2018
@aanderse
Copy link
Member Author

@GrahamcOfBorg test installer

@GrahamcOfBorg
Copy link

Success on aarch64-linux

Attempted: tests.installer

No partial log is available.

@GrahamcOfBorg
Copy link

Success on x86_64-linux

Attempted: tests.installer

No partial log is available.

@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Nov 11, 2018
@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-january/2002/2

@timokau
Copy link
Member

timokau commented Feb 2, 2019

Have you tried any other special characters (http://www.asciitable.com/)?

Assuming the format is the same as fstab, at least tab would need special attention (http://man7.org/linux/man-pages/man5/fstab.5.html). The /proc manpage doesn't give additional info (http://man7.org/linux/man-pages/man5/proc.5.html).

@timokau
Copy link
Member

timokau commented Feb 2, 2019

And is there any reason why you think this might work differently on ZFS vs BTRFS (or ext etc.)?

@aanderse
Copy link
Member Author

aanderse commented Feb 2, 2019

Have you tried any other special characters (http://www.asciitable.com/)?

Assuming the format is the same as fstab, at least tab would need special attention (http://man7.org/linux/man-pages/man5/fstab.5.html). The /proc manpage doesn't give additional info (http://man7.org/linux/man-pages/man5/proc.5.html).

Given the manual specifically mentions tabs I guess it does make sense to escape that character as well. I have not tried any other characters, and without a use case or documentation specifically stating what characters are accepted and how to handle them I don't think that we really should support other characters.

And is there any reason why you think this might work differently on ZFS vs BTRFS (or ext etc.)?

No reason. More testing is better and I'm not familiar enough with this script to be 100% certain of my change without peer review or other people testing it.

@timokau
Copy link
Member

timokau commented Feb 2, 2019

Makes sense.

@aanderse aanderse force-pushed the nixos-generate-config branch from 7140cd4 to 6156de6 Compare February 2, 2019 16:55
@aanderse aanderse changed the title nixos-generate-config: account for mount points & devices with spaces in the name nixos-generate-config: account for mount points & devices with spaces & tabs in the name Feb 2, 2019
@aanderse
Copy link
Member Author

aanderse commented Feb 2, 2019

@GrahamcOfBorg test installer

@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Feb 2, 2019
@aanderse
Copy link
Member Author

aanderse commented Feb 2, 2019

@timokau created a folder on my drive with a tab in the name, mounted something tot it, and ran nixos-generate-config.pl - the script picked up the new mount point and wrote out the proper entry.

@timokau
Copy link
Member

timokau commented Feb 3, 2019

Thanks! How do I test this? I mounted a btrfs subvolume with spaces and tabs in the subvolume name and the mount point name. I cloned your fork.

I tried to rebuild my system from it with NIX_PATH="nixpkgs=$PWD:$NIX_PATH sudo nixos-rebuild switch", but I'm not sure if the right nixos-generate-config is executed. Either way it doesn't show the subvolume.

I tried to execute the perl script directly, but that gives me

Can't locate File/Slurp.pm in @INC (you may need to install the File::Slurp module) (@INC contains: /nix/store/mbwav8kz8b3y471wjsybgzw84mrh4js9-perl-5.28.1/lib/perl5/site_perl/5.28.1/x86_64-linux-thread-multi /nix/store/mbwav8kz8b3y471wjsybgzw84mrh4js9-perl-5.28.1/lib/perl5/site_perl/5.28.1 /nix/store/mbwav8kz8b3y471wjsybgzw84mrh4js9-perl-5.28.1/lib/perl5/5.28.1/x86_64-linux-thread-multi /nix/store/mbwav8kz8b3y471wjsybgzw84mrh4js9-perl-5.28.1/lib/perl5/5.28.1) at nixos/modules/installer/tools/nixos-generate-config.pl line 8.
BEGIN failed--compilation aborted at nixos/modules/installer/tools/nixos-generate-config.pl line 8.

@aanderse
Copy link
Member Author

aanderse commented Feb 3, 2019

@timokau you can execute the script in a nix-shell with perl and perlPackages.FileSlurp

@timokau
Copy link
Member

timokau commented Feb 3, 2019 •
edited
Loading

That gives the same error. Even if I add perlPackages.FileSlurp to my environment.

@aanderse
Copy link
Member Author

aanderse commented Feb 3, 2019

That gives the same error. Even if I add perlPackages.FileSlurp to my environment.

@timokau This is how I run it:

$ nix-shell -p perl perlPackages.FileSlurp
[nix-shell:~/nixpkgs]$ perl ~/nixpkgs/nixos/modules/installer/tools/nixos-generate-config.pl --show-hardware-config

@timokau
Copy link
Member

timokau commented Feb 3, 2019

For some reason it didn't work with sudo, but it does with su. Weird.

Anyways, it works and picks up the subvolume with tabs and spaces :) Thanks for working on this!

@timokau timokau merged commit c01eeda into NixOS:master Feb 3, 2019
@aanderse aanderse deleted the nixos-generate-config branch February 3, 2019 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timokau timokau

@infinisil infinisil

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@aanderse @GrahamcOfBorg @nixos-discourse @timokau