machine: add sparse file writer #21763
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
this is great work @giuseppe I am currently testing this ... code wise, LGTM |
giuseppe
force-pushed
the
sparse-file
branch
from
8f4eab0 to
9320bb2
Compare
|
circling back, seems to work nicely |
giuseppe
force-pushed
the
sparse-file
branch
from
9320bb2 to
3b0c391
Compare
|
Logic looks fine. Not sure about not letting the caller control the permissions bits. |
should we export |
Signed-off-by: Giuseppe Scrivano <[email protected]>
giuseppe
force-pushed
the
sparse-file
branch
from
3b0c391 to
0b86135
Compare
I've changed the API to export only newSparseWriterToFile and not have the method to create directly a file. That can be done by the caller. @baude does the new API work fine for you? |
|
it looks fine i think ... /hold |
openshift-ci
bot
added
do-not-merge/hold
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
There was a problem hiding this comment.
This LGTM as for correctness of the output. As for creating the holes exactly in the desired situations, that seems that it might be possible to improve.
Also it vaguely seems to me that we shouldn’t need 3 separate state variables, but I’m not sure about that part yet.
| } | ||
| } | ||
|
|
||
| m.buffer.Next(int(m.pos) - m.buffer.Len()) |
There was a problem hiding this comment.
I don’t understand what this is intended to do.
- AFAICS it always calls
Next(0), which does nothing - If it did something, moving the read offset would interfere with the
bytes.Equalensuring the expected output.
| return nil | ||
| } | ||
|
|
||
| func testInputWithWriteLen(t *testing.T, input []byte, chunkSize int) { |
There was a problem hiding this comment.
Ensuring the implementation correctly preserves data is definitely valuable; I’d also like the test to ensure that code does create the expected holes.
| // add "hello" in the middle | ||
| largeInput = make([]byte, 1024*1024) | ||
| copy(largeInput[len(largeInput)/2:], []byte("hello")) | ||
| testInput(t, largeInput) |
There was a problem hiding this comment.
A hole (small and large) in the middle is also a possible case to test.
| sw.file.Close() | ||
| return err | ||
| } | ||
| if sw.lastIsZero { |
There was a problem hiding this comment.
In many (all I think?) situations this code can create a 1-byte shorter hole in the first place, instead of having to seek back.
openshift-merge-bot
bot
merged commit 1d4651b
into
containers:main
|
Consider reformulating as https://github.com/mtrmac/libpod/tree/sparse ; AFAICS that’s exactly the same logic, but, I think, rather easier to prove correct. |
would you mind opening a PR with your fixes? |
#21797, for the record. And yes, it is exactly the same logic, apart from the handling of zeroes at the very end. |
stale-locking-app
bot
added
the
locked - please file new issue/PR
Does this PR introduce a user-facing change?