Allows to open disk from already existing fs.File descriptor

[aol-nnov]

I.e. in some fs.FS compliant VFS, given that vfs.Open("some.img") returns fs.File

@deitch if you have no objections, would you mind merging this improvement as well?

[deitch]

It looks like this is identical to diskfs.Open(), except that it passes an fs.File rather than a string path to the device or file to open?

I didn't understand the need for it.

in some fs.FS compliant VFS, given that vfs.Open("some.img") returns fs.File

If that is the case, why OpenFile(f fs.File) and not OpenFile(f *os.File)? In the end, Open() calls os.OpenFile() to get an *os.File anyways.

[aol-nnov]

In the end, Open() calls os.OpenFile() to get an *os.File anyways.

But, actually, any implementation will fit as long as it supports fs.File + util.File, right? To be frank, I had a thought to replace this field with interface, too, but spent too little time elaborating on it. So, to start with I decided to add just diskfs.OpenFile()

why OpenFile(f fs.File) and not OpenFile(f *os.File)?

the only intent to use fs.File as argument is to hide type assertion inside OpenFile() implementation. If you insist, I can live with the assertion on my side...

[deitch]

But, actually, any implementation will fit as long as it supports fs.File + util.File, right?

Not quite. I almost responded that anything that supports ReaderAt, WriterAt and Closer, but that is not quite correct.

If you look at initDisk, it actually does an f.Stat() in order to get the size, determine if it is a file or block device, etc.

However, Stat() also is a property of fs.File, so that might work. On the other hand, fs.File explicitly is read-only. I once did a whole implementation of "writable FS" for an OSS project.

If you want to see if it is possible to replace Open(f *os.File) with Open(f SomeFIleType) where:

type SomeFIleType interface {
    fs.File
    ReaderAt
    WriterAt
    Closer
}

(but much better naming), then it is worth a try. But I wouldn't break Open(), instead have some other function that calls it, and unify them.

[aol-nnov]

But I wouldn't break Open()

Definitely! That's why I came up with a new function for that.

to replace Open(f *os.File) with Open(f SomeFIleType)

I'd rather keep SomeFIleType internal and have public api as I proposed in the PR: OpenFile(f fs.File, ...) and assert type internally (and return an error if incoming type does not fit). That builds more smooth API as far as I can tell.

To cut the long story short, my typical usage is:

myVfs := magicFs.New(...)
img, err:= myVfs.Open("disk.img")
disk, err:= diskfs.OpenFile(img)

That's why I want diskfs.OpenFile() accept fs.File as input.

I will investigate more and get back to you with my findings.

[deitch]

The problem is that fs.File is read only, so it doesn't work. If you really need to use just fs.File, probably best off casting it outside of the Open call.

I like the idea of an interface rather than an fs.File, but it has to have the necessary functions.

[aol-nnov]

fs.File is read only, so it doesn't work

But it works on my machine in my scenario )) (and, yes, to be frank, my scenario is for reading only - I didn't check any writing functionality)

Okay, I've got your points. I need some time to think out the ways...

[aol-nnov]

@deitch I had some mocking attempts during the day, but it appeared a rabbit hole to replace os.File with interface, I must admit.

At some point I thought it could be worth having a read-only backing file interface for read operations and rw one for write operations.

Again, why do I stick to fs.File as the base: it is the common interface for all cases which we could type-assert the underlying storage for our needs.

So, I've created a storage class that has methods to type-assert to a required functionality and used it all over the place. Changes seem massive and not yet complete.

I just wanted to ask, if it's the right investment.. (please see my brain dump that I've offloaded today into this PR)
If not, I'd rather stick to my initial initiative with your corrections - add a single method diskfs.OpenFile() with the following:

func OpenFile(f *os.File, opts ...OpenOpt) (*disk.Disk, error) {

	opt := &openOpts{
		mode:       ReadOnly,
		sectorSize: SectorSizeDefault,
	}

	for _, o := range opts {
		if err := o(opt); err != nil {
			return nil, err
		}
	}

	return initDisk(f, opt.mode, opt.sectorSize)
}

and perform all needed assertions outside. I'd be more than happy with that.

[deitch]

I am trying to wrap my head around what you did. I think I see something interesting, but I am not sure. Let's see if I get it.

