-
Notifications
You must be signed in to change notification settings - Fork 290
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 bare-user-only mode that works on non-xattrs filesystems #750
Conversation
I guess it needs some tests too |
src/ostree/ot-builtin-commit.c
Outdated
@@ -84,6 +85,7 @@ static GOptionEntry options[] = { | |||
{ "add-detached-metadata-string", 0, 0, G_OPTION_ARG_STRING_ARRAY, &opt_detached_metadata_strings, "Add a key/value pair to detached metadata", "KEY=VALUE" }, | |||
{ "owner-uid", 0, 0, G_OPTION_ARG_INT, &opt_owner_uid, "Set file ownership user id", "UID" }, | |||
{ "owner-gid", 0, 0, G_OPTION_ARG_INT, &opt_owner_gid, "Set file ownership group id", "GID" }, | |||
{ "canonical-permissions", 0, 0, G_OPTION_ARG_NONE, &opt_canonical_permissions, "Canonicallize permissions in the same way bare-user does for hardlinked files", NULL }, |
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.
s/Canonicallize/Canonicalize
src/ostree/ot-builtin-commit.c
Outdated
@@ -196,6 +198,13 @@ commit_filter (OstreeRepo *self, | |||
g_hash_table_remove (mode_adds, path); | |||
} | |||
|
|||
if (opt_canonical_permissions && | |||
g_file_info_get_file_type (file_info) == G_FILE_TYPE_REGULAR) |
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.
Why isn't this just the default if writing to bare-user-only
though?
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.
It could be, but we might want to commit this to an archive-z2 repo and then pull it into a bare-user-only one.
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.
Yep, I understand the rationale for the option, but I'd still say it should be on by default for bare-user. Among other things, every API caller will need to duplicate this logic if it's not.
This all looks pretty good to me! One comment about the perms on commit; feels like if we do that it'd reduce the diff in the tests a lot too. I also wonder if we should basically force on |
Force -U, or fail without it? |
Ah, i see what you mean, if its automatic we get less testcase noise. |
I'd vote force And with both of these things, what I'm actually arguing is we do the defaulting not at the command line level, but at the libostree level. For checkout this should be pretty easy, check the mode in For commit it's likely to be trickier in the general case because we have |
To put this another way - I'd say we've succeeded if flatpak doesn't need any code changes to work with this new repo mode other than the parts that e.g. initialize the repo with the new mode. Right? |
Well, some of the ostree test cases commit to archive-z2 repos, which are then pulled to bare-user-only, so forcing it on commit to bare-user-only is not enough. |
Right, I agree we still need the canonical permissions argument. Along the API lines, what about making it |
flatpak doesn't technically need any changes (other than making repos of this type). However, it would be nice if flatpak build-export creates commits that are canonical so that fsck works on the flatpak local repo. However, build-export generally exports to archive-z2 repos, so any change we make to that will not affect much. The amount of changes needed are very small though. |
src/libostree/ostree-repo-commit.c
Outdated
g_file_info_get_file_type (file_info) == G_FILE_TYPE_REGULAR) | ||
{ | ||
guint current_mode = g_file_info_get_attribute_uint32 (modified_info, "unix::mode"); | ||
g_file_info_set_attribute_uint32 (modified_info, "unix::mode", current_mode | 0744); |
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.
Don't we also need to set uid/gid to zero?
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.
Its already possible to do this with the separate uid/gid options, so i made it only do what we couldn't already do.
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.
We could make this automatically mean uid/gid 0 and no-xatts too, but that means you can't do the indiviual operations separately
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.
If you're committing to a bare-user-only repo, those are going to be discarded.
I can't think of a scenario where one is committing to an archive repo and wants to use this flag, but doesn't also want the uid/gid/no-xattrs.
I get what you're saying about everything else being achievable, but the flip side is - if we're not aware of a use case, why add it?
(A small reason to go the flag = full canon route is that at some point I'd like to optimize commit so we don't malloc()
like there's no tomorrow, and part of that is going to be to stop allocating GFileInfo
in favor of a struct stat
on the stack. Which we can only do right now if we don't have a commit modifier)
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.
That said this is a "lean against" feeling - if you really prefer it the way it is now, given flatpak would be the primary user that's OK by me.
I made --canonical-permissions imply everything. |
I'm gonna skip modifying the commit deep internals to do this automatically for bare-user-only repos though. There is to much code that has to be modified for that, and its not the common usecase i imagine anyway. |
Re changing the commit core, agree that's probably not worth it right now. I have a little concern we're going to hit corner cases without doing it, but OTOH if the tests pass, I feel fairly confident. Maybe though when this shows up in the next release we mark it as experimental, until flatpak has been using it for a bit in its devel stream? |
This cleans up some existing code, but it also allows us to later add new bare modes. Closes: #750 Approved by: cgwalters
This mode is similar to bare-user, but does not store the permission, ownership (uid/gid) and xattrs in an xattr on the file objects in the repo. Additionally it stores symlinks as symlinks rather than as regular files+xattrs, like the bare mode. The later is needed because we can't store the is-symlink in the xattr. This means that some metadata is lost, such as the uid. When reading a repo like this we always report uid, gid as 0, and no xattrs, so unless this is true in the commit the resulting repository will not fsck correctly. However, it the main usecase of the repository is to check out with --user-mode, then no information is lost, and the repository can work on filesystems without xattrs (such as tmpfs). Closes: #750 Approved by: cgwalters
This adds to file permission masks the same bitmask that will be applied to file objects in bare-user* repos. This will be needed in the testsuite to ensure that the things we commit will be expressable in bare-user-only repos. Closes: #750 Approved by: cgwalters
This is somewhat complicated by such repos only properly supporting some subset of file metadata (uid/gid 0, etc). We fix this by always commiting with filters that make it work. Closes: #750 Approved by: cgwalters
Hm, actually it looks like some of these test failures are not flakes, e.g.:
|
I though that was some generic ci issue. Does other PRs not get it? |
With our Homu instance, every test there except Travis has to pass to be pushed. |
But how my changes cause enosupp? |
💔 Test failed - status-atomicjenkins |
Not sure yet; built your branch and |
All the tests work here, but its not only test-basic thats failing with ENOSUPP, even tests/test-sysroot-c |
☔ The latest upstream changes (presumably 455cc5e) made this pull request unmergeable. Please resolve the merge conflicts. |
This cleans up some existing code, but it also allows us to later add new bare modes.
This mode is similar to bare-user, but does not store the permission, ownership (uid/gid) and xattrs in an xattr on the file objects in the repo. Additionally it stores symlinks as symlinks rather than as regular files+xattrs, like the bare mode. The later is needed because we can't store the is-symlink in the xattr. This means that some metadata is lost, such as the uid. When reading a repo like this we always report uid, gid as 0, and no xattrs, so unless this is true in the commit the resulting repository will not fsck correctly. However, it the main usecase of the repository is to check out with --user-mode, then no information is lost, and the repository can work on filesystems without xattrs (such as tmpfs).
This adds to file permission masks the same bitmask that will be applied to file objects in bare-user* repos. This will be needed in the testsuite to ensure that the things we commit will be expressable in bare-user-only repos.
This is somewhat complicated by such repos only properly supporting some subset of file metadata (uid/gid 0, etc). We fix this by always commiting with filters that make it work.
6008b97
to
ae279d0
Compare
I applied the fixups and rebased to master, fixing the (trivial) conflicts. |
So, i really want to start using this for the flatpak runtime builds, so i'm ack:ing this again (since colin already ack:ed it above). Hope that is ok. |
@alexlarsson: 🔑 Insufficient privileges: Collaborator required |
This mode is similar to bare-user, but does not store the permission, ownership (uid/gid) and xattrs in an xattr on the file objects in the repo. Additionally it stores symlinks as symlinks rather than as regular files+xattrs, like the bare mode. The later is needed because we can't store the is-symlink in the xattr. This means that some metadata is lost, such as the uid. When reading a repo like this we always report uid, gid as 0, and no xattrs, so unless this is true in the commit the resulting repository will not fsck correctly. However, it the main usecase of the repository is to check out with --user-mode, then no information is lost, and the repository can work on filesystems without xattrs (such as tmpfs). Closes: #750 Approved by: cgwalters
This adds to file permission masks the same bitmask that will be applied to file objects in bare-user* repos. This will be needed in the testsuite to ensure that the things we commit will be expressable in bare-user-only repos. Closes: #750 Approved by: cgwalters
This is somewhat complicated by such repos only properly supporting some subset of file metadata (uid/gid 0, etc). We fix this by always commiting with filters that make it work. Closes: #750 Approved by: cgwalters
☀️ Test successful - status-atomicjenkins |
This mode is similar to bare-user, but does not store the permission,
ownership (uid/gid) and xattrs in an xattr on the file objects in the
repo. Additionally it stores symlinks as symlinks rather than as
regular files+xattrs, like the bare mode. The later is needed because
we can't store the is-symlink in the xattr.
This means that some metadata is lost, such as the uid. When reading a
repo like this we always report uid, gid as 0, and no xattrs, so
unless this is true in the commit the resulting repository will
not fsck correctly.
However, it the main usecase of the repository is to check out with
--user-mode, then no information is lost, and the repository can
work on filesystems without xattrs (such as tmpfs).