Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
cmd/snap-update-ns: do not assume 'nogroup' exists #4191
Conversation
stolowski
requested changes
Nov 9, 2017
•
Hmm, by using root we're not properly testing XSnapGID, because it falls back to 'return 0, nil' if it doesn't match x-snapd.gid= prefix. In other words we may break XSnapGID and will not notice it.
Not sure what's the best strategy, perhaps the test should try both nogroup and nobody and see if one of them gives non-zero (that's a poor check too)...
bboozzoo
added some commits
Nov 3, 2017
|
@stolowski pushed commits addressing the issue you raised, can you re-review? |
| + // 'nobody' | ||
| + for _, grp := range []string{"nogroup", "nobody"} { | ||
| + nogroup = grp | ||
| + nogroupGID, _ = osutil.FindGid(nogroup) |
stolowski
Nov 10, 2017
•
Edited 1 time
-
stolowski
Nov 10, 2017
Contributor
-
Nov 10, 2017
You should break the loop when find succeeds I think?
| + // try to cover differences between distributions and find a suitable | ||
| + // 'nogroup' like group, eg. Ubuntu uses 'nogroup' while Arch uses | ||
| + // 'nobody' | ||
| + for _, grp := range []string{"nogroup", "nobody"} { |
stolowski
Nov 10, 2017
Contributor
Thank you for this change, I think that's the right thing to do and if we ever find a distro which has yet another name for nobody, we can add it here.
codecov-io
commented
Nov 13, 2017
Codecov Report
@@ Coverage Diff @@
## master #4191 +/- ##
=========================================
Coverage ? 75.56%
=========================================
Files ? 436
Lines ? 37814
Branches ? 0
=========================================
Hits ? 28574
Misses ? 7245
Partials ? 1995Continue to review full report at Codecov.
|
bboozzoo commentedNov 9, 2017
•
Edited 1 time
-
bboozzoo
Nov 9, 2017
Group 'nogroup' may not exist in given host system (eg. Arch). Although a
similar purpose group (eg. 'nobody') may be present, prefer using a hardcoded,
well know group name, such as 'root'.
Signed-off-by: Maciej Borzecki maciej.zenon.borzecki@canonical.com
Based on work in #4135 and standup discussions, the change is trying to reduce the number of places where we need actual group lookup calls.