Define an auto cleanup handler for use with repo locking. This is based
on the existing auto transaction cleanup. A wrapper for
ostree_repo_lock_push() is added with it. The intended usage is like so:
g_autoptr(OstreeRepoAutoLock) lock = NULL;
lock = ostree_repo_auto_lock_push (repo, lock_type, cancellable, error);
if (!lock)
return FALSE;
The functions and type are marked to be skipped by introspection since I
can't see them being usable from bindings.
Closes: #1343
Approved by: cgwalters
Currently ostree has no method of guarding against concurrent pruning.
When there are multiple repo writers, it's possible to have a pull or
commit race against a prune and end up with missing objects.
This adds a file based repo locking mechanism. The intention is to take
a shared lock when writing objects and an exclusive lock when deleting
them. In order to make use of the locking throughout the library in a
fine grained fashion, the lock acts recursively with a stack of lock
states. If the lock becomes exclusive, it will stay in that state until
the stack is unwound past the initial exclusive push. The file locking
is similar to GLnxLockFile in that it uses open file descriptor locks
but falls back to flock when needed.
The lock also attempts to be thread safe by storing the lock state in
thread local storage with GPrivate. This means that each thread will
have an independent lock for each repository it opens. There are some
drawbacks to that, but it seemed impossible to manage the lock state
coherently in the face of multithreaded access.
The API is a push/pop interface in accordance with the recursive nature
of the locking. The push interface uses an enum that's translated to
LOCK_SH or LOCK_EX as needed. Both interfaces use an internal timeout
field to decide whether to manage the lock in a blocking or non-blocking
fashion. The intention is to allow ostree applications as well as
administrators to control this timeout. For now, the default is a 30
second timeout.
Note that the timeout is handled synchronously in thread since the lock
is maintained in thread local storage. I.e., the thread that acquires
the lock needs to be the same thread that runs the operation. There may
be a way to offer an asynchronous version, but it's not clear exactly
how that would work since it would likely involve a separate thread that
invokes a callback when the locking operation completes.
https://bugzilla.gnome.org/show_bug.cgi?id=759442Closes: #1343
Approved by: cgwalters
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
Time to cut a new release, we've got the libcurl cleanup ordering patch which
several people have hit, along with safe early fixes for tmpdir cleanup. Let's
try to land the locking PR early next cycle.
Closes: #1359
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
This squashes the last race condition I was actively hitting while running
`test-concurrency.py` in a loop. The race is when process A finds a tmpdir to
reuse, and goes to lock it. Meanwhile process B deletes it and unlocks the lock.
Process A then succeeds at grabbing a lock, but the tmpdir is deleted.
Closes: #1352
Approved by: dbnicholson
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
When using dynamic remotes (LAN and USB), we cannot use their name with
the common remote related ops (ostree_repo_remote_...) because ostree
doesn't keep this type of remotes in its internal hash table.
Unfortunately this means that we cannot access the URL of those remotes
either (in order to e.g. set the right URL for those remotes in
Flatpak).
Since the URL is actually stored in a key file that belongs to the
OstreeRemote, then we can simply allow users access to it through a
getter.
So this patch adds a method that allows to return the URL directly from
the OstreeRemote without having to go through the OstreeRepo.
The test-repo-finder-config is also updated by this patch to check if
the URL is correct.
Closes: #1353
Approved by: cgwalters
We use utimens instead of utime, thus allowing nanosecond timestamps,
and also fixes a bug where we used to passed UTIME_OMIT to tv_nsec
which made the entire operation a no-op.
Closes: #1351
Approved by: cgwalters
They don't play nicely currently with HTTP2 where we may
have lots of requests queued.
https://github.com/ostreedev/ostree/issues/878#issuecomment-347228854
In practice anyways I think issues here are better solved on a higher level -
e.g. apps today can use an overall timeout on pulls and if they exceed the limit
set the cancellable.
Closes: #1349
Approved by: jlebon
If a newly allocated tmpdir can't be locked, set initialized to FALSE so
that glnx_tmpdir_cleanup doesn't delete it when new_tmpdir goes out of
scope.
Closes: #1346
Approved by: cgwalters
Another tmpdir user may have deleted an existing tmpdir between the time
the current user called readdir and tried to open it.
Closes: #1346
Approved by: cgwalters
By default, unless it’s const, an (out) GHashTable will be assumed to be
(transfer full). That means the binding needs to free all the items in
the hash table, plus the table itself.
However, all the GHashTables we use have free functions set already, so
freeing the hash table will free its items. This results in a
double-free.
Fix that by ensuring we annotate such (out) hash tables as (transfer
container). Also annotate some other hash tables as (transfer none)
where appropriate, for clarity.
This fixes OSTree.Repo.list_collection_refs() in the Python bindings.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1341
Approved by: dbnicholson
This reverts commit 519b30b7e1. Now that
the experimental GIR is being built correctly and OstreeRemote is a real
boxed type, this can be exposed again.
Closes: #1337
Approved by: pwithnall
Now that g-ir-scanner is being told about ENABLE_EXPERIMENTAL_API, it
can include these types correctly. Drop the __GI_SCANNER__ guards in the
header files so that all the declarations are found.
After this, you can actually construct the types normally:
>>> OSTree.CollectionRef.new('com.example.Foo', 'bar')
<OSTree.CollectionRef object at 0x7f2bba4c7528 (OstreeCollectionRef at 0x55c033ff2f30)>
Closes: #1337
Approved by: pwithnall
Without this, you can't really use OstreeRemote as a GObject, which is a
requirement for bindings.
This was found when attempting to include OstreeRemote in the GIR, and
g-ir-scanner wasn't able to link it's temporary object due to an
"undefined reference to `ostree_remote_get_type'" error.
Closes: #1337
Approved by: pwithnall
I wanted to inspect a summary file the other day and was saddened to
find it was broken:
$ ostree summary --raw
error: No option specified; use -u to update summary
Fix the test to do the normal thing of passing just --raw without
--view. It's legal to pass --raw and --view, but it shouldn't be a
requirement.
Closes: #1336
Approved by: cgwalters
Fedora switched to 'xz' compress kernel modules, and recently
[RHEL7 did too](https://bugzilla.redhat.com/show_bug.cgi?id=1367496).
This compression defeats bsdiff.
While we have a "rollsum-able" test, we don't have a "bsdiff-able" test as it'd
be very expensive (we'd have to bsdiff, then apply it and compare the result).
Let's do the tactical quick fix here and just not try to delta files ending in
`.xz.`. This avoids us using bsdiff pointlessly for over 4000 files, which is
quite a notable speed increase for generating deltas.
Closes: #1333
Approved by: jlebon
This allows more explicit handling of commit state in code using
libostree, rather than hard-coding a commit state of 0 for ‘normal’.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1335
Approved by: cgwalters
Reorder cleanup functions so that curl_multi_cleanup() runs before
self->sockets is destroyed. This avoids an assert and invalid memory
access in sock_cb where self->sockets is dereferenced during
curl_multi_cleanup().
Closes: https://github.com/ostreedev/ostree/issues/1331Closes: #1332
Approved by: cgwalters
A tricky thing here that caused this to go past a lot of our tests
is that the code was mostly OK if there was an available delta from
an older commit. But this case broke if we e.g. had a new OS
deployment and did a `--require-static-deltas` pull, i.e. the initial
state.
I cleaned up our "find static delta state" function to return an enumeration,
and extended it with an "already have the commit" state. A problem
I then hit is that we've historically fetched detached metadata for
non-delta pulls, even if the commit hasn't changed. I decided not to
do that for `--require-static-deltas` pulls for now; otherwise the
code gets notably more complex.
Closes: https://github.com/ostreedev/ostree/issues/1321Closes: #1323
Approved by: jlebon
The main thing here is that a ton of stuff has happened in gnulib since we
imported `parse-datetime.y`. I cherry-picked a little bit of it, but that
upstream doesn't seem to build with `-Wundef`, so I just deleted some hunks.
(Note I reindented the warnings consistently)
Update submodule: libglnx
Closes: #1320
Approved by: jlebon
Since ostree_remote_get_type is not made available to g-ir-scanner, it
treats OstreeRemote as a bare struct. That's not kosher for bindings and
it issues the following warning:
src/libostree/ostree-repo-pull.c:5560: Warning: OSTree:
ostree_repo_resolve_keyring_for_collection: return value: Invalid
non-constant return of bare structure or union; register as boxed type
or (skip)
For now, just skip this API for bindings.
Closes: #1322
Approved by: pwithnall
g-ir-scanner was spitting this warning:
src/libostree/ostree-core.c:281: Warning: OSTree:
ostree_validate_collection_id: unknown parameter 'rev' in
documentation comment, should be 'collection_id'
Closes: #1322
Approved by: pwithnall
It's the slowest part, let's show admins something. This "update every 10%" code
was copied from the fsck command; obviously a better approach would be "progress
every N seconds" but doing that somewhat accurately requires making things
async; not worth it here yet.
Closes: #1314
Approved by: jlebon
I didn't fully spelunk this, but from what `static-delta-generate-crosscheck.sh`
had, we appeared to be doing this before, and it's clearly useful for local
testing rather than needing to spin up a HTTP server.
Closes: #1313
Approved by: jlebon
First, the manual crosscheck script bitrotted; it got caught up
in the "use libtest repo creation wrapper" bit, and also it
seems like at some point `pull --require-static-deltas` changed
meaning when dealing with `file:///` repos. I have more work to
unwind that.
Next, I'm seeing a delta failure which looks like a static delta
miscompilation with rollsums; change the compiler to print out
the source object too, which helped me debug this.
And finally in the processing code, fix incorrect error prefixing, which was
misleading.
Closes: #1311
Approved by: ashcrow
Fixes: 93457071cb "lib/deltas: Use pread() instead of lseek()+read()"
Caught this when trying to test alex's patch locally. I am going to review our
static delta pulls and try to get something more comprehensive locally. But in
the meantime this patch is clearly right.
Closes: #1312
Approved by: jlebon
Directly when we allocate a new part we finish the old one,
writing the compressed data to a temporary file and generating
the delta header for it.
When all these are done we loop over them and collect the headers,
sizes and either copy the tempfile data into the inlined superblock
or link the tempfiles to disk with the proper names.
Closes: #1309
Approved by: cgwalters
This allows us to create the final delta desciptor directly on disk
rather than having it all in memory. This is nice because it can
become quite large if inlined parts are used.
Note however, that we currently generate all the delta parts in
memory before adding them to the delta, so we still keep all individual
parts in memory. Fixing that is the next step.
Closes: #1309
Approved by: cgwalters
This is similar to GVariantBuilder in that it constructs variant
containers, but it writes it directly to a file descriptor rather
than keep the entier thing in memory. This is useful to create large
variants without using a lot of memory.
Closes: #1309
Approved by: cgwalters
g_autoptr was new in GLib 2.44, but we officially only require 2.40,
so we need to use the backport in libglnx.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Closes: #1310
Approved by: cgwalters
This makes the code nicer too. Properly unit testing this though really wants
like a whole set of stuff around parent repos...but we do have coverage of the
non-parent path in the current pull tests.
Closes: https://github.com/ostreedev/ostree/issues/1306Closes: #1308
Approved by: alexlarsson
For example, tmpfs or a cgroup file system. This is basically an
optimisation of the list of file systems we check for repositories,
since we would never expect any of these file systems to be capable of
containing a repository.
Depends on the new API from
https://bugzilla.gnome.org/show_bug.cgi?id=788927.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1307
Approved by: cgwalters
These two code paths tried to propagate errors which had never been set.
Set new errors instead.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1303
Approved by: cgwalters
A GVariantIter* was being passed to a GVariant format string varargs,
rather than a GVariantIter**. This resulted in memory corruption.
So we can continue to reuse ref_map throughout the function, make it a
GVariantIter* rather than a stack-allocated GVariantIter.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1301
Approved by: cgwalters
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
Add some missing annotations and clarify that it always returns an open
repository on success.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1300
Approved by: cgwalters
This is a freeform string useful to track/display when a commit is "derived"
from some other format. For example, in the rpm-ostree test we make a
`vmcheck` ref that conceptually overlays the default ref like
`fedora-atomic:fedora/26/x86_64/atomic-host`.
My current patch sets the source title to e.g.
"Dev overlay on fedora-atomic:fedora/26/x86_64/atomic-host".
Another case I'm working on now is importing OCI images to use
as host images. For that case, the source title is
With this patch we could then set the original OCI image name + tag
as the source name, like:
"oci:cgwalters/demo-custom-fedora-atomic-host:26".
Closes: #1296
Approved by: jlebon
Pull out the commit metadata explicitly; still just rendering the version, but
this is prep for rendering other metadata keys.
Closes: #1296
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
I'm playing around with some ostree ⇔ OCI/Docker bits, and ran
into this while importing an OCI image that built from the Fedora
base image where `/home` is a regular directory, and I added a layer
that did the ostree bits of moving it to `/var` and leaving a symlink.
OCI/Docker supports this. Now since "process whiteouts" is really the
"enable OCI/Docker" mode, let's only replace dirs if that's enabled.
This leaves the `UNION_FILES` targeted for its original use case
which is unioning components/packages. (Although that use case itself
is now a bit superceded by `UNION_IDENTICAL`, but eh).
Closes: #1294
Approved by: jlebon
This is similar idea as
5c0bf88915,
The duplicated description is now removed, and the description
of the command is now displayed beneath the Usage.
For example:
ostree cat -h will output the following:
"Usage:
ostree cat [OPTION?] COMMIT PATH...
Concatenate contents of files"
Closes: #1267
Approved by: cgwalters
This is a similar approach as
12c34bb249.
One thing to note is when we parse the admin related functions,
we still keep the old admin related flags, and added a new parameter
to represent the command struct.
This allows us to identify the caller of the function, making it
easier for us to possibly deduplicate the subcommand handling in
the future. A similar approach is done in rpm-ostree:
83aeb018c1
This also makes it easier for us to change the prototype of the function.
If we want to add something new in the future, we won't need to touch every prototype.
Closes: #1267
Approved by: cgwalters
Added a description argument to all type
of commands. Now when we include -h or --help
for commands that contain subcommands, the description
for those subcommands are shown.
The added subcommands help will be provided to the following commands:
- ostree -h
- ostree admin -h
- ostree admin instutil -h
- ostree remote -h
- ostree static-delta -h
Closes: #1267
Approved by: cgwalters
This is another OstreeRepoFinder implementation; it returns results from
a given set of URIs. It’s designed to be used for implementing user
overrides to other repo-finders, or for implementing unit tests.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1281
Approved by: mwleeds
Use g_variant_iter_loop() rather than next(), since it automatically
handles freeing the child memory each iteration. Previously, we leaked
it for all but the last iteration.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1293
Approved by: cgwalters
Change the regexp for validating refs to require at least one letter or digit
before allowing the other special chars in the set `[.-_]`. Names that start
with `.` are traditionally Unix hidden files; let's ignore them under the
assumption they're metadata for some other tool, and we don't want to
potentially conflict with the special `.` and `..` Unix directory entries.
Further, names starting with `-` are problematic for Unix cmdline option
processing; there's no good reason to support that. Finally, disallow `_` just
on general principle - it's simpler to say that ref identifiers must start with
a letter or digit.
We also ignore any existing files (that might be previously created refs) that
start with `.` in the `refs/` directory - there's a Red Hat tool for content
management that injects `.rsync` files, which is why this patch was first
written.
V1: Update to ban all refs starting with a non-letter/digit, and
also add another call to `ostree_validate_rev` in the pull
code.
Closes: https://github.com/ostreedev/ostree/issues/1285Closes: #1286
Approved by: jlebon
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
This is another case where making an input stream out of a memory buffer is a
bit silly; just hash the `GBytes` directly.
Closes: #1287
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
That's why the syscall was invented, so let's use it. Just noticed while reading
the code while working on another patch.
Closes: #1270
Approved by: jlebon
This isn't perfect, but at least we fix an error-overwrite error, and in
practice `ostree admin unlock` isn't wrapped by `rpm-ostree` yet, so spew to
stderr is OK.
Closes: https://github.com/ostreedev/ostree/issues/1273Closes: #1279
Approved by: guyshapiro
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
I was working on a patch to do build on the work done to
import content objects async to do the same for metadata, but right
now we basically rely on writing them first to do the GPG verification
when scanning.
Things will be cleaner for that if we can pass the commit object directly into
`scan_commit_object()` and consistently use `gpg_verify_unwritten_commit()`.
We're careful here to continue to do it both ways (but at most one time), to
account for the case where a bad commit has been pulled and written - we need to
keep failing GPG verification there.
Closes: #1269
Approved by: jlebon
Prep for a later patch to do GPG verification before writing commit objects;
`_ostree_repo_gpg_verify_with_metadata()` already handles this, and so dropping
this gives us consistent error messages.
Closes: #1269
Approved by: jlebon
ENOTSUP and EOPNOTSUPP are numerically equal on most Linux ports,
but inexplicably differ on PA-RISC (hppa) and possibly other
rare architectures.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Closes: #1275
Approved by: cgwalters
There's a subtle issue going on with the way we use `UNION_IDENTICAL`
now in rpm-ostree. Basically, the crux of the issue is that we checkout
the whole tree from the system repo, but then overlay packages by
checking out from the pkgcache repo. This is an easy way to break the
assumption that we will be merging hardlinks from the same repo.
This ends up causing issues like:
https://github.com/projectatomic/rpm-ostree/issues/1047
There, `vim-minimal` is already part of the host and has an object for
`/usr/share/man/man1/ex.1.gz`. `vim-common` has that same file, but
because it's unpacked in the pkgcache repo first, the hardlinks are not
the same.
There are a few ways we *could* work around this in rpm-ostree itself,
e.g. by re-establishing hardlinks when we do the content pull into the
system repo, but it still felt somewhat hacky. Let's just do this the
proper way and fall back to checksumming the target file if needed,
which is what librpm does as well in this case. Note that we only
checksum if they're not hard links, but they're the same size.
Closes: #1258
Approved by: cgwalters
In case a filename contains invalid UTF-8 characters, libostree will
pass it to g_variant_builder_add() in create_tree_variant_from_hashes()
anyway, which leads to a critical warning from glib and an invalid
commit. This commit makes ostree print a useful error and exit instead.
Closes: #1271
Approved by: cgwalters
Let's react to `Ctrl-C` faster here. Noticed while I was doing an update on my
desktop and playing with cancellation.
Closes: #1266
Approved by: jlebon
This is like `ostree_checksum_file` but fd-relative. This will be used
by https://github.com/ostreedev/ostree/pull/1258.
AFAICT, we actually didn't have any tests that check the `checksum` CLI.
Add a basic one here to test the old code as well as the new code.
Closes: #1263
Approved by: cgwalters
No functional changes, prep for patch. (Well, I did add a new `success`
member in the async struct so that we return `FALSE` if we failed).
Closes: #1263
Approved by: cgwalters
This works around an (IMO) SpiderMonkey bug - it tries to
clean up in a shared library destructor, but doesn't install a
`pthread_atfork()` handler to unset its state.
Closes: https://github.com/ostreedev/ostree/issues/1262Closes: #1264
Approved by: dbnicholson
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
If the filesystem is already frozen, FIFREEZE returns EBUSY, and if the
filesystem is already thawed, FITHAW returns EINVAL. It's very unlikely
these issues would arise on a real ostree system since the sysroot would
be locked during the freeze/thaw cycle.
However, when multiple fake sysroots are used during the test suite (run
as root), the tests could race to run the freeze/thaw cycle without
locking. Furthermore, there's no reason why an independent process might
be trying to freeze the filesystem while ostree was deploying. Ignore
but warn for these errors since there's not much ostree can do about it,
anyways.
Closes: #1260
Approved by: cgwalters
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
What the deltas code is doing is weird/unfortunate. The name
`ot_variant_read()` conflicts too much with `ot_variant_read_fd()`.
Since nothing else uses it, move it into the deltas code.
Closes: #1254
Approved by: jlebon
A lot of the libostree code is honestly too complex for its
own good (this is mostly my fault). The way we do HTTP writes
is still one of those. The way the fetcher writes tempfiles,
then reads them back in is definitely one of those.
Now that we've dropped the "partial object" bits in:
https://github.com/ostreedev/ostree/pull/1176 i.e. commit
0488b4870e
we can simplify things a lot more by having the fetcher
return an `O_TMPFILE` rather than a filename.
For trusted archive mirroring, we need to enable linking
in the tmpfiles directly.
Otherwise for at least content objects they're compressed, so we couldn't link
them in. For metadata, we need to do similar logic to what we have around
`mmap()` to only grab a tmpfile if the size is large enough.
Closes: #1252
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
We want `pull` to be included as long as we have at least either
`libcurl` or `libsoup` to back it. Of course, this is a moot point for
now since `libsoup` is currently a build requirement.
Closes: #1244
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
This commit adds debug output whenever libostree reads GPG keys, which
can come from different locations in the file system. This is especially
helpful in debugging "GPG signatures found, but none are in trusted
keyring" errors, which in my case was caused by OSTree looking in
/usr/local/share/ostree/trusted.gpg.d/ rather than
/usr/share/ostree/trusted.gpg.d/.
Closes: #1241
Approved by: cgwalters
I'm regretting a bit having the `guint8*csum` variant of checksums
except for the serialized form. Once we start doing processing
it's easier to just have it remain hex.
Do an on-stack conversion for the metadata scanning function; this
drops a malloc and also just looks nicer.
Also add some long-awaited function comments to the two.
Closes: #1240
Approved by: jlebon
These shouldn’t change the bloom filter’s behaviour at all, but make it
a bit more obvious what the programmatical limitations are on the sizes
it can deal with.
In reality, those sizes should never be reached because they won’t fit
in a DNS-SD record.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1239
Approved by: cgwalters
I was reading the pull code for the last release, and spotted
a bug in commit f923c2e1ea - in
the case where the ref doesn't exist, we don't set an error,
tripping an assertion in the main code.
The previous code wanted the ref to always exist, so just flip back the boolean
for "ignore noent". I moved the `g_strchomp()` just into the HTTP path - if a
local repo is corrupted in this way it's something to fix in that repo.
Closes: #1238
Approved by: pwithnall
This is the new way of publishing repository metadata, rather than as
additional-metadata in the summary file. The use of an ostree-metadata
ref means that the metadata from multiple upstream collections is not
conflated when doing P2P mirroring of many repositories.
The new ref is only generated if the repository has a collection ID set.
The old summary file continues to be generated for backwards
compatibility (and because it continues to be the canonical ref →
checksum map for the repository).
The new code is only used if configured with --enable-experimental-api.
Includes unit tests.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1158
Approved by: cgwalters
There is no error handling to do, so just return everywhere instead.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1158
Approved by: cgwalters
Compiling with -Wconversion warns on this line, as the conversion from
guint64 to guint8 is implicit (but safe: there is no bug here, since the
implicit cast is applied after the modulus arithmetic).
Make the cast explicit to silence -Wconversion.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1231
Approved by: cgwalters
There was an implicit cast from guint64 to gsize (which is 32-bit on
armhf, for example) before the modulus arithmetic which safely narrows
the index.
Fix that by using a guint64 intermediate variable and making the cast
explicit.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1231
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
I was trying to do a change for rpm-ostree to use
`OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS`
for container builds with `bare-user-only,` but hit an assertion here
ultimtely because we weren't setting `standard::type`.
Rather than hand-rolling `GFileInfo` creation, use the stat buffer conversion
code which is more robust and used in multiple places already.
Closes: #1227
Approved by: jlebon
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
I noticed this while debugging why I was seeing "2 metadata objects" fetched for
a different PR. I knew 1 was detached meta, but the other turned out to be this.
There's no reason to request a delta if the ref is unchanged.
Closes: #1220
Approved by: jlebon
Propagate the refspec_name from the OstreeRemote returned by an
OstreeRepoFinder through to the set_ref() call.
This changes ostree_repo_pull_with_options() to accept the
previously-disallowed combination of passing override-remote-name in
options and also setting a remote name in remote_name_or_baseurl.
ostree_repo_pull_with_options() will continue to pull using the remote
config named in remote_name_or_baseurl as before; but will now use the
remote name from override-remote-name when it’s setting the refs at the
end of the pull. This is consistent with the documentation for
override-remote-name.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1202
Approved by: cgwalters
When pulling from a dynamic (peer to peer) remote, the remote’s name is
set to a unique, generated string which doesn’t exist in repo/config. If
doing a non-mirror pull, however, we don’t want to use this name in the
refspecs for newly created or updated refs — we want to use the name of
the remote which provided the keyring for the pull (this will be a
remote from repo/config whose collection ID matches that being used for
the peer to peer pull).
Store both names in OstreeRemote. The name to use for refspecs is stored
as refspec_name, and is typically NULL unless it differs from name.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1202
Approved by: cgwalters
Instead of returning just the keyring filename, return the entire
OstreeRemote, which has the keyring filename as one of its members. This
will simplify some upcoming changes, and allows slightly improved debug
logging.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1202
Approved by: cgwalters
If override-remote-name is specified in the options to
ostree_repo_pull_with_options(), but the remote_name_or_baseurl argument
is also set to a remote name, the override-remote-name would be leaked.
Note that this is currently an invalid configuration, so this leak is
basically never hit.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1202
Approved by: cgwalters
Followup for recent work in commits:
- 8a7a359709
- 1a9a473580
Keep track of how many objects we imported, and print that for `ostree
pull-local` (also do this even if noninteractive, like we did for `pull`).
In implementing this at first I used separate variables for import
from repo vs import from localcache, but that broke some of the
tests that checked those values.
It's easier to just merge them; we know from looking at whether or not
`remote_repo_local` is set whether or not we were doing a "HTTP pull with
localcache" versus a true `pull-local` and can use that when rendering status.
Closes: #1219
Approved by: jlebon
This is more efficient in the non-collection case; in the collection
case, the implementation of ostree_repo_resolve_collection_ref() needs
to be rewritten to improve efficiency.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1182
Approved by: cgwalters
This is a parallel for ostree_repo_resolve_rev_ext() which works on
collection–refs. At the moment, the implementation is simple and uses
ostree_repo_list_collection_refs(). In future, it could be rewritten to
check the checksum directly rather than enumerating all
potentially-relevant checksums.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1182
Approved by: cgwalters
This can be used to put OSTree repositories on USB sticks in a format
recognised by OstreeRepoFinderMount.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1182
Approved by: cgwalters
Previously, collection–refs could only be pulled from a repository if it
had a summary file (which listed them). There was no way to pull from a
local repository which doesn’t have a summary file, and where the refs
were stored as refs/remotes/$remote/$ref, with a config section linking
that $remote to the queried collection ID.
Fix that by explicitly supporting pull_data->remote_repo_local in
fetch_ref_contents().
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #1182
Approved by: cgwalters
I now think commit fab1e113db was a mistake;
because it breaks the mental model that at least I'd built up that "local repos
don't have checksums verified, HTTP does".
For example, a problem with this is (with that mental model in place) it's easy
for people who set up mirrors like this to then do local pulls, and at that
point we've done a deployment with no checksum verification.
Further, since then we did PR #671 AKA commit 3d38f03 which is really most of
the speed hit.
So let's switch the default even for this case to doing checksum verification,
and add `ostree pull --http-trusted`. People who are in situations where they
know they want this can find it and turn it on.
Closes: https://github.com/ostreedev/ostree/issues/1211Closes: #1212
Approved by: jlebon
Rather than carrying two booleans, just convert `OstreeRepoPullFlags`
into `OstreeRepoImportFlags`. This allows us to drop an internal
wrapper function and just directly call `_ostree_repo_import_object()`.
This though reveals that our mirroring import path doesn't check the
`OSTREE_REPO_PULL_FLAGS_UNTRUSTED` flag...it probably should.
Prep for further work.
Closes: #1212
Approved by: jlebon
Make the "local repo" processing conditional the same as the "localcache" bits;
this is really just a de-indent. Also add some comments. Prep for further work.
Closes: #1212
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
Add this as an additional well-known directory which is checked on
mounted removable drives to see if it contains OSTree repos we can pull
refs from.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
https://github.com/ostreedev/ostree/issues/1210Closes: #1213
Approved by: cgwalters
Introduce support for GnuTLS for computing cryptograpic
hashes, similar to the OpenSSL backend. A reason to do
this is some distributors want to avoid GPLv3, and GPG
pulls that in.
A possible extension of using GnuTLS would be replacing the GPG signing
with `PKCS#7` signatures and `X.509` keys.
We also support `--with-crypto=openssl`, which has the same effect
as `--with-openssl`, and continues to be supported.
Changes by Colin Walters <walters@verbum.org>:
- Drop libgcrypt option for now
- Unify buildsystem on --with-crypto
Link: https://mail.gnome.org/archives/ostree-list/2017-June/msg00002.html
Signed-off-by: Jussi Laako <jussi.laako@linux.intel.com>
Closes: #1189
Approved by: cgwalters
Dan Nicholson
Colin Walters
Joaquim Rocha
Alexander Larsson
Dusty Mabe
Carlos Alberto Lopez Perez
Philip Withnall
Kalev Lember
Shaun Taheri
Simon McVittie
Ruixin Bao
Jonathan Lebon
Matthew Leeds
Matthew Leeds
Jussi Laako