Skip to content
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

nixos/bcache: install udev rules outside initrd too #26344

Closed
wants to merge 3 commits into from
Closed

nixos/bcache: install udev rules outside initrd too #26344

wants to merge 3 commits into from

Conversation

bjornfor
Copy link
Contributor

@bjornfor bjornfor commented Jun 3, 2017 •
edited

Motivation for this change

Should fix #26281 ("Bcache udev rules not installed outside initrd").

(Apparently I cannot verify that these rules are working by generating symlinks to bcache devices in /dev/disk/by-*, because on my system I run LVM on top of bcache.)

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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/)
  • Fits CONTRIBUTING.md.

@bjornfor
bcache-tools: add name to the source store path
7b79889 
So that

 $ nix-build -A bcache-tools.src

gives

 /nix/store/HASH-bcache-tools-1.0.7.tar.gz

instead of

 /nix/store/HASH-v1.0.7.tar.gz
@bjornfor
nixos/udev: replace /bin/sh with ${bash}/bin/sh
654f756 
Needed for bcache-tools.

Mind the '"' which will prevent bad substitution in the future in case
bcache-tools gets patched to ${bash}/bin/sh instead of /bin/sh.

Note that this substitution rule is already implemented in the handling
of udev rules that goes into the initrd (stage-1.nix).
@mention-bot
Copy link

@bjornfor, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @abbradar and @urkud to be potential reviewers.

@bjornfor
nixos/bcache: add services.udev.packages = [ bcache-tools ]
045066d 
Adds /dev/disk/by-{id,label}/* symlinks for bcache device nodes, in the
final rootfs.

Symlinks will only be created for bcache devices with a filesystem.
So if you have a blank bcache device or run LVM on top of bcache you
will not get this kind of symlink.
@bjornfor bjornfor mentioned this pull request Jun 3, 2017
Closed
@bjornfor bjornfor requested a review from abbradar June 3, 2017 08:39
@bjornfor
Copy link
Contributor Author

bjornfor commented Jun 3, 2017

I rebuilt and rebooted my system with this. No harm done.

abbradar
abbradar approved these changes Jun 3, 2017
View reviewed changes
Copy link
Member

@abbradar abbradar left a comment •
edited

Choose a reason for hiding this comment

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

Seems all good to me!

I still wonder why do we need to replace /bin/sh -- shouldn't it work given that /bin/sh is installed and I don't think udev isolates itself in a chroot/mount namespace? Still, less sources of impurity is anyway better.

@bjornfor
Copy link
Contributor Author

bjornfor commented Jun 3, 2017

Good question. Maybe @dfoxfranke can answer, because it seems that was tested:

@dfoxfranke wrote:
Adding bcache-tools to services.udev.packages doesn't quite suffice as a workaround
because of a hardcoded reference to /bin/sh. Instead I had to copy the contents of
69-bcache.rules into services.udev.extraRules and replace /bin/sh with ${pkgs.stdenv.shell}.```

@dfoxfranke
Copy link
Contributor

dfoxfranke commented Jun 3, 2017 •
edited

@abbradar @bjornfor See bjornfor@2230f64#commitcomment-22384001

@abbradar
Copy link
Member

abbradar commented Jun 3, 2017

Argh, I definitely need more sleep - I have read this and acknowledged in my head but still asked the question again. Thanks!

@bjornfor
Copy link
Contributor Author

bjornfor commented Jun 3, 2017

Looking at 775f381 it is clear the /bin/sh substitute rule was added for bcache. I think I'll close this PR and open a new one from my old branch: master...bjornfor:bcache-udev-rules

And if the changes in the above branch does create a regression, it will be at build time (AFAICT), not runtime (when booting).

@bjornfor bjornfor mentioned this pull request Jun 3, 2017
Merged
7 tasks
@bjornfor
Copy link
Contributor Author

bjornfor commented Jun 3, 2017

Closing in favor of the new PR: #26354.

@bjornfor bjornfor closed this Jun 3, 2017
@bjornfor bjornfor deleted the bcache-udev-rules-v2 branch June 10, 2017 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abbradar abbradar abbradar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bcache udev rules not installed outside initrd
4 participants
@bjornfor @mention-bot @dfoxfranke @abbradar