From e8dad67f88b7af7d7d3d4933d1c301428c17a319 Mon Sep 17 00:00:00 2001 From: Adam Hughes <9903835+tri-adam@users.noreply.github.com> Date: Thu, 4 Jul 2024 19:54:32 +0000 Subject: [PATCH] refactor: improve delete compaction logic Improve compaction via OptDeleteCompact. Do not fail when compaction is requested on a data object that is not at the end of the data section. When compaction is requested, always calculate the end of the data section based on the in-use descriptors. --- pkg/sif/delete.go | 49 ++-------- pkg/sif/delete_test.go | 90 +++++++++++++++--- .../TestDeleteObject/OneCompact.golden | Bin 0 -> 32180 bytes .../testdata/TestDeleteObject/OneZero.golden | Bin 0 -> 32180 bytes .../TestDeleteObject/OneZeroCompact.golden | Bin 0 -> 32180 bytes .../{Zero.golden => TwoCompact.golden} | Bin 32178 -> 32178 bytes .../testdata/TestDeleteObject/TwoZero.golden | Bin 0 -> 32180 bytes ...roCompact.golden => TwoZeroCompact.golden} | Bin 32176 -> 32178 bytes 8 files changed, 86 insertions(+), 53 deletions(-) create mode 100644 pkg/sif/testdata/TestDeleteObject/OneCompact.golden create mode 100644 pkg/sif/testdata/TestDeleteObject/OneZero.golden create mode 100644 pkg/sif/testdata/TestDeleteObject/OneZeroCompact.golden rename pkg/sif/testdata/TestDeleteObject/{Zero.golden => TwoCompact.golden} (99%) create mode 100644 pkg/sif/testdata/TestDeleteObject/TwoZero.golden rename pkg/sif/testdata/TestDeleteObject/{ZeroCompact.golden => TwoZeroCompact.golden} (99%) diff --git a/pkg/sif/delete.go b/pkg/sif/delete.go index c0f0aba3..41c0a726 100644 --- a/pkg/sif/delete.go +++ b/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 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) } diff --git a/pkg/sif/delete_test.go b/pkg/sif/delete_test.go index 683e00dd..a7c2a165 100644 --- a/pkg/sif/delete_test.go +++ b/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 { diff --git a/pkg/sif/testdata/TestDeleteObject/OneCompact.golden b/pkg/sif/testdata/TestDeleteObject/OneCompact.golden new file mode 100644 index 0000000000000000000000000000000000000000..cb86f62d5719233811f5bdc7e2527fed9897d562 GIT binary patch literal 32180 zcmeI$u?fOZ5C-6j2x8|ZJ~vPVC54p(IDu|p?>OQhZX#%FVPeQj8o|y&_^x|ANBwM literal 0 HcmV?d00001 diff --git a/pkg/sif/testdata/TestDeleteObject/OneZero.golden b/pkg/sif/testdata/TestDeleteObject/OneZero.golden new file mode 100644 index 0000000000000000000000000000000000000000..ec7f18be315a571430b57f408ff038f1365911de GIT binary patch literal 32180 zcmeI$u?Ye}5CzaV5yZ?+JUdVXFM%JDMiV7nHVN#xp6&~Jm(yjIYEE`0RjXF5FkK+009C7 z2oNCfCjygYL^W!cts)-X&bM;u>3UuD*!D1PBlyK!5-N0t5&UAV7cs0RjXF z5FkK+009C72oNAZfB*pk1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+009C72oNAZfB*pk z1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+009C72oNAZfB*pk1PBlyK!5-N0zVOW-QQ7d B9xea? literal 0 HcmV?d00001 diff --git a/pkg/sif/testdata/TestDeleteObject/OneZeroCompact.golden b/pkg/sif/testdata/TestDeleteObject/OneZeroCompact.golden new file mode 100644 index 0000000000000000000000000000000000000000..ec7f18be315a571430b57f408ff038f1365911de GIT binary patch literal 32180 zcmeI$u?Ye}5CzaV5yZ?+JUdVXFM%JDMiV7nHVN#xp6&~Jm(yjIYEE`0RjXF5FkK+009C7 z2oNCfCjygYL^W!cts)-X&bM;u>3UuD*!D1PBlyK!5-N0t5&UAV7cs0RjXF z5FkK+009C72oNAZfB*pk1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+009C72oNAZfB*pk z1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+009C72oNAZfB*pk1PBlyK!5-N0zVOW-QQ7d B9xea? literal 0 HcmV?d00001 diff --git a/pkg/sif/testdata/TestDeleteObject/Zero.golden b/pkg/sif/testdata/TestDeleteObject/TwoCompact.golden similarity index 99% rename from pkg/sif/testdata/TestDeleteObject/Zero.golden rename to pkg/sif/testdata/TestDeleteObject/TwoCompact.golden index 036b3690a02c7a69c1fd86dd0562ce64f7395f3f..74b6489cc62b849d79558a5c92de01b40fb09315 100644 GIT binary patch delta 79 zcmdn=n{m@`#t8w8`Wpk+^INhzFfcGOG5`q%h7UlBVM8s5WME)|P;i=oGq2<0|NsAC M^v1^WT&7>=0O*$%ivR!s delta 22 ecmdn=n{m@`#t8w81{(v{^KT9iNXVTyAqW6@&Z4c&nIpMt1Utdv3nV{D9z6KE z7x8@^o89TK**Dv$S5eoW#q-PU*>zo%T$YNla*-d}bIEJYX{i$g2oNAZfB*pk1PBly zK!CvS1!n7rs!Dy*e)R3yGN4cTOIkw?1OAx#+ev@`0RjXF5FkK+009C72oNAZfB*pk z1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+009C72oNAZfB*pk1PBlyK!5-N0t5&UAV7cs z0RjXF5FkK+009C72oNAZfB*pk1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+0D*rMc-3apfith; R