Skip to content

Replace copySparse with go-qcow2reader/convert #2798

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 25, 2024
Merged

Replace copySparse with go-qcow2reader/convert #2798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6fb3d49
Update go-qcow2reader to v0.3.0
nirs Oct 25, 2024
0221c11
Replace copySparse with go-qcow2reader/convert
nirs Oct 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ require (
github.com/google/go-cmp v0.6.0
github.com/google/yamlfmt v0.13.0
github.com/invopop/jsonschema v0.12.0
github.com/lima-vm/go-qcow2reader v0.2.1
github.com/lima-vm/go-qcow2reader v0.3.0
github.com/lima-vm/sshocker v0.3.4
github.com/mattn/go-isatty v0.0.20
github.com/mattn/go-shellwords v1.0.12
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,8 @@ github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
github.com/leodido/go-urn v1.2.0 h1:hpXL4XnriNwQ/ABnpepYM/1vCLWNDfUNts8dX3xTG6Y=
github.com/leodido/go-urn v1.2.0/go.mod h1:+8+nEpDfqqsY+g338gtMEUOtuK+4dEMhiQEgxpxOKII=
github.com/lima-vm/go-qcow2reader v0.2.1 h1:aeusQHn4m+uNhf3Z4oBUaE+xpMsNHjNCMkKUbhEgbXU=
github.com/lima-vm/go-qcow2reader v0.2.1/go.mod h1:e3p29BzLT8hNh4jbLezdFAHU4eBijf0bm5GilStCRKE=
github.com/lima-vm/go-qcow2reader v0.3.0 h1:7nDU29oXPNymm/UMMfqN8bYXejlhxEYIHO76RRUzbbc=
github.com/lima-vm/go-qcow2reader v0.3.0/go.mod h1:e3p29BzLT8hNh4jbLezdFAHU4eBijf0bm5GilStCRKE=
github.com/lima-vm/sshocker v0.3.4 h1:5rn6vMkfqwZSZiBW+Udo505OIRhPB4xbLUDdEnFgWwI=
github.com/lima-vm/sshocker v0.3.4/go.mod h1:QT4c7XNmeQTv79h5/8EgiS7U51B9BLenlXV7idCY0tE=
github.com/linuxkit/virtsock v0.0.0-20220523201153-1a23e78aa7a2 h1:DZMFueDbfz6PNc1GwDRA8+6lBx1TB9UnxDQliCqR73Y=
Expand Down
50 changes: 8 additions & 42 deletions pkg/nativeimgutil/nativeimgutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
package nativeimgutil

import (
"bytes"
"errors"
"fmt"
"io"
"os"
Expand All @@ -12,6 +10,7 @@ import (
"github.com/containerd/continuity/fs"
"github.com/docker/go-units"
"github.com/lima-vm/go-qcow2reader"
"github.com/lima-vm/go-qcow2reader/convert"
"github.com/lima-vm/go-qcow2reader/image/qcow2"
"github.com/lima-vm/go-qcow2reader/image/raw"
"github.com/lima-vm/lima/pkg/progressbar"
Expand Down Expand Up @@ -75,17 +74,20 @@ func ConvertToRaw(source, dest string, size *int64, allowSourceWithBackingFile b
}

// Copy
srcImgR := io.NewSectionReader(srcImg, 0, srcImg.Size())
bar, err := progressbar.New(srcImg.Size())
if err != nil {
return err
}
const bufSize = 1024 * 1024
conv, err := convert.New(convert.Options{})
if err != nil {
return err
}
bar.Start()
copied, err := copySparse(destTmpF, bar.NewProxyReader(srcImgR), bufSize)
pra := progressbar.ProxyReaderAt{ReaderAt: srcImg, Bar: bar}
err = conv.Convert(destTmpF, &pra, srcImg.Size())
bar.Finish()
if err != nil {
return fmt.Errorf("failed to call copySparse(), bufSize=%d, copied=%d: %w", bufSize, copied, err)
return fmt.Errorf("failed to convert image: %w", err)
}

// Resize
Expand Down Expand Up @@ -128,42 +130,6 @@ func convertRawToRaw(source, dest string, size *int64) error {
return nil
}

func copySparse(w *os.File, r io.Reader, bufSize int64) (int64, error) {
var (
n int64
eof bool
)

zeroBuf := make([]byte, bufSize)
buf := make([]byte, bufSize)
for !eof {
rN, rErr := r.Read(buf)
if rErr != nil {
eof = errors.Is(rErr, io.EOF)
if !eof {
return n, fmt.Errorf("failed to read: %w", rErr)
}
}
// TODO: qcow2reader should have a method to notify whether buf is zero
if bytes.Equal(buf[:rN], zeroBuf[:rN]) {
n += int64(rN)
} else {
wN, wErr := w.WriteAt(buf[:rN], n)
if wN > 0 {
n += int64(wN)
}
if wErr != nil {
return n, fmt.Errorf("failed to write: %w", wErr)
}
if wN != rN {
return n, fmt.Errorf("read %d, but wrote %d bytes", rN, wN)
}
}
}

return n, nil
}

func MakeSparse(f *os.File, n int64) error {
if _, err := f.Seek(n, io.SeekStart); err != nil {
return err
Expand Down
12 changes: 12 additions & 0 deletions pkg/progressbar/progressbar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package progressbar

import (
"io"
"os"
"time"

Expand All @@ -9,6 +10,17 @@ import (
"github.com/sirupsen/logrus"
)

type ProxyReaderAt struct {
io.ReaderAt
Bar *pb.ProgressBar
}

func (r *ProxyReaderAt) ReadAt(p []byte, off int64) (int, error) {
n, err := r.ReaderAt.ReadAt(p, off)
r.Bar.Add(n)
Copy link
Member

@AkihiroSuda AkihiroSuda Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should check negative n?

Copy link
Member Author

@nirs nirs Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@AkihiroSuda AkihiroSuda Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If err is non-nil and n is negative, r.Bar.Add should not be called?

Copy link
Member Author

@nirs nirs Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return n, err
}

func New(size int64) (*pb.ProgressBar, error) {
bar := pb.New64(size)

Expand Down
Loading