It's completely unclear why this is necessary; the unmount calls
actually write to the separate mountpoints.json file, with a separate lock.
When this was originally introduced in b046fb5a9a ,
there wasn't a separate mountpoints.json file, and store.go was manually
calling Touch() everywhere; so my best guess is that this is just an
artifact of making those two API changes, and the Touch() is not actually
necessary.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Instead of going
3 store methods
store.updateNames+enum
3 sub-store methods
subStore.updateNames+enum
applyNameOperation+enum,
simplify to
3 store methods
store.updateNames+enum
subStore.updateNames+enum
applyNameOperation+enum,
Should not change behavior. Looking purely at updateNameOperation,
invalid values would now be detected after doing more work,
but there is no way for an external caller to trigger use of
an invalid value anyway.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
go { cmd.Run() } means the cmd will be destructed,
and stdin/stdout/stderr closed, concurrently with
other goroutines trying to access stdin/stdout/stderr.
Instead, do it the more traditional way and let the callers
who create those subprocesses explicitly manage their lifetime.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Have the action handlers return an error value, and let
main() format that error, if any; this avoids duplicated
error formating code in the action handlers, dropping
89 lines.
This might change the error format in some cases (typically
%v vs. %+v).
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
It can currently only fail with a broken pipe, or an invalid
pattern; in either case there's no point continuing with the loop.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
It is only used internally, and this avoids a warning
about a conflict with io.ByteReader.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
- imageTopLayerForMapping, in the cParentLayer loop, was reading all
additional layer stores, although only some of them might have been
locked at that point.
- getAutoUserNS -> getMaxSizeFromImage was assuming that all layer stores
are locked, but in fact the additional layer stores were all unlocked
at that point
- getMaxSizeFromImage was calling store.getLayerStore() and the like,
which could potentially trigger a reload via graphLock and return
a _different_ store than the one that was locked
So, lock _all_ layer stores in CreateContainer (at least on the path
where images are involved), and pass the locked layer stores down the
call stack.
Also, document a bit more explicitly what's going on.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... to avoid the repetition of getting, locking,
and reloading, the stores.
This also makes it less error-prone to use a consistent
locking order (but note that other instances of manual
locking still exist).
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... to avoid the repetition of getting, locking,
and reloading, the store.
Use it at least for the simplest writers.
This could be more elegant with Go generics, returning
(T, error), with T possibly being void = struct{}.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... to avoid the repetition of getting, locking,
and reloading, the store.
Use it at least for the simplest writers.
This could be more elegant with Go generic returning
(T, error), with T possibly being void = struct{}.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... to avoid the repetition of getting, locking,
and reloading, the store.
Use it at least for the simplest writers.
This could be more elegant with Go generics, returning
(T, error), with T possibly being void = struct{}.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... to remove the copy&pasted iteration and copy&pasted locking/
ReloadIfChanged.
That will also make it much easier to add more locking in the future.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... to remove the copy&pasted iteration and copy&pasted locking/
ReloadIfChanged.
That will also make it much easier to add more locking in the future.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
The function is being used in a number of places, notably container
removal and cleanup. While container removal already loops over EBUSY,
cleanup does not.
To make sure that all callers of Unmount get a fair chance of unmounting
cleanly, also loop there. I used the same values as containerd: 50
loops with 50ms sleeps.
Context: containers/podman/issues/11594
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
Return early instead of having a large body in nested branches.
Preserve the logic of returning ErrNotAContainer even on Get().
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
There is no public way to obtain implementations of these interfaces; so
replace the public interfaces with private ones, to make it clear that we
_can_ modify them.
For formal API compatibility, preserve the old interface definitions
as copies.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
It is not directly reachable by any public API, and there are no known
external users.
So, make it obviously private to make it easier to change the returned
interface in the future.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... to reduce duplication of read implementations of Store.
This can almost certainly be simplified further; this is enough
for the purposes of this PR, where we want to decrease the
number of callers that need to be updated.
Should not change behavior.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Daniel J Walsh