Merge pull request #373 from tri-adam/compaction

refactor: improve compaction logic

pkg/sif/create.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018-2023, Sylabs Inc. All rights reserved.
+// Copyright (c) 2018-2024, Sylabs Inc. All rights reserved.
 // Copyright (c) 2017, SingularityWare, LLC. All rights reserved.
 // Copyright (c) 2017, Yannick Cote <[email protected]> All rights reserved.
 // This software is licensed under a 3-clause BSD license. Please consult the
@@ -55,6 +55,20 @@ func writeDataObjectAt(ws io.WriteSeeker, offsetUnaligned int64, di DescriptorIn
 	return nil
 }
 
+// calculatedDataSize calculates the size of the data section based on the in-use descriptors.
+func (f *FileImage) calculatedDataSize() int64 {
+	dataEnd := f.DataOffset()
+
+	f.WithDescriptors(func(d Descriptor) bool {
+		if objectEnd := d.Offset() + d.Size(); dataEnd < objectEnd {
+			dataEnd = objectEnd
+		}
+		return false
+	})
+
+	return dataEnd - f.DataOffset()
+}
+
 var (
 	errInsufficientCapacity = errors.New("insufficient descriptor capacity to add data object(s) to image")
 	errPrimaryPartition     = errors.New("image already contains a primary partition")
@@ -80,6 +94,8 @@ func (f *FileImage) writeDataObject(i int, di DescriptorInput, t time.Time) erro
 	d := &f.rds[i]
 	d.ID = uint32(i) + 1
 
+	f.h.DataSize = f.calculatedDataSize()
+
 	if err := writeDataObjectAt(f.rw, f.h.DataOffset+f.h.DataSize, di, t, d); err != nil {
 		return err
 	}

pkg/sif/delete.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018-2023, Sylabs Inc. All rights reserved.
+// Copyright (c) 2018-2024, Sylabs Inc. All rights reserved.
 // Copyright (c) 2017, SingularityWare, LLC. All rights reserved.
 // Copyright (c) 2017, Yannick Cote <[email protected]> All rights reserved.
 // This software is licensed under a 3-clause BSD license. Please consult the
@@ -8,32 +8,16 @@
 package sif
 
 import (
-	"errors"
 	"fmt"
 	"io"
 	"time"
 )
 
-// isLast return true if the data object associated with d is the last in f.
-func (f *FileImage) isLast(d *rawDescriptor) bool {
-	isLast := true
-
-	end := d.Offset + d.Size
-	f.WithDescriptors(func(d Descriptor) bool {
-		isLast = d.Offset()+d.Size() <= end
-		return !isLast
-	})
-
-	return isLast
-}
-
 // zeroReader is an io.Reader that returns a stream of zero-bytes.
 type zeroReader struct{}
 
 func (zeroReader) Read(b []byte) (int, error) {
-	for i := range b {
-		b[i] = 0
-	}
+	clear(b)
 	return len(b), nil
 }
 
@@ -47,13 +31,6 @@ func (f *FileImage) zero(d *rawDescriptor) error {
 	return err
 }
 
-// truncateAt truncates f at the start of the padded data object described by d.
-func (f *FileImage) truncateAt(d *rawDescriptor) error {
-	start := d.Offset + d.Size - d.SizeWithPadding
-
-	return f.rw.Truncate(start)
-}
-
 // deleteOpts accumulates object deletion options.
 type deleteOpts struct {
 	zero    bool
@@ -97,8 +74,6 @@ func OptDeleteWithTime(t time.Time) DeleteOpt {
 	}
 }
 
-var errCompactNotImplemented = errors.New("compact not implemented for non-last object")
-
 // DeleteObject deletes the data object with id, according to opts.
 //
 // To zero the data region of the deleted object, use OptDeleteZero. To compact the file following
@@ -125,24 +100,12 @@ func (f *FileImage) DeleteObject(id uint32, opts ...DeleteOpt) error {
 		return fmt.Errorf("%w", err)
 	}
 
-	if do.compact && !f.isLast(d) {
-		return fmt.Errorf("%w", errCompactNotImplemented)
-	}
-
 	if do.zero {
 		if err := f.zero(d); err != nil {
 			return fmt.Errorf("%w", err)
 		}
 	}
 
-	if do.compact {
-		if err := f.truncateAt(d); err != nil {
-			return fmt.Errorf("%w", err)
-		}
-
-		f.h.DataSize -= d.SizeWithPadding
-	}
-
 	f.h.DescriptorsFree++
 	f.h.ModifiedAt = do.t.Unix()
 
@@ -156,6 +119,14 @@ func (f *FileImage) DeleteObject(id uint32, opts ...DeleteOpt) error {
 	// Reset rawDescripter with empty struct
 	*d = rawDescriptor{}
 
+	if do.compact {
+		f.h.DataSize = f.calculatedDataSize()
+
+		if err := f.rw.Truncate(f.h.DataOffset + f.h.DataSize); err != nil {
+			return fmt.Errorf("%w", err)
+		}
+	}
+
 	if err := f.writeDescriptors(); err != nil {
 		return fmt.Errorf("%w", err)
 	}

pkg/sif/delete_test.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018-2023, Sylabs Inc. All rights reserved.
+// Copyright (c) 2018-2024, Sylabs Inc. All rights reserved.
 // This software is licensed under a 3-clause BSD license. Please consult the
 // LICENSE file distributed with the sources of this project regarding your
 // rights to use or distribute this software.
@@ -17,7 +17,7 @@ func TestDeleteObject(t *testing.T) {
 	tests := []struct {
 		name       string
 		createOpts []CreateOpt
-		id         uint32
+		ids        []uint32
 		opts       []DeleteOpt
 		wantErr    error
 	}{
@@ -26,44 +26,104 @@ func TestDeleteObject(t *testing.T) {
 			createOpts: []CreateOpt{
 				OptCreateDeterministic(),
 			},
-			id:      1,
+			ids:     []uint32{1},
 			wantErr: ErrObjectNotFound,
 		},
 		{
-			name: "Zero",
+			name: "Compact",
+			createOpts: []CreateOpt{
+				OptCreateDeterministic(),
+				OptCreateWithDescriptors(
+					getDescriptorInput(t, DataGeneric, []byte{0xfa, 0xce}),
+					getDescriptorInput(t, DataGeneric, []byte{0xfe, 0xed}),
+				),
+			},
+			ids: []uint32{1, 2},
+			opts: []DeleteOpt{
+				OptDeleteCompact(true),
+			},
+		},
+		{
+			name: "OneZero",
 			createOpts: []CreateOpt{
 				OptCreateDeterministic(),
 				OptCreateWithDescriptors(
 					getDescriptorInput(t, DataGeneric, []byte{0xfa, 0xce}),
+					getDescriptorInput(t, DataGeneric, []byte{0xfe, 0xed}),
 				),
 			},
-			id: 1,
+			ids: []uint32{1},
 			opts: []DeleteOpt{
 				OptDeleteZero(true),
 			},
 		},
 		{
-			name: "Compact",
+			name: "OneCompact",
 			createOpts: []CreateOpt{
 				OptCreateDeterministic(),
 				OptCreateWithDescriptors(
 					getDescriptorInput(t, DataGeneric, []byte{0xfa, 0xce}),
+					getDescriptorInput(t, DataGeneric, []byte{0xfe, 0xed}),
 				),
 			},
-			id: 1,
+			ids: []uint32{1},
 			opts: []DeleteOpt{
 				OptDeleteCompact(true),
 			},
 		},
 		{
-			name: "ZeroCompact",
+			name: "OneZeroCompact",
+			createOpts: []CreateOpt{
+				OptCreateDeterministic(),
+				OptCreateWithDescriptors(
+					getDescriptorInput(t, DataGeneric, []byte{0xfa, 0xce}),
+					getDescriptorInput(t, DataGeneric, []byte{0xfe, 0xed}),
+				),
+			},
+			ids: []uint32{1},
+			opts: []DeleteOpt{
+				OptDeleteZero(true),
+				OptDeleteCompact(true),
+			},
+		},
+		{
+			name: "TwoZero",
+			createOpts: []CreateOpt{
+				OptCreateDeterministic(),
+				OptCreateWithDescriptors(
+					getDescriptorInput(t, DataGeneric, []byte{0xfa, 0xce}),
+					getDescriptorInput(t, DataGeneric, []byte{0xfe, 0xed}),
+				),
+			},
+			ids: []uint32{2},
+			opts: []DeleteOpt{
+				OptDeleteZero(true),
+			},
+		},
+		{
+			name: "TwoCompact",
+			createOpts: []CreateOpt{
+				OptCreateDeterministic(),
+				OptCreateWithDescriptors(
+					getDescriptorInput(t, DataGeneric, []byte{0xfa, 0xce}),
+					getDescriptorInput(t, DataGeneric, []byte{0xfe, 0xed}),
+				),
+			},
+			ids: []uint32{2},
+			opts: []DeleteOpt{
+				OptDeleteCompact(true),
+			},
+		},
+		{
+			name: "TwoZeroCompact",
 			createOpts: []CreateOpt{
 				OptCreateDeterministic(),
 				OptCreateWithDescriptors(
 					getDescriptorInput(t, DataGeneric, []byte{0xfa, 0xce}),
+					getDescriptorInput(t, DataGeneric, []byte{0xfe, 0xed}),
 				),
 			},
-			id: 1,
+			ids: []uint32{2},
 			opts: []DeleteOpt{
 				OptDeleteZero(true),
 				OptDeleteCompact(true),
@@ -78,7 +138,7 @@ func TestDeleteObject(t *testing.T) {
 				),
 				OptCreateWithTime(time.Unix(946702800, 0)),
 			},
-			id: 1,
+			ids: []uint32{1},
 			opts: []DeleteOpt{
 				OptDeleteDeterministic(),
 			},
@@ -91,7 +151,7 @@ func TestDeleteObject(t *testing.T) {
 					getDescriptorInput(t, DataGeneric, []byte{0xfa, 0xce}),
 				),
 			},
-			id: 1,
+			ids: []uint32{1},
 			opts: []DeleteOpt{
 				OptDeleteWithTime(time.Unix(946702800, 0)),
 			},
@@ -106,7 +166,7 @@ func TestDeleteObject(t *testing.T) {
 					),
 				),
 			},
-			id: 1,
+			ids: []uint32{1},
 		},
 	}
 
@@ -119,8 +179,10 @@ func TestDeleteObject(t *testing.T) {
 				t.Fatal(err)
 			}
 
-			if got, want := f.DeleteObject(tt.id, tt.opts...), tt.wantErr; !errors.Is(got, want) {
-				t.Errorf("got error %v, want %v", got, want)
+			for _, id := range tt.ids {
+				if got, want := f.DeleteObject(id, tt.opts...), tt.wantErr; !errors.Is(got, want) {
+					t.Errorf("got error %v, want %v", got, want)
+				}
 			}
 
 			if err := f.UnloadContainer(); err != nil {