[Do-not-merge] Test the updated tar-split with podman.#28018
[Do-not-merge] Test the updated tar-split with podman.#28018jankaluza wants to merge 1 commit intocontainers:mainfrom
Conversation
Signed-off-by: Jan Kaluza <jkaluza@redhat.com>
|
The proper way to test things is using a go.mod replace |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
Some comments as part of looking at vbatts/tar-split#86 .
This is quite a bit of ceremony, and there is one other caller in the project.
So I suppose we’d want an internal/ utility to deal with all this… and would we then consider changing the API in tar-split instead, to make such a utility unnecessary?
| uncompressed, err := archive.DecompressStream(defragmented) | ||
| if err != nil { | ||
| return -1, err | ||
| } |
There was a problem hiding this comment.
It’s not obvious to me why the creation of uncompressed was moved. Is there a reason to do that? (“Aesthetics” could be a valid reason, I just want to know I’m not missing anything.)
There was a problem hiding this comment.
Trying to remember and I believe the reason was that this reduced one more place where I would have to call uncrompressed.Close() in case of error. If it would stay in its original place, I would have to call it also in the error handling block of NewLogger.
But reading your comment above about defer uncompressed.Close(), it could even stay where it is is that defer is done.
| return applyDriverFunc(payload) | ||
| // cleanup closes tsRdr, waits for tar-split to finish, then closes uncompressed. | ||
| // It returns the tar-split goroutine's error (if any). | ||
| var once sync.Once |
There was a problem hiding this comment.
(A cleanupDone bool would work, this does not need to handle concurrency.)
| } | ||
| defer idLogger.Close() // This must happen before uidLog and gidLog is consumed. | ||
|
|
||
| uncompressed, err := archive.DecompressStream(defragmented) |
There was a problem hiding this comment.
I’m a little unhappy about removing the defer uncompressed.Close() here.
AFAIK defers are processed in LIFO order, so that should still be fine. We rely on that for metadata vs. compressor already.
|
|
||
| size, applyErr := applyDriverFunc(tsRdr) | ||
|
|
||
| // Normal path: cleanup exactly once. |
There was a problem hiding this comment.
Critically, we must wait until writes to metadata and reads from uncompressed are truly done. I think that’s worth documenting.
|
@mtrmac , thanks for your feedback even here. I will see how vbatts/tar-split#86 ends up before updating this one, since there is no real blocker which would prove vbatts/tar-split#86 useless. |
|
A friendly reminder that this PR had no activity for 30 days. |
3 participants
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?