• diskfs.Open() remains unchanged
• diskfs.OpenFile() is new. It takes as first parameter fs.File, and passes it to initDisk()
• initDisk() takes an fs.File as first parameter, instead of *os.File.
• Rather than storing Disk.Writable as a separate property, it has a new structure called storage.Disk that wraps the underlying disk device, as well as storing the writable state
• Whenever we otherwise would have tried to write to the device, rather than checking d.Writeable, we call d.File.Rw(). If it returns RwFile, we know it is read-write, and then we can use it, otherwise return an error. For things where we do not need to read, we can call d.File.Ro() and get RoFile.
• Lots of underlying functions get passed this RoFile or RwFile rather than whatever they had before, I guess for stricter type-checking.

The goals appear to be:

• Have the ability to pass an fs.File, which is more universal than os.File
• Have the ability to pass an fs.File, a read-only file structure, when we only will do read operations

Is that about it? If so, I think I like the goals, and most of the direction. I would suggest some simplifications, but let's wait until I see if I understood you correctly.

[aol-nnov]

Thanks for the prompt response and, yes, you understood the idea quite right!

One clarification (looks like a plain typo of yours)

For things where we do not need to read, we can call d.File.Ro() and get RoFile.

For things where we do not need to write

[deitch]

OK, got it. I have some thoughts on simplifications. They might or might not be worth it, once I type them out. I will get to them a bit later.

[aol-nnov]

Whenever you're comfortable to! No rush. I'd be glad to hear your suggestions! To be frank, that massive plot twist during my coding session was unexpected, but yet, I decided to share my thoughts ))

I'm leaving for today (I'm GMT+3) and will be back online in next 12 hours or so. I'll have occasional access from smartphone for next 4 hours, though.

[deitch]

In that case, I will get to it when I do. Might be in the morning.

[deitch]

I will comment here, although some of it may be inline as well.

devcontainer

I don't mind have devcontainer.json for it, but it should be in a different PR. I also would likely use the standard library/golang.

storage.File

Originally, I didn't like the idea of it being in a new package, but I have come around to it.

The language of Ro vs Rw is somewhat misleading. All underlying storage at least must support read; some functions require it to be write as well. So maybe storage.File should be readable as is, while you can get a "writable variant" if you ask for one. Something like (I know this isn't 100% correct, just the idea):

type File struct {
	fs.File
	io.ReaderAt
	io.Seeker
	io.Closer
}


type Writable interface {
	File
	io.WriterAt
}

// OS-stecific file for ioctl calls via fd
func (f File) Os() (*os.File, error) {
	if osFile, ok := f.storage.(*os.File); ok {
		return osFile, nil
	}
	return nil, errNotSuitable
}

// file for read-only operations is not needed, by definition I can use `storage.File` for anything readable

// file for writable operations
func (f File) Writable() (Writable, error) {
	if rwFile, ok := f.storage.(Writable); ok {
		if !f.readonly {
			return rwFile, nil
		} else {
			return nil, errIncorrectOpenMode
		}
	}
	return nil, errNotSuitable
}

So my usage becomes:

func (d *Disk) ReadPartitionContents(part int, writer io.Writer) (int64, error) {
        /* this all goes away, since d.File _is_ readable
	backingRoFile, err := d.File.Ro()
	if err != nil {
		return -1, err
	}
        */
...
	return partitions[part-1].ReadContents(d.File, writer)

usage of struct vs interface

I think this only appears in one place, but changing this:

func (fs *FileSystem) readMetadata(r io.ReaderAt, c Compressor, firstBlock int64, initialBlockOffset uint32, byteOffset uint16, size int) ([]byte, error) {

to that:

func (fs *FileSystem) readMetadata(r storage.RoFile, c Compressor, firstBlock int64, initialBlockOffset uint32, byteOffset uint16, size int) ([]byte, error) {

doesn't really buy anything. The only part of storage.RoFile (or just storage.File) that readMetadata() needs is its io.ReaderAt, so might as well leave it as the most basic interface needed.

initDisk

I like what you did there. Why pass it an *os.File when a more abstract fs.File will do. Since os.File implements fs.File, it can be called in the same way without changing the calling function. Much cleaner. Use the most basic interface needed (similar to the point in the previous section).

Thoughts?

[aol-nnov]

Thank you for the detailed comments, Avi!

Devcontainer configs (and I've added some vscode configs later, too) definitely deserve a separate PR. Will split that into separate one in a moment. As for container image, it was offered by vscode wizard, so I suppose it contains some vscode-related tweaks for more comfortable development. I didn't dig any further in that direction yet.

As for newly introduced storage package, it was needed to avoid import cycles. At start I was trying to avoid it as much as I could, but failed.

I've got your idea about storage.File improvements and will proceed in that direction, thank you.

Regarding usage of struct vs interface in readMetadata and such, totally agreed. I will revert that part. It was a collateral damage of my fight with red underlines (type mismatches) all over the place during my intial attempt. Did not think much during that fighting, ha )

[aol-nnov]

Hey, @deitch ! Had some time to implement your suggestions today. Please, check if I'm moving in the right direction and got everything right.

Tests are still failing, but I'm on it. Just syncing and showing you my progress.

Few words on why I implemented storage.Storage and storage.File:

If I put Sys() (*os.File, error) and Writable() (WritableFile, error) into storage.File interface, it'll break xxx.Read in cases like this as the type of first argument f does not implement Sys() and Writable(), so, I've made it that way. What do you think?

[deitch]

Yeah, that is pretty nice. Cleaner and easier to understand. It took more work, but I like it.

[aol-nnov]

Glad you like it!

I'll fix the tests then and we'll see it in action.

Btw, while separating ro and rw operations I've stumbled upon a fat32 test, where traversing directories actually was creating them. Now it's failing and I temporary commented it out. Needs more thinking! ))

[deitch]

Btw, while separating ro and rw operations I've stumbled upon a fat32 test, where traversing directories actually was creating them. Now it's failing and I temporary commented it out. Needs more thinking!

We have had cases like that before. Do you mind opening a separate issue just around the fat32 test and how to recreate it?

[aol-nnov]

Do you mind opening a separate issue just around the fat32 test

(putting in my to-do list) Will do, eventually! ))) I haven't dove into concrete implementations yet.

