-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
nixos/wpa_supplicant: prefer 'install' over 'touch/chmod/mkdir/chgrp' #121395
Conversation
if test "$(stat --printf '%G' ${suppl.userControlled.socketDir})" != "${suppl.userControlled.group}"; then | ||
echo "ERROR: bad ownership on ${suppl.userControlled.socketDir}" >&2 | ||
exit 1 | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a downside to overwriting the group of an existin directory? Maybe we should keep this but put it before the install
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a downside to overwriting the group of an existin directory?
I don't know. I'm not really familiar with wpa_supplicant. I tested if ACLs are preserved with install
, and they are.
My thinking:
Old code: configure dir once and fail if group ownership differs later.
New code: configure dir always. (The code will only fail if there is insufficient permissions to configure things.)
I don't see any benefit of the old code. The new one automates something the old code would require user action to solve.
Thank you for your work, @bjornfor! On a superficial reading, your change looks good to me -- however, I currently don't have the time to think this through properly or to test this change. Sorry! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge in a couple of days if no one objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any problem with the new code, too. Let's merge this.
Motivation for this change
Ref #121293.
Untested.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)