-
Notifications
You must be signed in to change notification settings - Fork 789
fix: Remove premature return after closing sparse diff file so Convert executes. #4421
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,23 +38,23 @@ func EnsureDisk(ctx context.Context, instDir, diskSize string, diskImageFormat i | |
| if err != nil { | ||
| return err | ||
| } | ||
| destDisk := baseDisk | ||
| if isBaseDiskISO { | ||
| // Create an empty data volume (sparse) | ||
| destDisk = diffDisk | ||
|
|
||
| // Create an empty data volume for the diff disk | ||
| diffDiskF, err := os.Create(diffDisk) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| err = diskUtil.MakeSparse(ctx, diffDiskF, 0) | ||
| if err != nil { | ||
| diffDiskF.Close() | ||
| return fmt.Errorf("failed to create sparse diff disk %q: %w", diffDisk, err) | ||
| if err = diffDiskF.Close(); err != nil { | ||
| return err | ||
| } | ||
| return diffDiskF.Close() | ||
| } | ||
| // Check whether to use ASIF format | ||
|
|
||
| if err = diskUtil.Convert(ctx, diskImageFormat, baseDisk, diffDisk, &diskSizeInBytes, false); err != nil { | ||
| if err = diskUtil.Convert(ctx, diskImageFormat, destDisk, diffDisk, &diskSizeInBytes, false); err != nil { | ||
| return fmt.Errorf("failed to convert %q to a disk %q: %w", baseDisk, diffDisk, err) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This error message has to be updated too |
||
| } | ||
| return err | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,150 @@ | ||
| // SPDX-FileCopyrightText: Copyright The Lima Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package driverutil | ||
|
|
||
| import ( | ||
| "crypto/sha256" | ||
| "encoding/hex" | ||
| "os" | ||
| "path/filepath" | ||
| "runtime" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/lima-vm/go-qcow2reader" | ||
| "github.com/lima-vm/go-qcow2reader/image" | ||
| "gotest.tools/v3/assert" | ||
|
|
||
| "github.com/lima-vm/lima/v2/pkg/iso9660util" | ||
| "github.com/lima-vm/lima/v2/pkg/limatype/filenames" | ||
| "github.com/lima-vm/lima/v2/pkg/osutil" | ||
| ) | ||
|
|
||
| const ( | ||
| typeRAW = image.Type("raw") | ||
| typeASIF = image.Type("asif") | ||
| ) | ||
|
|
||
| func writeMinimalISO(t *testing.T, path string) { | ||
| t.Helper() | ||
| entries := []iso9660util.Entry{ | ||
| {Path: "/hello.txt", Reader: strings.NewReader("hello world")}, | ||
| } | ||
| assert.NilError(t, iso9660util.Write(path, "TESTISO", entries)) | ||
| } | ||
|
|
||
| func writeNonISO(t *testing.T, path string) { | ||
| t.Helper() | ||
| size := 64 * 1024 | ||
| buf := make([]byte, size) | ||
| copy(buf[0x8001:], "XXXXX") | ||
| assert.NilError(t, os.WriteFile(path, buf, 0o644)) | ||
| } | ||
|
|
||
| func sha256File(t *testing.T, path string) string { | ||
| t.Helper() | ||
| b, err := os.ReadFile(path) | ||
| assert.NilError(t, err) | ||
| sum := sha256.Sum256(b) | ||
| return hex.EncodeToString(sum[:]) | ||
| } | ||
|
|
||
| func detectImageType(t *testing.T, path string) image.Type { | ||
| t.Helper() | ||
| f, err := os.Open(path) | ||
| assert.NilError(t, err) | ||
| defer f.Close() | ||
| img, err := qcow2reader.Open(f) | ||
| assert.NilError(t, err) | ||
| return img.Type() | ||
| } | ||
|
|
||
| func checkDisk(t *testing.T, diff string, expectedType image.Type) { | ||
| t.Helper() | ||
| fi, err := os.Stat(diff) | ||
| assert.NilError(t, err) | ||
| assert.Assert(t, fi.Size() > 0) | ||
| assert.Equal(t, detectImageType(t, diff), expectedType) | ||
| } | ||
|
|
||
| func isMacOS26OrHigher() bool { | ||
| if runtime.GOOS != "darwin" { | ||
| return false | ||
| } | ||
| version, err := osutil.ProductVersion() | ||
| if err != nil { | ||
| return false | ||
| } | ||
| return version.Major >= 26 | ||
| } | ||
|
|
||
| func TestEnsureDisk_WithISOBaseImage(t *testing.T) { | ||
| instDir := t.TempDir() | ||
| base := filepath.Join(instDir, filenames.BaseDisk) | ||
| diff := filepath.Join(instDir, filenames.DiffDisk) | ||
|
|
||
| writeMinimalISO(t, base) | ||
| isISO, err := iso9660util.IsISO9660(base) | ||
| assert.NilError(t, err) | ||
| assert.Assert(t, isISO) | ||
| baseHashBefore := sha256File(t, base) | ||
|
|
||
| formats := []image.Type{typeRAW} | ||
| if isMacOS26OrHigher() { | ||
| formats = append(formats, typeASIF) | ||
| } | ||
|
|
||
| for _, format := range formats { | ||
| assert.NilError(t, EnsureDisk(t.Context(), instDir, "2MiB", format)) | ||
| isISO, err = iso9660util.IsISO9660(base) | ||
| assert.NilError(t, err) | ||
| assert.Assert(t, isISO) | ||
| assert.Equal(t, baseHashBefore, sha256File(t, base)) | ||
| checkDisk(t, diff, format) | ||
| assert.NilError(t, os.Remove(diff)) | ||
| } | ||
| } | ||
|
|
||
| func TestEnsureDisk_WithNonISOBaseImage(t *testing.T) { | ||
| instDir := t.TempDir() | ||
| base := filepath.Join(instDir, filenames.BaseDisk) | ||
| diff := filepath.Join(instDir, filenames.DiffDisk) | ||
|
|
||
| writeNonISO(t, base) | ||
| isISO, err := iso9660util.IsISO9660(base) | ||
| assert.NilError(t, err) | ||
| assert.Assert(t, !isISO) | ||
|
|
||
| formats := []image.Type{typeRAW} | ||
| if isMacOS26OrHigher() { | ||
| formats = append(formats, typeASIF) | ||
| } | ||
|
|
||
| for _, format := range formats { | ||
| assert.NilError(t, EnsureDisk(t.Context(), instDir, "2MiB", format)) | ||
| checkDisk(t, diff, format) | ||
| assert.NilError(t, os.Remove(diff)) | ||
| } | ||
| } | ||
|
|
||
| func TestEnsureDisk_ExistingDiffDisk(t *testing.T) { | ||
| instDir := t.TempDir() | ||
| base := filepath.Join(instDir, filenames.BaseDisk) | ||
| diff := filepath.Join(instDir, filenames.DiffDisk) | ||
|
|
||
| writeNonISO(t, base) | ||
|
|
||
| formats := []image.Type{typeRAW} | ||
| if isMacOS26OrHigher() { | ||
| formats = append(formats, typeASIF) | ||
| } | ||
|
|
||
| for _, format := range formats { | ||
| assert.NilError(t, os.WriteFile(diff, []byte("preexisting"), 0o644)) | ||
| origHash := sha256File(t, diff) | ||
| assert.NilError(t, EnsureDisk(t.Context(), instDir, "2MiB", format)) | ||
| assert.Equal(t, sha256File(t, diff), origHash) | ||
| assert.NilError(t, os.Remove(diff)) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -62,10 +62,10 @@ func convertTo(destType image.Type, source, dest string, size *int64, allowSourc | |||||||
| logrus.Infof("Converting %q (%s) to a %s disk %q", source, srcImg.Type(), destType, dest) | ||||||||
| switch t := srcImg.Type(); t { | ||||||||
| case raw.Type: | ||||||||
| if err = srcF.Close(); err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
srcImg.size() was giving the value -1. internally it was not able to run the syscall os.stat because it was being closed early when the close statement was outside the if statement. This change ensures that the the file is open when trying to get scrImg.Size() and that the value is not -1.
The above defer statements already take care of closing the file after processing. |
||||||||
| if destType == raw.Type { | ||||||||
| if err = srcF.Close(); err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
| return convertRawToRaw(source, dest, size) | ||||||||
| } | ||||||||
| case qcow2.Type: | ||||||||
|
|
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is hard to read, as
destDiskis passed assourcelima/pkg/imgutil/manager.go
Lines 21 to 22 in 77d181a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.