[aol-nnov]

Oh, wow...

To my surprise, the similar plot is rolling out on the qcow2 branch! Just noticed it and now the puzzle fits quite well, yay!

May be we should tweak the naming here, too? It's no longer a d.File, it's something more generic!

It's now something like a "backend storage". Backend? Or, may be a Driver as you named it those days?

[deitch]

the similar plot is rolling out on the qcow2 branch! Just noticed it and now the puzzle fits quite well, yay

Do you mean the test directory duplication issue?

It's now something like a "backend storage". Backend? Or, may be a Driver as you named it those days?

No objection. I don't really think it is a driver, as it doesn't include lots of logic. It is a storage or backing store or backend.

[aol-nnov]

Do you mean the test directory duplication issue?

No, I mean the whole interface-instead-of-os.File thing! ))

[aol-nnov]

Once in a while I'm thinking to myself, isn't it a time for v2, ha! ))

P.S.: I don't get it.. Why does it fail only on ubuntu?! Darwin has the same tests, no?!

[aol-nnov]

Is there a parallel for Create?

Not more than the snippet below, as I stated earlier.

backend, _ := raw.CreateFromPath("...")
disk.OpenStorage(backend)

If you have better ideas, please let me know!

[deitch]

Not more than disk.OpenStorage(raw.CreateFromPath("...")), as I stated #265 (comment).

Are you saying that diskfs.Create() is not really needed? Here is the source for Open() and Create():

func Open(device string, opts ...OpenOpt) (*disk.Disk, error) {
	err := checkDevice(device)
	if err != nil {
		return nil, err
	}

	opt := openOptsDefaults()
	for _, o := range opts {
		if err := o(opt); err != nil {
			return nil, err
		}
	}

	m, ok := openModeOptions[opt.mode]
	if !ok {
		return nil, errors.New("unsupported file open mode")
	}

	f, err := os.OpenFile(device, m, 0o600)
	if err != nil {
		return nil, fmt.Errorf("could not open device %s with mode %v: %w", device, m, err)
	}
	// return our disk
	return initDisk(f, ReadWriteExclusive, opt.sectorSize)
}

