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.

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

Add NixOS modules search logic into libkmod #17738

Merged
merged 6 commits into from Aug 15, 2016

Conversation

Projects
None yet
6 participants
@abbradar
Copy link
Member

commented Aug 14, 2016

Motivation for this change

NixOS needs special logic to search for kernel modules. We first want to check /run/current-system/kernel-modules and, if there are no modules for our kernel version there, fall back to /run/booted-system/kernel-modules. This way we both allow to enable new kernel packages without a reboot and make the system run consistently after a kernel update before new kernel was loaded.

Unfortunately, this logic previously has been implemented as a wrapper on top of kmod, namely system.sbin.modprobe. Therefore it was required that either it is inserted into PATH of an application which wants to use modprobe et all, or, if an application links to libkmod and uses it directly (or if we can't hijack PATH), to set MODULE_DIR to /run/current-system/kernel-modules or the other one. In this situation one can have either system consistency or ability to add new kernel packages, but not both.

This set of patches moves this logic into kmod itself. Therefore, we don't need MODULE_DIR nor system.sbin.modprobe at all anymore -- kmod and all applications using it would get all the nice things automatically.

As a side improvement, I've replaced sbin by bin in several places (this was done while I studied how kmod is used treewide).

As an example of nice user-side improvement, bumblebee now works correctly after a switch to a new kernel.

Things done
  • Tested using sandboxing
    (nix.useChroot on NixOS,
    or option build-use-chroot in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • 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.

@mention-bot

This comment has been minimized.

Copy link

commented Aug 14, 2016

@abbradar, thanks for your PR! By analyzing the annotation information on this pull request, we identified @edolstra, @shlevy and @dezgeg to be potential reviewers

@dezgeg

View changes

pkgs/tools/networking/network-manager/default.nix Outdated
@@ -19,7 +19,7 @@ stdenv.mkDerivation rec {
substituteInPlace configure --replace /usr/bin/uname ${coreutils}/bin/uname
substituteInPlace configure --replace /usr/bin/file ${file}/bin/file
substituteInPlace src/devices/nm-device.c --replace /usr/bin/ping ${inetutils}/bin/ping
substituteInPlace src/NetworkManagerUtils.c --replace /sbin/modprobe /run/current-system/sw/sbin/modprobe
substituteInPlace src/NetworkManagerUtils.c --replace /sbin/modprobe /run/current-system/sw/bin/modprobe

This comment has been minimized.

Copy link
@dezgeg

dezgeg Aug 15, 2016

Contributor

Maybe we should get rid of these /run/current-system/sw/bin/modprobe references at the same time, to make things consistent?

This comment has been minimized.

Copy link
@abbradar

abbradar Aug 15, 2016

Author Member

Done, thanks!

@abbradar abbradar changed the title Add NixOS modules search login into libkmod Add NixOS modules search logic into libkmod Aug 15, 2016

@abbradar abbradar force-pushed the abbradar:modprobe-fix branch Aug 15, 2016

@shlevy

This comment has been minimized.

Copy link
Member

commented Aug 15, 2016

I think a colon-separated MODULE_DIRS might be better here, and set the defaults in NixOS instead of nixpkgs. That might even be acceptable upstream eventually.

@abbradar

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2016

I've done a new patch that does just that (renamed the variable to MODULES_PATH to have PATH in name but this is up to discussion), but I'm not sure now where exactly do we want it set.

  1. We want it set for systemd itself in stage-1;
  2. We want it set for all sessions;
  3. We want it set for several daemons.

I very much don't like duplication of a long magical string "/run/current-system/kernel-modules/lib/modules:/run/booted-system/kernel-modules/lib/modules" everywhere. How should I proceed to at least minimize this duplication? Currently everyone sets it by themselves if needed (and inconsistently, too).

@dezgeg

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2016

I agree that the existing MODULE_DIRS environments were ugly and people will get it wrong/inconsistent again, so I'd prefer to avoid that.

@shlevy How about a compile-time default? E.g. --with-default-module-path=/run/current-system/kernel-modules/lib/modules:/run/booted-system/kernel-modules/lib/modules

@abbradar

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2016

Meanwhile it would be nice to have the patches reviewed: https://github.com/abbradar/kmod/commits/master

@abbradar

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2016

After some thoughts, I propose to take my old patch (with hardcoded array of paths and MODULE_DIR as an override mechanism), but remove NixOS directories from it. Try to upstream it (as it's generic) and also make a distro-specific patch to add our directories. This way we both (hopefully) upstream majority of the code and don't require people to set MODULE_DIR(S) everywhere. I want this instead of current patch with MODULE_DIRS because if we decide (and I argue that's a good idea after all) to compile our NixOS-specific paths in, then the ability to search several directories via environment variable is redundant.

@shlevy

This comment has been minimized.

Copy link
Member

commented Aug 15, 2016

Haven't had time to look through all of the recent comments yet, but just realized that instead of modifying kmod we could have the activation script create a directory with the union of /run/booted-system modules and /run/current-system modules and just always point kmod there, instead of having two directories to look at.

@abbradar

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2016

@shlevy I like this less than what I have proposed, that's why:

  1. I think we want to go away from setting MODULES_DIR everywhere -- it's error-prone and it's a constant after all. That means hardcoding it, and because it's really a constant it's a good idea in my opinion;
  2. Simultaneously, we want to save an ability of kmod nixpkgs package to work with /lib/modules on non-NixOS system;
  3. Therefore, we would need to implement the same logic in kmod -- try one path first, then if it's not found fall back to another (/lib/modules);
  4. When it's done it makes no sense to make a union of directories when kmod can already search them in order.
@shlevy

This comment has been minimized.

Copy link
Member

commented Aug 15, 2016

Fair enough. Will review the rest of the discussion later.

@abbradar abbradar force-pushed the abbradar:modprobe-fix branch Aug 15, 2016

@abbradar

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2016

Pushed updated patchset -- kmod now has two patches, one of which is upstreamable (squashed version of https://github.com/abbradar/kmod/commits/master). I'll propose them upstream after/if we agree that's the way to go.

@abbradar abbradar force-pushed the abbradar:modprobe-fix branch to b067b53 Aug 15, 2016

@abbradar

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2016

After a discussion and review from @shlevy (thank you again very much) I've updated this PR with a new patch. As a highlight, kmod now has --with-modulesdirs configure option which allows us to drop the distro-specific patch.

${pkgs.gptfdisk}/sbin/sgdisk -A 2:set:2 /dev/vda
${pkgs.gptfdisk}/sbin/sgdisk -h 2 /dev/vda
${pkgs.gptfdisk}/sbin/sgdisk -C /dev/vda
${pkgs.gptfdisk}/bin/sgdisk -G /dev/vda

This comment has been minimized.

Copy link
@shlevy

shlevy Aug 15, 2016

Member

Have you verified that gptfdisk and all the other packages here have linked sbin and bin?

This comment has been minimized.

Copy link
@abbradar

abbradar Aug 15, 2016

Author Member

Yeah, all is well.

@shlevy

This comment has been minimized.

Copy link
Member

commented Aug 15, 2016

Other than the one commend, looks good to me! Will merge after that.

@shlevy shlevy merged commit b067b53 into NixOS:staging Aug 15, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@gebner

This comment has been minimized.

Copy link
Member

commented Aug 24, 2016

Something went really, really wrong here. The revert commit has been merged to staging now, and reverted all of this PR, including other PRs such as #17495.

@gebner gebner referenced this pull request Aug 24, 2016

Merged

mesa: 11.2.2 -> 12.0.1 #17495

4 of 7 tasks complete
@shlevy

This comment has been minimized.

Copy link
Member

commented Aug 24, 2016

@gebner Please review #17957 to see if it brings back everything we need brought back

@obadz

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2016

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.