From c29e34100d6d34daea6673ebacbea9f1a6639ced Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Guilherme=20de=20Campos=20Magalh=C3=A3es?= Date: Thu, 9 Jan 2020 16:03:31 -0300 Subject: [PATCH] introduce OpenWithMode() (#1) * introduce OpenWithMode() Signed-off-by: Guilherme Magalhaes * add tests with readonly disks for write operations Signed-off-by: Guilherme Magalhaes --- disk/disk.go | 13 ++++ disk/disk_test.go | 157 +++++++++++++++++++++++++++++----------------- diskfs.go | 87 ++++++++++++++++++++++--- diskfs_test.go | 25 ++++++-- 4 files changed, 210 insertions(+), 72 deletions(-) diff --git a/disk/disk.go b/disk/disk.go index 4aad59ec..99a1392a 100644 --- a/disk/disk.go +++ b/disk/disk.go @@ -25,6 +25,7 @@ type Disk struct { LogicalBlocksize int64 PhysicalBlocksize int64 Table partition.Table + Writable bool } // Type represents the type of disk this is @@ -37,6 +38,10 @@ const ( Device ) +var ( + errIncorrectOpenMode = errors.New("disk file or device not open for write") +) + // GetPartitionTable retrieves a PartitionTable for a Disk // // returns an error if the Disk is invalid or does not exist, or the partition table is unknown @@ -51,6 +56,9 @@ func (d *Disk) GetPartitionTable() (partition.Table, error) { // // Actual writing of the table is delegated to the individual implementation func (d *Disk) Partition(table partition.Table) error { + if !d.Writable { + return errIncorrectOpenMode + } // fill in the uuid err := table.Write(d.File, d.Info.Size()) if err != nil { @@ -67,6 +75,9 @@ func (d *Disk) Partition(table partition.Table) error { // returns an error if there was an error writing to the disk, reading from the reader, the table // is invalid, or the partition is invalid func (d *Disk) WritePartitionContents(partition int, reader io.Reader) (int64, error) { + if !d.Writable { + return -1, errIncorrectOpenMode + } if d.Table == nil { return -1, fmt.Errorf("cannot write contents of a partition on a disk without a partition table") } @@ -122,6 +133,8 @@ func (d *Disk) CreateFilesystem(spec FilesystemSpec) (filesystem.FileSystem, err err error ) switch { + case !d.Writable: + return nil, errIncorrectOpenMode case spec.Partition == 0: size = d.Size start = 0 diff --git a/disk/disk_test.go b/disk/disk_test.go index 2e8ec43b..1a91c4ce 100644 --- a/disk/disk_test.go +++ b/disk/disk_test.go @@ -69,6 +69,7 @@ func TestGetPartitionTable(t *testing.T) { File: f, LogicalBlocksize: 512, PhysicalBlocksize: 512, + Writable: false, } table, err := d.GetPartitionTable() @@ -109,6 +110,7 @@ func TestPartition(t *testing.T) { LogicalBlocksize: 512, PhysicalBlocksize: 512, Info: fileInfo, + Writable: true, } // this is partition start and end in sectors, not bytes sectorSize := 512 @@ -150,6 +152,7 @@ func TestPartition(t *testing.T) { LogicalBlocksize: 512, PhysicalBlocksize: 512, Info: fileInfo, + Writable: true, } // this is partition start and end in sectors, not bytes sectorSize := 512 @@ -167,71 +170,94 @@ func TestPartition(t *testing.T) { t.Errorf("Unexpected err: %v", err) } }) + t.Run("readonly", func(t *testing.T) { + d := &disk.Disk{ + Writable: false, + } + expectedErr := fmt.Errorf("disk file or device not open for write") + err := d.Partition(&mbr.Table{}) + if err.Error() != expectedErr.Error() { + t.Errorf("Mismatched error, actual '%v', expected '%v'", err, expectedErr) + } + }) } func TestWritePartitionContents(t *testing.T) { - oneMB := uint64(1024 * 1024) - partitionStart := uint64(2048) - partitionSize := uint64(5) * oneMB - partitionEnd := partitionStart + partitionSize/512 - 1 - table := &gpt.Table{ - Partitions: []*gpt.Partition{ - {Start: 2048, End: partitionEnd, Type: gpt.EFISystemPartition, Name: "EFI System"}, - }, - LogicalSectorSize: 512, - } - tests := []struct { - table partition.Table - partition int - err error - }{ - // various invalid table scenarios - {nil, 1, fmt.Errorf("cannot write contents of a partition on a disk without a partition table")}, - {nil, 0, fmt.Errorf("cannot write contents of a partition on a disk without a partition table")}, - {nil, -1, fmt.Errorf("cannot write contents of a partition on a disk without a partition table")}, - // invalid partition number scenarios - {table, -1, fmt.Errorf("cannot write contents of a partition without specifying a partition")}, - {table, 5, fmt.Errorf("Requested partition %d but only have %d partitions", 5, 1)}, - {table, 1, nil}, - } - for _, tt := range tests { - f, err := tmpDisk("") - if err != nil { - t.Fatalf("Error creating new temporary disk: %v", err) + t.Run("gpt", func(t *testing.T) { + oneMB := uint64(1024 * 1024) + partitionStart := uint64(2048) + partitionSize := uint64(5) * oneMB + partitionEnd := partitionStart + partitionSize/512 - 1 + table := &gpt.Table{ + Partitions: []*gpt.Partition{ + {Start: 2048, End: partitionEnd, Type: gpt.EFISystemPartition, Name: "EFI System"}, + }, + LogicalSectorSize: 512, } - defer f.Close() - - if keepTmpFiles { - defer os.Remove(f.Name()) - } else { - fmt.Println(f.Name()) + tests := []struct { + table partition.Table + partition int + err error + }{ + // various invalid table scenarios + {nil, 1, fmt.Errorf("cannot write contents of a partition on a disk without a partition table")}, + {nil, 0, fmt.Errorf("cannot write contents of a partition on a disk without a partition table")}, + {nil, -1, fmt.Errorf("cannot write contents of a partition on a disk without a partition table")}, + // invalid partition number scenarios + {table, -1, fmt.Errorf("cannot write contents of a partition without specifying a partition")}, + {table, 5, fmt.Errorf("Requested partition %d but only have %d partitions", 5, 1)}, + {table, 1, nil}, } + for _, tt := range tests { + f, err := tmpDisk("") + if err != nil { + t.Fatalf("Error creating new temporary disk: %v", err) + } + defer f.Close() - fileInfo, err := f.Stat() - if err != nil { - t.Fatalf("Error reading info on temporary disk: %v", err) - } + if keepTmpFiles { + defer os.Remove(f.Name()) + } else { + fmt.Println(f.Name()) + } + fileInfo, err := f.Stat() + if err != nil { + t.Fatalf("Error reading info on temporary disk: %v", err) + } + + d := &disk.Disk{ + File: f, + LogicalBlocksize: 512, + PhysicalBlocksize: 512, + Info: fileInfo, + Table: tt.table, + Writable: true, + } + b := make([]byte, partitionSize, partitionSize) + rand.Read(b) + reader := bytes.NewReader(b) + written, err := d.WritePartitionContents(tt.partition, reader) + switch { + case (err == nil && tt.err != nil) || (err != nil && tt.err == nil) || (err != nil && tt.err != nil && !strings.HasPrefix(err.Error(), tt.err.Error())): + t.Errorf("mismatched error, actual then expected") + t.Logf("%v", err) + t.Logf("%v", tt.err) + case tt.err != nil && written > 0: + t.Errorf("Unexpectedly wrote %d bytes, expected 0", written) + } + } + }) + t.Run("readonly", func(t *testing.T) { d := &disk.Disk{ - File: f, - LogicalBlocksize: 512, - PhysicalBlocksize: 512, - Info: fileInfo, - Table: tt.table, - } - b := make([]byte, partitionSize, partitionSize) - rand.Read(b) - reader := bytes.NewReader(b) - written, err := d.WritePartitionContents(tt.partition, reader) - switch { - case (err == nil && tt.err != nil) || (err != nil && tt.err == nil) || (err != nil && tt.err != nil && !strings.HasPrefix(err.Error(), tt.err.Error())): - t.Errorf("mismatched error, actual then expected") - t.Logf("%v", err) - t.Logf("%v", tt.err) - case tt.err != nil && written > 0: - t.Errorf("Unexpectedly wrote %d bytes, expected 0", written) + Writable: false, } - } + expectedErr := fmt.Errorf("disk file or device not open for write") + _, err := d.WritePartitionContents(0, nil) + if err.Error() != expectedErr.Error() { + t.Errorf("mismatched error, actual '%v' expected '%v'", err, expectedErr) + } + }) } func TestReadPartitionContents(t *testing.T) { @@ -286,6 +312,7 @@ func TestReadPartitionContents(t *testing.T) { PhysicalBlocksize: 512, Info: fileInfo, Table: tt.table, + Writable: false, } var writer bytes.Buffer read, err := d.ReadPartitionContents(tt.partition, &writer) @@ -356,6 +383,7 @@ func TestReadPartitionContents(t *testing.T) { PhysicalBlocksize: 512, Info: fileInfo, Table: tt.table, + Writable: false, } var writer bytes.Buffer read, err := d.ReadPartitionContents(tt.partition, &writer) @@ -401,6 +429,7 @@ func TestCreateFilesystem(t *testing.T) { LogicalBlocksize: 512, PhysicalBlocksize: 512, Info: fileInfo, + Writable: true, } expected := fmt.Errorf("cannot create filesystem on a partition without a partition table") fs, err := d.CreateFilesystem(disk.FilesystemSpec{Partition: 1, FSType: filesystem.TypeFat32}) @@ -435,6 +464,7 @@ func TestCreateFilesystem(t *testing.T) { PhysicalBlocksize: 512, Info: fileInfo, Size: fileInfo.Size(), + Writable: true, } fs, err := d.CreateFilesystem(disk.FilesystemSpec{Partition: 0, FSType: filesystem.TypeFat32}) if err != nil { @@ -477,6 +507,7 @@ func TestCreateFilesystem(t *testing.T) { Info: fileInfo, Size: fileInfo.Size(), Table: table, + Writable: true, } fs, err := d.CreateFilesystem(disk.FilesystemSpec{Partition: 1, FSType: filesystem.TypeFat32}) if err != nil { @@ -486,7 +517,16 @@ func TestCreateFilesystem(t *testing.T) { t.Errorf("Returned filesystem was unexpectedly nil") } }) - + t.Run("readonly", func(t *testing.T) { + d := &disk.Disk{ + Writable: false, + } + expectedErr := fmt.Errorf("disk file or device not open for write") + _, err := d.CreateFilesystem(disk.FilesystemSpec{}) + if err.Error() != expectedErr.Error() { + t.Errorf("Mismatched error, actual '%v', expected '%v'", err, expectedErr) + } + }) } func TestGetFilesystem(t *testing.T) { @@ -513,6 +553,7 @@ func TestGetFilesystem(t *testing.T) { LogicalBlocksize: 512, PhysicalBlocksize: 512, Info: fileInfo, + Writable: false, } expected := fmt.Errorf("cannot read filesystem on a partition without a partition table") fs, err := d.GetFilesystem(1) @@ -547,6 +588,7 @@ func TestGetFilesystem(t *testing.T) { PhysicalBlocksize: 512, Info: fileInfo, Size: fileInfo.Size(), + Writable: false, } fs, err := d.GetFilesystem(0) if err != nil { @@ -589,6 +631,7 @@ func TestGetFilesystem(t *testing.T) { Info: fileInfo, Size: fileInfo.Size(), Table: table, + Writable: false, } fs, err := d.GetFilesystem(1) if err != nil { diff --git a/diskfs.go b/diskfs.go index bb0595f7..5f3553f1 100644 --- a/diskfs.go +++ b/diskfs.go @@ -130,7 +130,33 @@ const ( Raw Format = iota ) -func initDisk(f *os.File) (*disk.Disk, error) { +// OpenModeOption represents file open modes +type OpenModeOption int + +const ( + // ReadOnly open file in read only mode + ReadOnly OpenModeOption = iota + // ReadWriteExclusive open file in read-write exclusive mode + ReadWriteExclusive +) + +var openModeOptions = map[OpenModeOption]int{ + ReadOnly: os.O_RDONLY, + ReadWriteExclusive: os.O_RDWR | os.O_EXCL, +} + +func writableMode(mode OpenModeOption) bool { + m, ok := openModeOptions[mode] + if ok { + if m&os.O_RDWR != 0 || m&os.O_WRONLY != 0 { + return true + } + } + + return false +} + +func initDisk(f *os.File, openMode OpenModeOption) (*disk.Disk, error) { var ( diskType disk.Type size int64 @@ -173,25 +199,66 @@ func initDisk(f *os.File) (*disk.Disk, error) { //var goodBlocks, orphanedBlocks int //goodBlocks = size / lblksize - return &disk.Disk{File: f, Info: devInfo, Type: diskType, Size: size, LogicalBlocksize: lblksize, PhysicalBlocksize: pblksize}, nil + writable := writableMode(openMode) + + return &disk.Disk{ + File: f, + Info: devInfo, + Type: diskType, + Size: size, + LogicalBlocksize: lblksize, + PhysicalBlocksize: pblksize, + Writable: writable, + }, nil } -// Open a Disk from a path to a device -// Should pass a path to a block device e.g. /dev/sda or a path to a file /tmp/foo.img -// The provided device must exist at the time you call Open() -func Open(device string) (*disk.Disk, error) { +func checkDevice(device string) error { if device == "" { - return nil, errors.New("must pass device name") + return errors.New("must pass device name") } if _, err := os.Stat(device); os.IsNotExist(err) { - return nil, fmt.Errorf("provided device %s does not exist", device) + return fmt.Errorf("provided device %s does not exist", device) + } + + return nil +} + +// Open a Disk from a path to a device in read-write exclusive mode +// Should pass a path to a block device e.g. /dev/sda or a path to a file /tmp/foo.img +// The provided device must exist at the time you call Open() +func Open(device string) (*disk.Disk, error) { + err := checkDevice(device) + if err != nil { + return nil, err } f, err := os.OpenFile(device, os.O_RDWR|os.O_EXCL, 0600) if err != nil { return nil, fmt.Errorf("Could not open device %s exclusively for writing", device) } // return our disk - return initDisk(f) + return initDisk(f, ReadWriteExclusive) +} + +// OpenWithMode open a Disk from a path to a device with a given open mode +// If the device is open in read-only mode, operations to change disk partitioning will +// return an error +// Should pass a path to a block device e.g. /dev/sda or a path to a file /tmp/foo.img +// The provided device must exist at the time you call OpenWithMode() +func OpenWithMode(device string, mode OpenModeOption) (*disk.Disk, error) { + err := checkDevice(device) + if err != nil { + return nil, err + } + m, ok := openModeOptions[mode] + if !ok { + return nil, errors.New("unsupported file open mode") + } + f, err := os.OpenFile(device, m, 0600) + if err != nil { + return nil, fmt.Errorf("Could not open device %s exclusively for writing", device) + } + // return our disk + return initDisk(f, mode) } // Create a Disk from a path to a device @@ -213,7 +280,7 @@ func Create(device string, size int64, format Format) (*disk.Disk, error) { return nil, fmt.Errorf("Could not expand device %s to size %d", device, size) } // return our disk - return initDisk(f) + return initDisk(f, ReadWriteExclusive) } // to get the logical and physical sector sizes diff --git a/diskfs_test.go b/diskfs_test.go index d8f1516b..49471320 100644 --- a/diskfs_test.go +++ b/diskfs_test.go @@ -66,16 +66,31 @@ func TestOpen(t *testing.T) { } for _, tt := range tests { - disk, err := diskfs.Open(tt.path) + d, err := diskfs.Open(tt.path) msg := fmt.Sprintf("Open(%s)", tt.path) switch { case (err == nil && tt.err != nil) || (err != nil && tt.err == nil) || (err != nil && tt.err != nil && !strings.HasPrefix(err.Error(), tt.err.Error())): t.Errorf("%s: mismatched errors, actual %v expected %v", msg, err, tt.err) - case (disk == nil && tt.disk != nil) || (disk != nil && tt.disk == nil): - t.Errorf("%s: mismatched disk, actual %v expected %v", msg, disk, tt.disk) - case disk != nil && (disk.LogicalBlocksize != tt.disk.LogicalBlocksize || disk.PhysicalBlocksize != tt.disk.PhysicalBlocksize || disk.Size != tt.disk.Size || disk.Type != tt.disk.Type): + case (d == nil && tt.disk != nil) || (d != nil && tt.disk == nil): + t.Errorf("%s: mismatched disk, actual %v expected %v", msg, d, tt.disk) + case d != nil && (d.LogicalBlocksize != tt.disk.LogicalBlocksize || d.PhysicalBlocksize != tt.disk.PhysicalBlocksize || d.Size != tt.disk.Size || d.Type != tt.disk.Type): t.Errorf("%s: mismatched disk, actual then expected", msg) - t.Logf("%v", disk) + t.Logf("%v", d) + t.Logf("%v", tt.disk) + } + } + + for _, tt := range tests { + d, err := diskfs.OpenWithMode(tt.path, diskfs.ReadOnly) + msg := fmt.Sprintf("Open(%s)", tt.path) + switch { + case (err == nil && tt.err != nil) || (err != nil && tt.err == nil) || (err != nil && tt.err != nil && !strings.HasPrefix(err.Error(), tt.err.Error())): + t.Errorf("%s: mismatched errors, actual %v expected %v", msg, err, tt.err) + case (d == nil && tt.disk != nil) || (d != nil && tt.disk == nil): + t.Errorf("%s: mismatched disk, actual %v expected %v", msg, d, tt.disk) + case d != nil && (d.LogicalBlocksize != tt.disk.LogicalBlocksize || d.PhysicalBlocksize != tt.disk.PhysicalBlocksize || d.Size != tt.disk.Size || d.Type != tt.disk.Type): + t.Errorf("%s: mismatched disk, actual then expected", msg) + t.Logf("%v", d) t.Logf("%v", tt.disk) } }