// Create 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 not exist at the time you call Create()
func Create(device string, size int64, _ Format, sectorSize SectorSize) (*disk.Disk, error) {
	if device == "" {
		return nil, errors.New("must pass device name")
	}
	if size <= 0 {
		return nil, errors.New("must pass valid device size to create")
	}
	f, err := os.OpenFile(device, os.O_RDWR|os.O_EXCL|os.O_CREATE, 0o666)
	if err != nil {
		return nil, fmt.Errorf("could not create device %s: %w", device, err)
	}
	err = os.Truncate(device, size)
	if err != nil {
		return nil, fmt.Errorf("could not expand device %s to size %d: %w", device, size, err)
	}
	// return our disk
	return initDisk(f, ReadWriteExclusive, sectorSize)
}

Are you saying that we don't really need Create(), it could be moved to raw.CreateFromPath(), because all it does is:

1. ensure that a path was provided - storage takes care of that
2. Open the file with RDWR|EXCL|CREATE, so it creates the file if it does not exist
3. Ensures the size is provided and then grow/shrink the file to that size
4. call initDisk

So you are saying, that should be outside of diskfs basic functions, part of storage - create a file, set its size - and then you just would call Open() like any other that exists?

[aol-nnov]

You caught my thought perfectly correct!

That's what I was trying to say while introducing Resize() and Create() here.

But definitely, it all needs more thinking... If you're fine with merging this PR as it is now and proceed in the next PRs, I'm totally fine, too!

Also, this change will introduce the API breakage, so we need to decide how to handle such a big change.

[deitch]

Also, this change will introduce the API breakage, so we need to decide how to handle such a big change

This one? This doesn't break anything. 😕
Oh, maybe Create()? Why break that?

You caught my thought perfectly correct!

Then we should document it. Maybe add an example to show how to use the storage to Create or Open, and the main doc a bit?

[aol-nnov]

This one?

No, the one you proposed!

and then you just would call Open() like any other that exists?

In such case we could introduce a single diskfs.Open(b storage.Backend) but golang does not support method overloading as C does. Thus the API breakage - diskfs.Open would have a totally different set of parameters..

[aol-nnov]

Maybe add an example to show how to use the storage to Create or Open, and the main doc a bit?

Yeah, will do later today or tomorrow morning! Also, I've got an idea to add raw.OpenFromPath("..") to parallel diskfs.Open.

So, making it as draft again! ))

[aol-nnov]

Added some example usage and raw.CreateFromPath()/raw.OpenFromPath() utility methods.

[aol-nnov]

@deitch probably, I had to make it more clear, that I was done with it - too much 'draft'-toggling back and forth... So, this is it, finally, ha!

Please review when you have time and let me know if there are any more improvements needed.

[deitch]

Sorry, took me some time to get to it. Trying now.

[deitch]

Mostly some questions, a few recommended naming changes. Overall, this looks solid. Thank you.

[deitch]

Reading this twice made me think, is "raw" the right name? Or should the backend just be file or path? That would simplify the interface to file.Open() and file.Create() (or path.Open() and path.Create()).

[aol-nnov]

Reading this twice made me think, is "raw" the right name?

I was thinking of naming backends according to supported image formats. So, raw backend supports raw images. The next one will be, probably, qcow2.

Do you think, it is misleading?

[deitch]

But qcow2 is a filesystem, which is lower-level than a disk. You get to qcow2 by doing diskfs.Open() (or Create()), which returns a handle to a Disk. What you do with that - reading a partition table, or a filesystem, or creating one, or partition with a filesystem on one of the partitions, is all at a next level down. Disk doesn't care or know about what the internal structure is.

[aol-nnov]

But qcow2 is a filesystem, which is lower-level than a disk

Correct, it is on the same level as raw backend is!

You get to qcow2 by doing diskfs.Open()

Something like diskfs.OpenBackend(qcow2.New("./the_image")), right? Disk does not care or know about what the internal structure is, indeed!

[deitch]

Yeah, but I don't think you need a separate backend for qcow2. Backend does not know or think about the contents.

---> Disk (wraps) ---> Backend (handles) ---> low-level i/o

Dealing with qcow2 is handled at a higher level, not at a lower level. If you look at the qcow2 branch, it just changes how Disk deals with its writes. Adding your (good) Backend abstraction shouldn't change that. It just abstracts out i/o; what gets written to it is handled in Disk (or a level above).

