GNOME Bugzilla – Bug 741125
Support usermode bare repos
Last modified: 2014-12-08 19:48:21 UTC
I'd like to have support for a usermode (i.e. all files owned by the user) bare repo that support -U checkouts that can hardlink into the repo. This would be useful in general, and in particular for my work on gnome apps (to allow user-installed apps).
Created attachment 292143 [details] [review] checkout: fchown symlink to proper uid/gid When commiting a symlink we do store the uid/gid of the actual symlink (i.e. not target). However, this was not restored on non-user-mode checkout as it should. This commit fixes that, and additionally it ensures xattrs on symlinks are not set in user-mode checkout.
Created attachment 292144 [details] [review] Add ot_lgetxattrat and ot_lsetxattrat utils These are implementation of the missing corresponding syscalls that are done with the /proc/self/fd mechanism described at: https://mail.gnome.org/archives/ostree-list/2014-February/msg00017.html
Created attachment 292145 [details] [review] Support for "bare-user" repo format This format is pretty much the same as the "bare" format, except the file ownership and xattrs is not stored in the actual filesystem object, but rather on the side in a user xattr. This means two things: 1) An unprivileged user can store such a repo independent of the types of files in it or their xattrs. And you can later (as root) reconstruct the real filesystem tree with ownership. Although you can't do that using hardlink-sharing. This also means ostree fsck does a full verification. 2) Such a repository can be checked out with user-mode (checkout -U) as an unprivileged user using hardlinks for space sharing. Additionally, symlinks are stored as regular files (with the content being the symlink target) because user xattrs are not supported on symlinks. We know at checkout time if the file is a symlink because the original st_mode is stored in the xattr metadata.
Review of attachment 292143 [details] [review]: Oops, good catch! I guess it's rather rare in OS content to have non-root-owned symlinks at least.
Comment on attachment 292143 [details] [review] checkout: fchown symlink to proper uid/gid Attachment 292143 [details] pushed as bb82c17 - checkout: fchown symlink to proper uid/gid
Review of attachment 292145 [details] [review]: Given that this code runs as non-root, I'd really like to have some test cases for it. You could just fork test-basic.sh and replicate a few key test items from it. It would be more work to abstract it so it could run on both bare and bare-user, but if you want to do that, look at how test-admin-deploy-{syslinux,uboot} source admin-test.sh. ::: src/libostree/ostree-repo-commit.c @@ +76,3 @@ + g_variant_ref_sink (ret_metadata); + + return ret_metadata; Any reason for extra local variable instead of just: return g_variant_ref_sink (g_variant_new (...)) ? Doesn't matter much though. @@ +144,3 @@ + * + * Note, this does not apply for bare-user repos, as they store symlinks + * as regulare files. regular @@ +203,3 @@ + } + + if (objtype == OSTREE_OBJECT_TYPE_FILE && self->mode == OSTREE_REPO_MODE_BARE_USER) I'm wondering how this doesn't trigger a bug for below, looks like we're writing the xattr metadata for symlinks. @@ +513,3 @@ + char *target = g_strdup (g_file_info_get_symlink_target (file_info)); + /* For bare-user we can't store symlinks as symlinks, as symlinks don't + support user xattrs to store the ownership. So, instead store them Hmmm, really? That seems odd. Ok, you're clearly right, from linux/fs/xattr.c: /* * In the user.* namespace, only regular files and directories can have * extended attributes. For sticky directories, only the owner and * privileged users can write attributes. */ But...why? Oh well. Ok, so, we're just writing the symlink target to the file as a non-NUL terminated string. But hmm...won't this confuse some of the "is_symlink" logic elsewhere? @@ +549,3 @@ { if (!_ostree_make_temporary_symlink_at (self->tmp_dir_fd, g_file_info_get_symlink_target (file_info), Like this, right? Won't we be making a symlink in BARE_USER when we need to make a regular file? ::: src/libostree/ostree-repo.c @@ +1418,3 @@ + g_file_info_set_attribute_uint32 (info, "unix::gid", gid); + g_file_info_set_attribute_uint32 (info, "unix::mode", mode); + ot_transfer_out_value (out_xattrs, &ret_xattrs); Why not just actually return the value? The ot_transfer_out_value() macro is mainly useful for functions which can return an error. @@ +1533,3 @@ + goto out; + targetbuf[target_size] = '\0'; + g_file_info_set_symlink_target (ret_file_info, targetbuf); If we defined the file format to have the trailing NUL, then we could just mmap() it. (This would matter more if we had pack files). But obviously not a big deal.
Review of attachment 292144 [details] [review]: ::: src/libotutil/ot-fs-utils.c @@ +55,3 @@ + const char *path, + const char *attribute, + GError **error) Hm, duplicating the code in libgsystem =/ Well...I'm ok with this now, but at some point I'd like to do a "feed ostree bits into libgsystem" push.
Review of attachment 292145 [details] [review]: ::: src/libostree/ostree-repo-commit.c @@ +203,3 @@ + } + + if (objtype == OSTREE_OBJECT_TYPE_FILE && self->mode == OSTREE_REPO_MODE_BARE_USER) The reason it works is that before calling this, in write_object we detected this case (is-symlink + bare-user) and create a regular file instead of a symlink. @@ +513,3 @@ + char *target = g_strdup (g_file_info_get_symlink_target (file_info)); + /* For bare-user we can't store symlinks as symlinks, as symlinks don't + support user xattrs to store the ownership. So, instead store them No, all the code atm handles this, but I agree that its not necessarily obvious what is going on in the code. @@ +549,3 @@ { if (!_ostree_make_temporary_symlink_at (self->tmp_dir_fd, g_file_info_get_symlink_target (file_info), No. is_symlink is TRUE, yes, but at this point so is temp_file_is_regular. So, due to the "else" part here we're doing the right thing. But, I'll try to come up with some better naming here to make this more obvious. ::: src/libostree/ostree-repo.c @@ +1533,3 @@ + goto out; + targetbuf[target_size] = '\0'; + g_file_info_set_symlink_target (ret_file_info, targetbuf); I don't think in this paticular case that mmap is a great idea (vm shenigans are rarely effective for small things), but you're right that its safer and better all-around to embedd the nul in the file.
Created attachment 292185 [details] [review] union checkout: Fix symlink handling for xattrs Applying xattrs on a symlink during checkout failed since it was setting the xattrs on the final filename, not the temporary name. This made the "checkout union 1" test in test-basic.sh fail.
Created attachment 292186 [details] [review] Support for "bare-user" repo format This format is pretty much the same as the "bare" format, except the file ownership and xattrs is not stored in the actual filesystem object, but rather on the side in a user xattr. This means two things: 1) An unprivileged user can store such a repo independent of the types of files in it or their xattrs. And you can later (as root) reconstruct the real filesystem tree with ownership. Although you can't do that using hardlink-sharing. This also means ostree fsck does a full verification. 2) Such a repository can be checked out with user-mode (checkout -U) as an unprivileged user using hardlinks for space sharing. Additionally, symlinks are stored as regular files (with the content being the symlink target) because user xattrs are not supported on symlinks. We know at checkout time if the file is a symlink because the original st_mode is stored in the xattr metadata.
Created attachment 292187 [details] [review] Split out basic tests from test-basic.sh This will let us reuse them with other repo types
Created attachment 292188 [details] [review] Add test-basic-user.sh testing for bare-user repos This just does whatever test-basic.sh does, but on a bare-user repo. This works standalone, but unfortunately it breaks in gnome-desktop-testing-runner as /tmp doesn't support xattrs.
Updated the basic patch with your feedback. Added a new bugfix and a test. Unfortunately the test doesn't work with the test runner since /tmp on tmpfs doesn't support xattrs.
Review of attachment 292185 [details] [review]: Ok, and this doesn't fail with the core ostree admin use case because we don't do unioning checkouts. Looks good.
Comment on attachment 292185 [details] [review] union checkout: Fix symlink handling for xattrs Attachment 292185 [details] pushed as 22ed7d0 - union checkout: Fix symlink handling for xattrs
Created attachment 292192 [details] [review] Add test-basic-user.sh testing for bare-user repos This just does whatever test-basic.sh does, but on a bare-user repo. This works standalone, but unfortunately it breaks in gnome-desktop-testing-runner as /tmp doesn't support xattrs, so it is not installed atm.
Review of attachment 292186 [details] [review]: Ok to commit after the critical fixes plus whichever optional changes you choose to make. ::: src/libostree/ostree-repo-commit.c @@ +512,3 @@ + as regular files */ + temp_file_is_regular = TRUE; + temp_file_is_symlink = FALSE; I find the dual boolean approach much more readable here, thanks! @@ +514,3 @@ + temp_file_is_symlink = FALSE; + if (file_input != NULL) + g_object_unref (file_input); I'm wondering whether it would be better to g_assert (file_input == NULL). Eh, that'd probably require changing other places. No big deal, let's keep this. @@ +517,3 @@ + + /* Include the terminating zero so we can e.g. mmap this file */ + file_input = g_memory_input_stream_new_from_data (target, strlen (target) + 1, g_free); I think this would be a little cleaner using GBytes. const char *target_str = g_file_info_get_symlink_target (file_info); GBytes *target = g_bytes_new (target_str, strlen (str) + 1); ... file_input = g_memory_input_stream_new_from_bytes (target); @@ +551,3 @@ goto out; } + else if ((repo_mode == OSTREE_REPO_MODE_BARE || repo_mode == OSTREE_REPO_MODE_BARE_USER) && temp_file_is_symlink) But the BARE_USER + temp_file_is_symlink case can't happen, right? Why change this? This should be covered by the else { g_assert_not_reached (); } right? ::: src/libostree/ostree-repo.c @@ +1521,3 @@ + g_file_info_set_file_type (ret_file_info, G_FILE_TYPE_SYMBOLIC_LINK); + g_file_info_set_size (ret_file_info, 0); + g_file_info_set_symlink_target (ret_file_info, targetbuf); Copy&paste error? This line is setting the target to garbage... @@ +1529,3 @@ + target_input = g_unix_input_stream_new (fd, TRUE); + + if (!g_input_stream_read_all (target_input, targetbuf, sizeof (targetbuf) - 1, I think there's a boundary condition here - if the symlink target is exactly PATH_MAX, we'll be truncating it, right? Shouldn't this be sizeof (targetbuf) ? @@ +1546,3 @@ + if (!gs_file_get_all_xattrs (full_path, &ret_xattrs, + cancellable, error)) + goto out; Hmm, we should switch this to use the *at() method. But I see you're just moving this code, would be a separate patch.
Review of attachment 292187 [details] [review]: Right.
Review of attachment 292192 [details] [review]: Yep, looks good.
BTW, a new test case you could add would be doing a pull-local from an archive-z2 to bare-user, and vice versa. That would verify the full-cycle nature.
Now that we have this, I think we could consider deprecating the uncompressed-object-cache approach. For gnome-continuous, we would have two ostree repositories; a bare-user one that's used for the build system, and then on commit we pull-local the content we want for export.
Review of attachment 292186 [details] [review]: ::: src/libostree/ostree-repo-commit.c @@ +517,3 @@ + + /* Include the terminating zero so we can e.g. mmap this file */ + file_input = g_memory_input_stream_new_from_data (target, strlen (target) + 1, g_free); did this @@ +551,3 @@ goto out; } + else if ((repo_mode == OSTREE_REPO_MODE_BARE || repo_mode == OSTREE_REPO_MODE_BARE_USER) && temp_file_is_symlink) Yeah, just a leftover ::: src/libostree/ostree-repo.c @@ +1521,3 @@ + g_file_info_set_file_type (ret_file_info, G_FILE_TYPE_SYMBOLIC_LINK); + g_file_info_set_size (ret_file_info, 0); + g_file_info_set_symlink_target (ret_file_info, targetbuf); yeah. @@ +1529,3 @@ + target_input = g_unix_input_stream_new (fd, TRUE); + + if (!g_input_stream_read_all (target_input, targetbuf, sizeof (targetbuf) - 1, yeah
Created attachment 292293 [details] [review] Add local-pull archive-z2 <=> bare-user roundtrip test This creates a archive-z2 repo, pull-locals it to bare-user and then again back to archive-z2 making sure things fsck along the way. Then it checks out all repos and makes sure each one reproduces the same result. Unfortunately we can't install this as a real test because it doesn't work in the test-runner because tmpfs doesn't support user xattrs.
Attachment 292144 [details] pushed as 26a47b9 - Add ot_lgetxattrat and ot_lsetxattrat utils Attachment 292186 [details] pushed as 47c612e - Support for "bare-user" repo format Attachment 292187 [details] pushed as a342279 - Split out basic tests from test-basic.sh Attachment 292192 [details] pushed as fcd3caf - Add test-basic-user.sh testing for bare-user repos Attachment 292293 [details] pushed as dbf717a - Add local-pull archive-z2 <=> bare-user roundtrip test
Review of attachment 292293 [details] [review]: ::: tests/test-local-pull.sh @@ +53,3 @@ +find checkout3 -printf '%P %s %#m %u/%g %y %l\n' > checkout3.files + +cmp checkout1.files checkout2.files One thing was bugging me about this; does "find" sort the listing not? I'm not finding useful information about this online, and it's not in the man page/info as far as I can tell. Some google searching implies it doesn't: http://unix.stackexchange.com/questions/34325/sorting-the-output-of-find And empirically it doesn't appear to. So while I definitely like the approach of using find for verification, it seems like it might break down the line unless we sort.
True, but a simple pipe through sort should be good enough.
Created attachment 292321 [details] [review] test-local-pull: Sort find output to make test robust There is no guarantee that find will produce output in the same order, so we need to sort the output to ensure we always get the same output.
Comment on attachment 292321 [details] [review] test-local-pull: Sort find output to make test robust Attachment 292321 [details] pushed as e908ebd - test-local-pull: Sort find output to make test robust
Cool, thanks for this work!