storage: cleanup staged layer if unused#2390
Conversation
mtrmac
left a comment
There was a problem hiding this comment.
Thanks!
This works, but it would be easy to forget next time again.
So I’m leaning towards
out, err := …
if err != nil { … }
succeeded := false
defer func() {
if !succeeded { … CleanupStagedLayer … }
}()
…
…diffOutputs = …
s.lock.Unlock()
succeeded = true
return …If you think that’s never going to be necessary, shrug, this PR is an improvement as is.
But, also, that code path should never be hit. Is that assumption incorrect?
5e5e2e9 to
41c9867
Compare
your version is better, I've fixed it and pushed a new version |
mtrmac
left a comment
There was a problem hiding this comment.
If I understand correctly, this is to follow the rules, allowing us to more precisely use locks in containers/storage#1916 .
But right now there’s containers/storage#1916 (comment) , so I’d prefer to wait a bit here until the that question is resolved.
|
containers/storage#1916 is merged, can we reconsider this? |
mtrmac
left a comment
There was a problem hiding this comment.
AFAICS, on return from ApplyDiffFromStagingDirectory, the callers can’t tell whether the lock is now unlocked.
But, also, the locks only exist for directories created by MkdirTemp, and the unlock code is ~idempotent, so extra calls of CleanupStagedLayer don’t actually hurt.
LGTM.
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
notify c/storage that the staged layer is not used and must be cleaned up on error. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
41c9867 to
77d7ff5
Compare
|
LGTM |
notify c/storage that the staged layer is not used and must be cleaned up on error.