Replace copySparse with go-qcow2reader/convert #2798
Conversation
|
|
||
| func (r *ProxyReaderAt) ReadAt(p []byte, off int64) (int, error) { | ||
| n, err := r.ReaderAt.ReadAt(p, off) | ||
| r.Bar.Add(n) |
There was a problem hiding this comment.
This should check negative n?
There was a problem hiding this comment.
This is documented to return positive value so I think we are safe.
WriteAt writes len(p) bytes from p to the underlying data stream at offset off. It returns the number of bytes written from p (0 <= n <= len(p)) and any error encountered that caused the write to stop early. WriteAt must return a non-nil error if it returns n < len(p).
If we get negative value the convert will fail here:
lima-vm/go-qcow2reader@2cdb233#diff-b4093fac13dc66eefdab36a0bd2f4ee72c635a84a9c5687f8fc3e33185c6d9aeR131
There was a problem hiding this comment.
If err is non-nil and n is negative, r.Bar.Add should not be called?
There was a problem hiding this comment.
If err is non-nil and n is negative
n is never negative
I pasted io.WriterAt docs before, but io.ReaderAt docs are the same:
ReadAt reads len(p) bytes into p starting at offset off in the underlying input source. It returns the number of bytes read (0 <= n <= len(p)) and any error encountered.
r.Bar.Add should not be called?
err may be io.EOF:
If the n = len(p) bytes returned by ReadAt are at the end of the input source, ReadAt may return either err == EOF or err == nil.
So I think always adding n and let the caller handle errors in the right thing. This is also what pb.Reader is doing: https://github.com/cheggaaa/pb/blob/4ca3463f4bcf4446e1746efc6e484e795cfe7b12/reader.go#L15
We can handle n == 0, but I'm not sure it is helpful to add a check that fail for all good reads, for case that happens only when err != nil, maybe once when per convert.
nirs
force-pushed
the
go-qcow2reader-convert
branch
from
5ab6a41 to
ad0bd28
Compare
|
Updated to build with latest version. |
go.mod
Outdated
| ) | ||
|
|
||
| // Temporary raplcement for testing https://github.com/lima-vm/go-qcow2reader/pull/36 | ||
| replace github.com/lima-vm/go-qcow2reader v0.2.1 => github.com/nirs/go-qcow2reader v0.0.0-20241025131821-46183d3fe6b2 |
There was a problem hiding this comment.
There was a problem hiding this comment.
go.mod was not updated yet, should I updated it in this PR?
There was a problem hiding this comment.
Add a commit to updated the dependency, can be removed if we go with the standard dependency update.
AkihiroSuda
left a comment
AkihiroSuda
left a comment
There was a problem hiding this comment.
LGTM after updating go.mod
To consume the new convert package. Signed-off-by: Nir Soffer <[email protected]>
With this converting the default ubuntu 24.10 qcow2 compressed image to
raw is 5.4 times faster:
Before:
% limactl create --tty=false
3.50 GiB / 3.50 GiB [-------------------------------------] 100.00% 317.58 MiB/s
After:
% limactl create --tty=false
3.50 GiB / 3.50 GiB [-------------------------------------] 100.00% 1.67 GiB/s
Signed-off-by: Nir Soffer <[email protected]>
nirs
force-pushed
the
go-qcow2reader-convert
branch
from
ad0bd28 to
0221c11
Compare
nirs
changed the title
2 participants
With this converting the default ubuntu 24.10 qcow2 compressed image to raw is 5.4 times faster:
Before:
After:
Fixes #2579