In ostree_repo_abort_transaction, if we pass a cancellable and it gets
canceled, then the function may fail to fully clean up the transaction
state. This was happening e.g. when the ostree_repo_pull_with_options
call got cancelled.
To fix this, as suggested by Colin Walters, we set the passed
cancellable as NULL, in order for it to be ignored.
https://github.com/ostreedev/ostree/issues/1491Closes: #1492
Approved by: jlebon
When a new object is added to the repository, create a
$PAYLOAD-SHA256.payload-link symlink file as well. The target of the
symlink is the checksum of the object that was added the repository.
Whenever we add a new object file, in addition to lookup if the file is
already present with the same checksum we also check if an object with
the same payload is in the repository.
If a file with the same payload is already present in the repository, we
copy it with `glnx_regfile_copy_bytes` that internally attempts to
create a reflink (ioctl (..., FICLONE, ..)) to the target file if the
file system supports it. This enables to have objects that share the
payload but have a different inode and xattrs.
By default the payload-link-threshold value is G_MAXUINT64 that disables
the feature.
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Closes: #1443
Approved by: cgwalters
SPDX License List is a list of (common) open source
licenses that can be referred to by a “short identifier”.
It has several advantages compared to the common "license header texts"
usually found in source files.
Some of the advantages:
* It is precise; there is no ambiguity due to variations in license header
text
* It is language neutral
* It is easy to machine process
* It is concise
* It is simple and can be used without much cost in interpreted
environments like java Script, etc.
* An SPDX license identifier is immutable.
* It provides simple guidance for developers who want to make sure the
license for their code is respected
See http://spdx.org for further reading.
Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Closes: #1439
Approved by: cgwalters
Previously we were doing e.g. `ot_util_filename_validate()` specifically inline
in dirtree objects, but only *after* writing them into the staging directory (by
default). In (non-default) cases such as not using a transaction, such an object
could be written directly into the repo.
A notable gap here is that `pull-local --untrusted` was *not* doing
this verification, just checksums. We harden that (and also the
static delta writing path, really *everything* that calls
`ostree_repo_write_metadata()` to also do "structure" validation
which includes path traversal checks. Basically, let's try hard
to avoid having badly structured objects even in the repo.
One thing that sucks in this patch is that we need to allocate a "bounce buffer"
for metadata in the static delta path, because GVariant imposes alignment
requirements, which I screwed up and didn't fulfill when designing deltas. It
actually didn't matter before because we weren't parsing them, but now we are.
In theory we could check alignment but ...eh, not worth it, at least not until
we change the delta compiler to emit aligned metadata which actually may be
quite tricky. (Big picture I doubt this really matters much right now
but I'm not going to pull out a profiler yet for this)
The pull test was extended to check we didn't even write a dirtree
with path traversal into the staging directory.
There's a bit of code motion in extracting
`_ostree_validate_structureof_metadata()` from `fsck_metadata_object()`.
Then `_ostree_verify_metadata_object()` builds on that to do checksum
verification too.
Closes: #1412
Approved by: jlebon
For the [rpm-ostree jigdo ♲📦](https://github.com/projectatomic/rpm-ostree/issues/1081) work.
We're basically doing "pull" via a non-libostree mechanism, and this
should be fully supported. As I mentioned earlier we should try to
have `ostree-repo-pull.c` only use public APIs; this gets us closer
to that.
Closes: #1376
Approved by: jlebon
In the non-`CONSUME` path for regfiles (which happens currently for
`bare-user`), we go to a lot of contortions to make an "object stream",
only to immediately parse it again.
Fixing this will also enable the `G_IS_FILE_DESCRIPTOR_BASED()` fast path in
commit, since the input stream will actually reference the file descriptor and
not be an `_OstreeChainInputStream`.
There's a slight concern here in that we're no longer checksumming *literally*
the object stream passed in for the stream case, but I mention in the comment,
the data should be the same, and if it's not somehow we're not adding risk,
since the checksum is still covering the data we actually care about.
Prep for further changes to break up the `write_content_object()` path into
separate paths for archive, as well as regfile vs symlink in non-archive.
Closes: #1371
Approved by: jlebon
One major thing we can do to speed up local commits is multithreading. In
preparation for that, split up the recursion function so that the subdirectory
case is separate from the content (regfile/symlink) case. Then for non-subdirs,
we can easily peel off worker threads and gather the final checksums and update
the mtree from the main thread.
The diff here looks large but it's pretty straightforward; amazingly this change
compiled the very first time I tried it!
Closes: #1365
Approved by: jlebon
I was getting a bare `error: Creating temp file: No such file or directory` when
debugging `test-concurrency.py`; with this I get
`error: Writing content object: Creating temp file: No such file or directory`
which helps me pin it down.
Closes: #1343
Approved by: cgwalters
For rpm-ostree I'd like to do importing in parallel with threads; the code is
*almost* ready for that except today it calls
`ostree_repo_transaction_set_ref()`.
Looking at the code, there's really a "transaction" struct here,
not just stats. Let's lift that struct out, and move the refs
into it under the existing lock.
Clarify the documentation around multithreading for various functions.
Closes: #1358
Approved by: jlebon
I was seeing the `Writing OSTree commit...` phase of rpm-ostree
being very slow lately. This turns out to be more fallout from
https://github.com/ostreedev/ostree/pull/1170
AKA commit: 8fe4536
Loading the xattrs is slow on my system (F27AW, XFS+LVM, NVMe). I haven't fully
traced through why, but AIUI at least on XFS the xattrs are often stored outside
of the inode so it's a little bit like doing an `open()+read()`. Plus there's
the LSM overhead, etc.
The thing is that for rpm-ostree's package layering use case, we
basically always want to treat the on-disk state as canonical. (There's
a subtle case here if one does overrides for something that contains
policy but we'll fix that).
Anyways, so we're in a state now where we do the slow but correct thing by
default, which seems sane. But let's allow the app to opt-in to telling us
"really trust devino". The difference between a `stat()` + hash table lookup
versus the full xattr load on my test case of `rpm-ostree install
./tree-1.7.0-10.fc27.x86_64.rpm` is absolutely dramatic; consistently on the
order of 10s without this support, and <1s with (800ms).
Closes: #1357
Approved by: jlebon
Previously we'd delete the tmpdir in `rename_pending_loose_objects()`
but do the unlock inside `ostree_repo_commit_transaction()`. Move
them into the same place in the latter function for consistency.
Doesn't fix anything, just a cleanup while reading the code and
working on `test-concurrency.py`.
Closes: #1352
Approved by: dbnicholson
Prep for future work here; let's cleanly separate the path for cleaning up the
txn staging directories from the code that cleans up "other stuff". Currently
only the former case uses the `GLnxLockFile` etc.
Closes: #1352
Approved by: dbnicholson
This closes a race condition I was seeing with `test-concurrency.py`. If we
don't have `O_TMPFILE` (or for symlinks) we'll create temporary files;
previously these would be subject to the date-based pruning because we set the
timestamp to 0 for objects.
Having our temporary files also in the txn staging dir ensures that they're
covered by the locking we do for that directory, and it's also generally cleaner
since the lifecycle of all the temporary data for a txn is in one place.
Closes: #1352
Approved by: dbnicholson
This lowers into the commit core what the static delta code
was doing, and improves the API.
The bigger picture issue is that for writing large files, our current "pull" API
where the caller provides a `GInputStream` is very awkward in some scenarios.
For example, we have a whole "libarchive input stream" that is a ~200 line
GObject that boils down to wrapping `archive_read_data()`.
This came more to a head when I was working on rpm-ostree jigdo since I had to
copy that object.
One step we can take after this is to further split `write_content_object()`
into a "write symlink or archive object" versus "write bare content object"
(it already has a mess of conditionals) and teach the latter case to call
this.
The eventual goal here is to make this API public.
Closes: #1355
Approved by: jlebon
For situations where fsync is disabled, there's basically
no reason to do the whole "staging directory" dance. Just
write directly into the repo.
Today I use `fsync=false` for my build/cache repos.
I briefly considered not allocating a tmpdir at all
in this case, but we actually do want the txn tmpdir
for the non-`O_TMPFILE` case.
Part of https://github.com/ostreedev/ostree/issues/1184Closes: #1354
Approved by: giuseppe
This is more subtle fallout from:
https://github.com/ostreedev/ostree/pull/1170
AKA commit: 8fe4536257
Before, if we found a devino cache hit, we'd use it unconditionally.
Recall that `bare-user` repositories are very special in that they're the only
mode where the on disk state ("physical state") is not the "real" state. The
latter is stored in the `user.ostreemeta` xattr. (`bare-user` repos are also
highly special in that symlinks are regular files physically, but that's not
immediately relevant here).
Since we now have `bare-user-only` for the "pure unprivileged container" case,
`bare-user` should just be used for "OS builds" which have nonzero uids (and
possibly SELinux labels etc.)
In an experimental tool I'm writing "skopeo2ostree" which imports OCI images
into refs, then squashes them together into a single final commit, we lost the
the `81` group ID for `/usr/libexec/dbus-1/dbus-daemon-launch-helper`.
This happened because the commit code was loading the "physical" disk state,
where the uid/gid are zero because that's the uid I happened to be using. We
didn't just directly do the link speedup because I was using `--selinux-policy`
which caused the xattrs to change, which caused us to re-commit objects from the
physical state.
The unit test I added actually doesn't quite trigger this, but I left
it because "why not". Really testing this requires the installed test
which uses SELinux policy from `/`.
The behavior without this fix looks like:
```
-00755 0 0 12 { [(b'user.ostreemeta', [byte 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x51, 0x00, 0x00, 0x81, 0xed]), (b'security.selinux', b'system_u:object_r:lib_t:s0')] } /usr/lib/dbus-daemon-helper
```
which was obviously totally broken - we shouldn't be picking up the
`user.ostreemeta` xattr and actually committing it of course.
Closes: #1297
Approved by: jlebon
I was trying to use this with pygobject for an OCI+ostree project, and pygobject
rejected simply assigning to the field (understandably, since it can't bind the
lifetime together).
Add a wrapper function, which is still unsafe, but hides that unsafety
where most people shouldn't find it. And if they do...well, sorry,
Rust wasn't invented when ostree was started.
Closes: #1295
Approved by: pwithnall
The way `_ostree_repo_open_content_bare()` did both looking for the object and
possibly creating a new fd was just weird and inconsistent with e.g. the pull
code where we always call `has_object()` first.
Just call `has_object()` in the delta paths that used this too, making the
implementation right now a thin wrapper around
`glnx_open_tmpfile_linkable_at()`, but this is prep for a later patch which does
more.
Closes: #1283
Approved by: jlebon
A side effect of commit 8fe4536257 is that
we started listing all xattrs even for files with device/inode matches;
further, we did that using the dfd/name which means we went through
the `/proc` path, which is slower and uglier.
Noticed this in strace while looking at adoption code.
Closes: #1280
Approved by: jlebon
For checkouts that are on the same device, for regular files we can simply
"adopt" existing files. This is useful in the "build from subtrees" pattern that
happens with e.g. `rpm-ostree install` as well as flatpak and gnome-continuous.
New files are things like an updated `ldconfig` cache, etc. And particularly for
`rpm-ostree` we always regenerate the rpmdb, which for e.g. this workstation is
`61MB`.
We probably should have done this from the start, and instead had a `--copy`
flag to commit, but obviously we have to be backwards compatible.
There's more to do here - the biggest gap is probably for `bare-user` repos,
which are often used with things like `rpm-ostree compose tree` for host
systems. But we can do that later.
Closes: #1272
Approved by: jlebon
This ends up a lot better IMO. This commit is *mostly* just
`s/glnx_close_fd/glnx_autofd`, but there's also a number of hunks like:
```
- if (self->sysroot_fd != -1)
- {
- (void) close (self->sysroot_fd);
- self->sysroot_fd = -1;
- }
+ glnx_close_fd (&self->sysroot_fd);
```
Update submodule: libglnx
Closes: #1259
Approved by: jlebon
It's no longer called directly by the pull code, so make it static.
The goal here is to have the pull and local-fs commit paths use higher level
more efficient APIs, and eventually make those APIs public.
Closes: #1257
Approved by: jlebon
This simplifies a lot of code; the header function was structured
to write to an input stream, but many callers only wanted the checksum,
so it's simpler (and error-free) to simply allocate a whole buffer
and checksum that.
For the callers that want to write it, it's also still simpler to allocate the
buffer and write the whole thing rather than having this function do the
writing.
A lot of the complexity here again is a legacy of the packfile code, which is
dead.
This is prep for faster regfile commits where we can avoid `G{In,Out}putStream`.
Closes: #1257
Approved by: jlebon
Nothing was using the `bytes_written` data (we always discard partially written
tmpfiles), so simplify everything by dropping it. Further, we always passed an
offset of `0`, so drop that argument too. (I believe that this was previously
used by the "pack files" code that we deleted long ago)
Second, we had an unnecessary internal wrapper for this function; drop that too.
Closes: #1257
Approved by: jlebon
The faster (OpenSSL/GnuTLS) code lived in a `GInputStream` wrapper, and that
adds a lot of weight (GObject + vtable calls). Move it into a simple
autoptr-struct wrapper, and use it in the metadata path, so we're
now using the faster checksums there too.
This also drops a malloc there as the new API does hexdigest in place to a
buffer.
Prep for more work in the commit path to avoid `GInputStream` for local file
commits, and ["adopting" files](https://github.com/ostreedev/ostree/pull/1255).
Closes: #1256
Approved by: jlebon
For many cases of commit, we can actually optimize things by simply "adopting"
the object rather than writing a new copy. For example, in rpm-ostree package
layering.
We can only make that optimization though if we take ownership of the file. This
commit hence adds an API where a caller tells us to do so. For now, that just
means we `unlink()` the files/dirs as we go, but we can now later add the
"adopt" optimization.
Closes: #1255
Approved by: jlebon
Buried in this large patch is a logical fix:
```
- if (!map)
- return glnx_throw_errno_prefix (error, "mmap");
+ if (map == (void*)-1)
+ return glnx_null_throw_errno_prefix (error, "mmap");
```
Which would have helped me debug another patch I was working
on. But it turns out that actually correctly checking for
errors from `mmap()` triggers lots of other bugs - basically
because we sometimes handle zero-length variants (in detached
metadata). When we start actually returning errors due to
this, things break. (It wasn't a problem in practice before
because most things looked at the zero size, not the data).
Anyways there's a bigger picture issue here - a while ago
we made a fix to only use `mmap()` for reading metadata from disk
only if it was large enough (i.e. `>16k`). But that didn't
help various other paths in the pull code and others that were
directly doing the `mmap()`.
Fix this by having a proper low level fs helper that does "read all data from
fd+offset into GBytes", which handles the size check. Then the `GVariant` bits
are just a clean layer on top of this. (At the small cost of an additional
allocation)
Side note: I had to remind myself, but the reason we can't just use
`GMappedFile` here is it doesn't support passing an offset into `mmap()`.
Closes: #1251
Approved by: jlebon
Appease Coverity by using the same condition for both the ternary check
and the if-condition later on. It should be smart enough to figure out
that `dir_enum == NULL` implies that `dfd_iter != NULL` from the
assertion at the top of the function.
Coverity CID: #1457318Closes: #1250
Approved by: cgwalters
Spotted while reading through the code, it looks like the
copy_detached_metadata() call is accidentally omitted if a hardlink
already exists for the .commit object.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1242
Approved by: cgwalters
We can't use the cache if the file we want to commit has been modified
by the client through the file info or xattr modifiers. We would
prematurely look into the cache in `write_dfd_iter_to_mtree_internal`,
regardless of whether any filtering applied.
We remove that path there, and make sure that we only use the cache if
there were no modifications. We rename the `get_modified_xattrs` to
`get_final_xattrs` to reflect the fact that the xattrs may not be
modified.
One tricky bit that took me some time was that we now need to store the
st_dev & st_ino values in the GFileInfo because the cache lookup relies
on it. I'm guessing we regressed on this at some point.
This patch does slightly change the semantics of the xattr callback.
Previously, returning NULL from the cb meant no xattrs at all. Now, it
means to default to the on-disk state. We might want to consider putting
that behind a flag instead. Though it seems like a more useful behaviour
so that callers can only override the files they want to without losing
original on-disk state (and if they don't want that, just return an
empty GVariant).
Closes: #1165Closes: #1170
Approved by: cgwalters
In particular I'd like to get the copy fix in, since it might affect users for
the keyring bits.
Update submodule: libglnx
Closes: #1225
Approved by: jlebon
Noticed this while reading the code. The `child` var hasn't been
initialized yet at the time we throw this error (and even then, it's
only conditionally initialized). To be nice, let's just always calculate
the child path and pass that along.
Also do some minor style porting to decl near use.
Closes: #1216
Approved by: cgwalters
Add a few comments for each of the central functions used for committing
data from a directory. Took me a bit to understand the relationship
between those functions.
Closes: #1216
Approved by: cgwalters
This fixes up the last of the embarassing bits I saw from
the stack trace in:
https://github.com/ostreedev/ostree/issues/1184
We had a hardlink fast path, but that doesn't apply across
devices, which occurs in two notable cases:
- Installer ISO with local repo
- Tools like pungi that copy the repo to a local snapshot
Obviously there are a lot of subtleties here around things like the
bare-user-only conversions as well as exactly what data we copy. I think to get
better test coverage we may want to add `pull-local --no-hardlink` or so.
Closes: #1197
Approved by: jlebon
For the old `OSTREE_REPO_MODE_ARCHIVE_Z2`. Use it mostly tree
wide except for the repo finder tests (to avoid conflicting with
some outstanding PRs).
Just noted another user coming in some of those tests and wanted to do a
cleanup.
Closes: #1209
Approved by: jlebon
We added a `.dir-locals.el` in commit: 9a77017d87
There's no need to have it per-file, with that people might think
to add other editors, which is the wrong direction.
Closes: #1206
Approved by: jlebon
While opening a repo we've recorded the device/inode for a while; use it to
avoid calling `linkat()` during object import if we know it's going to fail.
Closes: #1193
Approved by: jlebon
Conceptually `ostree-repo-pull.c` should be be written using
just public APIs; we theoretically support building without HTTP
for people who just want to use the object store portion and
do their own fetching.
We have some nontrivial behaviors in the pull layer though; one
of those is the "bareuseronly" verification. Make a new internal
API that accepts flags, move it into `commit.c`. This
is prep for further work in changing object import to support
reflinks.
Closes: #1193
Approved by: jlebon
There are use cases for not syncing at all; think build cache repos, etc. Let's
be consistent here and make sure if fsync is disabled we do no sync at all.
I chose this opportunity to add tests using the shiny new strace fault
injection. I can forsee using this for a lot more things, so I made
the support for detecting things generic.
Related: https://github.com/ostreedev/ostree/issues/1184Closes: #1186
Approved by: jlebon
Update libglnx, which is mostly port the repo stagedir code
to the new tmpdir API. This turned out to require some
libglnx changes to support de-allocating the tmpdir ref while
still maintaining the on-disk dir.
Update submodule: libglnx
Closes: #1172
Approved by: jlebon
Doing this in prep for libglnx tmpdir porting, but I think we should also do
this because the partial fetch code IMO was never fully baked; among other
things it was never integrated into the scheme we came up with for "boot id
sync" that we use for complete/staged objects.
There's a lot of complexity here that while we have some coverage for, I think
we need to refocus on the core functionality. The libcurl backend doesn't have
an equivalent to this today.
In particular for small objects, this is simply overly complex. The downside is
clearly for large objects like FAH's 61MB initramfs; not being able to resume
fetches of those is unfortunate.
In practice though, I think most people should be using deltas, and we need to
make sure deltas work for large objects anyways.
Further ultimately the peer-to-peer work should help a lot for people
with truly unreliable connections.
Closes: #1176
Approved by: jlebon
There were some important ones there like a random `syncfs()`. The remaining
users are mostly blocked on the "fstatat enoent" case, I'll wait to port those.
Closes: #1150
Approved by: jlebon
The new --selinux-policy added in [0] exposed a subtle issue in the way
we handle labeling during commit. The CI system in rpm-ostree hit this
when trying to make use of it[1].
Basically, because of the way we use a GVariant to represent xattrs, if
a file to be committed already has an SELinux label, the xattr object
ends up with *two* label entries. This of course throws off fsck later
on, since the checksum will have gone over both entries, even though the
on-disk file will only have a single label (in which the second entry
wins).
I confirmed that the `fsck` added in the installed test fails without
the rest of this patch.
[0] https://github.com/ostreedev/ostree/pull/1114
[1] https://github.com/projectatomic/rpm-ostree/pull/953Closes: #1121
Approved by: cgwalters
The wrapping here is unnecessary, since `_ostree_repo_commit_modifier_apply()`
already does what this function did. Further, the return type was wrong.
Saw this while doing some libarchive work.
Closes: #1104
Approved by: jlebon
This is mostly the `copy_file_range` changes plus the Coverity files.
```
Colin Walters (4):
localalloc: Abort on EBADF from close() by default
local-alloc: Remove almost all macros like glnx_free, glnx_unref_variant
console: Fix Coverity NULL deref warning
fdio: Merge systemd code to use copy_file_range(), use FICLONE
Jonathan Lebon (1):
console: trim useless check
Matthew Leeds (1):
dirfd: Fix typo in comment
Philip Withnall (1):
glnx-console: Add missing NULL check before writing out text
```
Update submodule: libglnx
Closes: #1081
Approved by: jlebon
(remaining > 0) is asserted by the loop condition, and remaining is not
modified between that check and the G_UNLIKELY — so the condition in the
G_UNLIKELY will always be true.
Spotted by Coverity as issue #1452617.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1059
Approved by: cgwalters
Part of cleaning up our usage of libglnx; we want to use what's in GLib where we
can.
Had to change a few .c files to `#include ostree.h` early on to pick up
autoptrs for the core types.
Closes: #1040
Approved by: jlebon
There are multiple use cases where we'd like to alias refs.
First, having a "stable" alias which gets swapped across major
versions: https://pagure.io/atomic-wg/issue/228
Another case is when a ref is obsoleted;
<https://pagure.io/atomic-wg/issue/303>
This second one could be done with endoflife rebase, but I think
this case is better on the server side, as we might later change
our minds and do actual releases there.
I initially just added some test cases for symlinks in the `refs/heads` dir to
ensure this actually works (and it did), but I think it's worth having APIs.
Closes: #1033
Approved by: jlebon
Coverity complained that the `else if (bytes_read == 0)` was dead
code if we happened to find it was already false when testing
`else if (G_UNLIKELY (bytes_read == 0 ...`.
There was nothing wrong with the logic, but let's rework it to
only test the value once; I think it does end up nicer anyways.
Coverity CID: 1452186
Closes: #1037
Approved by: jlebon
Mostly for the latest `-Wmaybe-uninitialized` fix, but while here also port some
places to newer APIs.
Update submodule: libglnx
Closes: #1027
Approved by: jlebon
Regression from previous tmpfile refactoring; unfortunately
the `OSTREE_REPO_COMMIT_MODIFIER_FLAGS_GENERATE_SIZES` option
only has coverage via gjs currently.
Might expose it via the cmdline in a later option, but in the big picture the
idea was that this data is better kept in static deltas.
Closes: https://github.com/ostreedev/ostree/issues/1014Closes: #1016
Approved by: jlebon
Using the error prefixing in the delta processing allows us to
do new code style. Also strip trailing whitespace.
Use error prefixing in a few other random places. I didn't
hunt for all of them, just testing out the new API.
Use `glnx_fchmod()`. Also note I dropped one `fchmod (tmpf, 0600)`
which is no longer necessary.
Update submodule: libglnx
Closes: #1011
Approved by: jlebon
Use goffset rather than gsize for file sizes. More importantly, get the
unpacked_size from g_file_info_get_size() (goffset) rather than from the
splice return value, which has type gssize.
This will make a difference on 32-bit systems, where goffset is defined
as off64_t, but gsize is 32 bits.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #999
Approved by: cgwalters
And in general, if for some reason we can't write `user.` xattrs, provide an
error immediately rather than doing it during a later pull. This way the failure
cause is a lot more obvious.
Related: https://github.com/ostreedev/ostree/issues/991Closes: #993
Approved by: jlebon
For ostree-as-host, we're the superuser, so we'll blow past
any reserved free space by default. While deltas have size
metadata, if one happens to do a loose fetch, we can fill
up the disk.
Another case is flatpak: the system helper has similar concerns
here as ostree-as-host, and for `flatpak --user`, we also
want to be nice and avoid filling up the user's quota.
Closes: https://github.com/ostreedev/ostree/issues/962Closes: #987
Approved by: jlebon
This is prep for storage space checks, where we look at free
space after parsing the metadata, before we write anything.
We did length-limited writes in the fd-based input path, but not for the
`GInputStream` path which in practice is used for HTTP pulls.
Closes: #987
Approved by: jlebon
Some of the Jenkins jobs for Fedora Atomic Host broke after updating
to 2017.7, and it turns out that we regressed handling unreadable
files in `bare-user` mode. An example of this is `/etc/shadow`, which
ends up in the ostree-as-host content as `/usr/etc/shadow`.
Now there are better fixes here; we should probably delete it and create it
during the config merge if it doesn't exist. In general, having secret files in
ostree really isn't supported, so it doesn't make sense to include them.
But let's fix this regression - when operating as an unprivileged user we don't
have `CAP_DAC_OVERRIDE` and hence will fail to open un-user-readable objects.
(We still preserve the actual `0` mode of course in the xattr and will
apply it in `bare`)
Closes: #989
Approved by: jlebon
I had thought `glnx_link_tmpfile_at()` actually consumed the tmpfile;
it does consume the *path* but not the fd. In the non-delta path
things were fine since we used the autocleanup.
But the delta code had a tmpfile allocated in its struct that got reused, and
hence leaked the fd. Fix this by making the commit API actually consume the
tmpfile fully, just like the path path.
Closes: #986
Approved by: jlebon
It's more natural for a few calling places. Prep for patches to go the other
way, which in turn are prep for adding a commit filter v2 that takes `struct
stat`.
`ot_gfile_type_for_mode()` was only used in this function, so inline it here.
Closes: #974
Approved by: jlebon
Use the new macros introduced recently in libglnx to make iterating over
hash tables cleaner. This is just a start, it does not migrate the whole
tree.
Update submodule: libglnx
Closes: #971
Approved by: cgwalters
There's lots of mechanically replacing `OtTmpFile` with `GLnxTmpfile`;
the biggest changes are in the commit path. Symlink commits are now
very clearly separated from regular files. Symlinks are `OtCleanupUnlinkat`,
and regular files are `GLnxTmpfile`.
The commit codepath separates those as `_ostree_repo_commit_path_final()` and
`_ostree_repo_commit_tmpf_final()`. A nice aspect of all of this is that they
both *consume* the temporary on success. This avoids an extra spurious
`unlink()` call.
One of the biggest bits of code motion is in `commit_loose_regfile_object()`,
which no longer needs to care about symlinks. For the most parth though it's
just removing conditionals.
Update submodule: libglnx
Closes: #958
Approved by: jlebon
The `OstreeRepoContentBareCommit` struct was basically an `OtTmpFile`, so let's
make it one. I moved the "convert to `GOutputStream`" logic into the callers,
since that bit can't fail; it makes the implementation much simpler since we can
just return the result of `ot_open_tmpfile_linkable_at()`.
Prep for `GLnxTmpfile` porting.
Closes: #957
Approved by: jlebon
The pull code also could make use of this in both the metadata and content
paths. I changed it to own the tempfile malloc (just like `GLnxTmpFile`), since
there's no reason to have different lifetimes for the filename and the file, and
that way we only have one variable rather than two.
The content path turns out to be a special case though, where
at least for mirroring archives, we directly pass the file *path*
down into `_ostree_repo_commit_loose_final()`.
This is prep for `GLnxTmpFile` porting.
Closes: #957
Approved by: jlebon
The variables here were duplicative; we don't need two booleans to distinguish
between symlinks and regular files. What we do need to handle is the "physical"
state versus the "object" state. Symlinks objects are stored as regular files in
`bare-user` and `archive`.
Prep for more cleanup.
Closes: #957
Approved by: jlebon
These are tuples of (collection ID, ref name) which are a globally-unique
form of local ref. They use OstreeCollectionRef as an identifier, and hence
need to be accessed using new API, as the existing API uses string
identifiers and sometimes accepts refspecs. Remote names are not
supported as part an OstreeCollectionRef.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #924
Approved by: cgwalters
Both callers of `commit_loose_object_trusted()` were passing
`OSTREE_OBJECT_TYPE_FILE`, so drop that parameter. This in turn
allows us to drop lots of checking of that inside the function.
Add a doc comment, and rename to `commit_loose_content_object()` for clarity.
Closes: #914
Approved by: alexlarsson
I noticed my previous patches incorrectly started doing `return glnx_throw*`
inside a `goto out;` function. Fix this by porting forward consistently to new
style. We just do the error prefixing in the caller.
Closes: #914
Approved by: alexlarsson
When a transaction is finished and we have moved all the staged loose
objects into the repo we fsync all the object directory, to ensure the
filenames are stable before we update the refs files to point to the
new commits.
With out this an unclean shutdown after the transaction is finished
could result in a refs file that points to an incomplete commit.
https://bugzilla.gnome.org/show_bug.cgi?id=759442Closes: #918
Approved by: cgwalters
These exist in the wild for flatpak, and aren't really a problem. The canonical
permissions are still either `0755` or `0644`, we just support the additional
writable bit for the group (i.e. extend the set to include `0775` and `0664`)
now to avoid breaking some flatpak content.
Closes: #913
Approved by: alexlarsson
Having every object in a bare-user repo (and checkouts) be executable
is ugly. I can't think of a good reason to do that; they should only
be executable if their input is. This does
for `bare-user` what we did for `bare-user-only` in
https://github.com/ostreedev/ostree/pull/909
It's also a stronger version of what we do with `checkout -U` in suppressing
suid - here we also strip world-writable files and the sticky bit (even though
that's meaningless today, it might not be in the future).
Closes: https://github.com/ostreedev/ostree/issues/907Closes: #908
Approved by: alexlarsson
For the flatpak use case where bare-user-only was introduced, we actually
don't want to support s{u,g} id files in particular.
Actually, I can't think of a reason to have anything outside of the
`0755 i.e. (u=rwx,g=rx,o=rx)` mask, so that's what we do here.
This will have the effect of treating existing `bare-user-only` repositories as
corrupted if they have files outside of that mask, but I think we should do this
now; most of the flatpak users will still be on `bare-user`, and we haven't
changed the semantics of that mode yet.
Note that in this patch we will also *reject* file content that doesn't
match this. This is somewhat asymmetric, since we aren't similarly rejecting
e.g. directory metadata. But, this will close off the biggest source
of the problem for flatpak (setuid binaries).
See: https://github.com/ostreedev/ostree/pull/908
See: https://github.com/flatpak/flatpak/pull/837Closes: #909
Approved by: alexlarsson
There was a lot of conditionals inside `write_object()` differentating
between metadata/content, and then for content, on the different repo
types. Further, in the metadata path since the logic is simpler, can
present a non-streaming API, and further use `OtTmpfile`, etc.
Splitting them up helps drop a lot of conditionals. We introduce a small
`CleanupUnlinkat` that allows us to fully convert to the new code style in both
functions.
This itself is still prep for fully switching to `GLnxTmpfile`.
Closes: #881
Approved by: jlebon
If we have an expected checksum, call `fstatat(repo_dfd, checksum)`
early on before we do much else. This actually duplicates code,
but future work here is going to split up the metadata/content
commit paths, so they'll need to diverge anyways.
Closes: #881
Approved by: jlebon
First, the streaming metadata API is pretty dumb, since metadata
should be small. Really we should have supported a `GBytes`
version. Currently, this API *is* used when we do local pulls,
so this commit has test coverage. However, I plan to change
the object import to avoid using this. But that's fine, since
I can't think of why someone would use this API.
Next, the only difference between `ostree_repo_write_metadata()` and
`ostree_repo_write_metadata_trusted()` is whether or not we pass
an output checksum; so just dedup the implementations.
Also while I'm here break out the input length validation and do
it early in the streaming case.
Closes: #881
Approved by: jlebon
Rather than `g_output_stream_splice()`, where the input is a regular
file.
See https://github.com/GNOME/libglnx/pull/44 for some more information.
I didn't try to measure the performance difference, but seeing the
read()/write() to/from userspace mixed in with the pointless `poll()` annoyed me
when reading strace.
As a bonus, we will again start using reflinks (if available) for `/etc`,
which is a regression from the https://github.com/ostreedev/ostree/pull/797
changes (which before used `glnx_file_copy_at()`).
Also, for the first time we'll use reflinks when doing commits from file-backed
content. This happens in `rpm-ostree compose tree` today for example.
Update submodule: libglnx
Closes: #817
Approved by: jlebon
Allow GI bindings to delete refs through ostree_repo_transaction_set_ref
and ostree_repo_transaction_set_refspec by setting the checksum to NULL.
Closes: #834
Approved by: cgwalters
While running the testsuite under valgrind a small memory leak showed up:
==16487== 65 bytes in 1 blocks are definitely lost in loss record 773 of 1,123
==16487== at 0x4C2BBAF: malloc (vg_replace_malloc.c:299)
==16487== by 0x6048E08: g_malloc (gmem.c:94)
==16487== by 0x6062EAE: g_strdup (gstrfuncs.c:363)
==16487== by 0x54CE3E6: write_object (ostree-repo-commit.c:776)
==16487== by 0x54CF2D4: ostree_repo_write_metadata (ostree-repo-commit.c:1528)
==16487== by 0x54CF505: _ostree_repo_write_directory_meta (ostree-repo-commit.c:1712)
==16487== by 0x54D0AB4: write_dfd_iter_to_mtree_internal (ostree-repo-commit.c:2650)
==16487== by 0x54D0E2D: ostree_repo_write_dfd_to_mtree (ostree-repo-commit.c:2793)
==16487== by 0x1190C4: ostree_builtin_commit (ot-builtin-commit.c:474)
==16487== by 0x11F2EE: ostree_run (ot-main.c:200)
==16487== by 0x116F32: main (main.c:78)
The reason for this is that ot_checksum_instream_get_string returns a chunk of newly allocated memory which never got freed.
Make actual_checksum something that gets autocleanend and own the memory
assigned to it in all cases.
Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
Closes: #827
Approved by: pwithnall
If we're freeing the segment, it's basically always better to use
`autoptr()`. Fewer lines, more reliable, etc.
Noticed an instance of this in the pull code while reviewing a different PR,
decided to do a grep for it and fix it tree wide.
Closes: #836
Approved by: pwithnall
I was working on `rpm-ostree livefs` which does some ostree-based
filesystem diffs, and noticed that we were ending up with `/proc`
not being labeled in our base trees.
Reading the selinux-policy source, indeed we have:
```
/proc -d <<none>>
/proc/.* <<none>>
```
This dates pretty far back. We really don't want unlabeled
content in ostree. In this case it's mostly OK since the kernel
will assign a label, but again *everything* should be labeled via
OSTree so that it's all consistent, which will fix `ostree diff`.
Notably, `/proc` is the *only* file path that isn't covered when composing a
Fedora Atomic Host. So I added a hack here to hardcode it (although I'm a bit
uncertain about whether it should really be `proc_t` on disk before systemd
mounts or not).
Out of conservatism, I made this a flag, so if we hit issues down the line, we
could easily change rpm-ostree to stumble on as it did before.
Closes: #768
Approved by: jlebon
Joaquim Rocha
Giuseppe Scrivano
Marcus Folkesson
Colin Walters
Dan Nicholson
Jonathan Lebon
Philip Withnall
Alexander Larsson
Sjoerd Simons