To use your example, I think it would be more d := diskfs.OpenBackend(file.New("./the_image")). Inside of d we would have ways of handling the raw vs qcow2. I don't think it goes in Backend. I may discover otherwise as I pick up qcow2 again, but I don't think so.

[aol-nnov]

Also, if you'd like to bring some magic (as in man 5 magic) to users by automatically selecting the backend based on provided file image format, we could implement such a selector, too.

[deitch]

bring some magic

Do you mean something like this magic? Haven't touched it in some time, but still use it in a few places. 😁

bit against the beloved "unix way"

I disagree, I think it is precisely the Unix way. Backend handles one thing, and one thing only: interfacing with a storage device. It is a "storage driver", not a "format driver". Following the unix way, as you suggested, I could see us splitting a "file" backend from a "block device" backend (or device driver, etc.), i.e. difference between /home/aol-nnv/disk.img and /dev/sd0. Right now, they are really close, we just have a bit of logic to distinguish between them; that would be easier if we had different backends. Another might be memory (for really small ones).

If we did vmdk (I looked at it at some point, really remember nothing about it), like qcow2, these would be a level above, right now in Disk. Also, I like that idea, although vmdk is becoming less and less interesting, as VMWare itself appears to be going through some challenges post-acquisition. On the other hand, demand for utilities to handle those may grow. Who knows?

had some thoughts about factoring ext4, fat and iso out of this repo and to establish an API for registering external implementations to allow users to use their own

Feel free to open an iissue to discuss it; I want to see where your thoughts are going.

Still not convinced?

Nope, not really. I can see how in the future we might want some layer to distinguish between disk types, beyond having them part of Disk itself. I don't think those will be Backend - which will be purely i/o drivers, sort of "layer 2" (OSI model) - but either part of Disk or something else yet to be discovered.

Of course, we only will know for sure once we get there.

[deitch]

bring some magic

Do you mean something like this magic? Haven't touched it in some time, but still use it in a few places. 😁

bit against the beloved "unix way"

I disagree, I think it is precisely the Unix way. Backend handles one thing, and one thing only: interfacing with a storage device. It is a "storage driver", not a "format driver". Following the unix way, as you suggested, I could see us splitting a "file" backend from a "block device" backend (or device driver, etc.), i.e. difference between /home/aol-nnv/disk.img and /dev/sd0. Right now, they are really close, we just have a bit of logic to distinguish between them; that would be easier if we had different backends. Another might be memory (for really small ones).

If we did vmdk (I looked at it at some point, really remember nothing about it), like qcow2, these would be a level above, right now in Disk. Also, I like that idea, although vmdk is becoming less and less interesting, as VMWare itself appears to be going through some challenges post-acquisition. On the other hand, demand for utilities to handle those may grow. Who knows?

had some thoughts about factoring ext4, fat and iso out of this repo and to establish an API for registering external implementations to allow users to use their own

Feel free to open an iissue to discuss it; I want to see where your thoughts are going.

Still not convinced?

Nope, not really. I can see how in the future we might want some layer to distinguish between disk types, beyond having them part of Disk itself. I don't think those will be Backend - which will be purely i/o drivers, sort of "layer 2" (OSI model) - but either part of Disk or something else yet to be discovered.

Of course, we only will know for sure once we get there.

[deitch]

What is "BDFL"?

[aol-nnov]

Do you mean something like this magic?

Yep, exactly)

What is "BDFL"?

https://en.m.wikipedia.org/wiki/Benevolent_dictator_for_life

I thought, it's an established term among programmers 😁

Okay, I'll try to find a few minutes tomorrow to finalize this. Turned into a bit of a journey already)) (and I'm not saying I didn't like it!)

[aol-nnov]

Sorry, took me some time to get to it. Trying now.

No worries and, definitely, no rush!

I'll address your comments tomorrow. I'm a bit overwhelmed with U-Boot back-porting right now, ha ))

[aol-nnov]

@deitch I've renamed raw backend to file as per you request. We're close to merge complete than ever before ))

[deitch]

That was some job, wasn't it? Thank you for all the effort!

[aol-nnov]

It's a pleasure to deal with you, Avi! Looking forward to contribute more! :)