lib/commit: autofix permissions for bare-user-only #2412
Conversation
lucab
force-pushed
the
ups/lib-commit-canonicalize
branch
from
8dda504 to
620bbc1
Compare
|
Obviously it would be good to add a test that committing to a bare-user-only repo without Also, are you planning to handle |
|
@dbnicholson I'd like to scope this to the uid/gid/mode part only, and tackle xattrs separately. Thanks for your other suggestions, I'll incorporate them. Right now though this is a WIP as I'm blocked tracking down the non-identical glitch I'm seeing, which for some reason does not happen in CI (possibly because it runs as root in a container). |
lucab
force-pushed
the
ups/lib-commit-canonicalize
branch
from
620bbc1 to
f5f8014
Compare
lucab
changed the title
|
Rebased, this is now ready to land. |
src/libostree/ostree-repo.h
Outdated
| @@ -678,10 +678,14 @@ typedef OstreeRepoCommitFilterResult (*OstreeRepoCommitFilter) (OstreeRepo *r | |||
| * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_NONE: No special flags | |||
| * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_SKIP_XATTRS: Do not process extended attributes | |||
| * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_GENERATE_SIZES: Generate size information. | |||
| * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS: Canonicalize permissions for bare-user-only mode. | |||
| * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS: Canonicalize permissions, for bare-user-only mode. | |||
There was a problem hiding this comment.
Should we just simplify this to
| * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS: Canonicalize permissions, for bare-user-only mode. | |
| * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS: Canonicalize permissions |
?
Since the only reason you would pass this explicitly now is if you want the behaviour on non-bare-user-only repos.
tests/test-basic-user-only.sh
Outdated
| ostree_repo_init repo init --mode=bare-user-only | ||
| rm files -rf && mkdir files | ||
| echo afile > files/afile | ||
| $OSTREE commit --no-xattrs -b perms files |
There was a problem hiding this comment.
We should be able to drop --canonical-permissions from COMMIT_ARGS and use COMMIT_ARGS here too, no?
There was a problem hiding this comment.
Yes! And... it uncovered another bug 😞
It looks like file adoption (commit --consume) is also broken for bare-user-only.
src/libostree/ostree-repo-commit.c
Outdated
| (modifier->filter == NULL && | ||
| (modifier->flags & OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS) == 0)) | ||
| /* Auto-detect bare-user-only repo, force canonical permissions. */ | ||
| canonicalize_perms |= (self->mode == OSTREE_REPO_MODE_BARE_USER_ONLY); |
There was a problem hiding this comment.
Minor: this is OK, though to be consistent with the rest of the codebase might be worth avoiding bitwise ops for these pseudo-booleans.
There was a problem hiding this comment.
How would you prefer to write this? if (mode is user-only) { canonicalize_perms = TRUE }?
There was a problem hiding this comment.
Yup, exactly! With a newline and less braces. :)
| } | ||
|
|
||
| modified_info = g_file_info_dup (file_info); | ||
| if (modifier->filter) | ||
|
|
||
| if (has_filter) | ||
| result = modifier->filter (self, path, modified_info, modifier->user_data); |
There was a problem hiding this comment.
Aside: hmm, we should be able to just return early here if the filter returned OSTREE_REPO_COMMIT_FILTER_SKIP. Need to double-check, maybe there's a subtle reason we still need to modify the GFileInfo.
lucab
force-pushed
the
ups/lib-commit-canonicalize
branch
from
f5f8014 to
132adfc
Compare
This tweaks commit logic to detect bare-user-only repositories and canonicalize permissions automatically.
lucab
force-pushed
the
ups/lib-commit-canonicalize
branch
from
132adfc to
8a5241d
Compare
|
Rebased once more. |
|
OK I can't run fixes one bug, but there are more. |
|
I did test each individual PR locally plus the current tip of |
This tweaks commit logic to detect bare-user-only repositories and
canonicalize permissions automatically.