Skip to content

Conversation

aykevl
Copy link
Member

@aykevl aykevl commented Mar 30, 2020

This allows applications to mount filesystems in the os package. This is useful for mounting external flash filesystems, for example.

TODO: this is not finished: there is no support yet for paths relative to the current working directory.

Question: do we even want such a thing? Or is the Go io.Writer / io.Reader etc abstraction enough?

@aykevl
Copy link
Member Author

aykevl commented Mar 30, 2020

@jaddr2line it appears that this PR with this code surfaces a bug in #991:

package main

import (
	"fmt"
	"os"
)

func main() {
	err := os.Mkdir("/tmp/foo", 0777)
	fmt.Println("made dir:", err)
}

When /tmp/foo exists, it shows the following error:

made dir: : file exists

When I rebase on the commit on 57320c0, I get the following (correct) error instead:

made dir: mkdir /tmp/foo: file exists

@niaow
Copy link
Member

niaow commented Mar 30, 2020

I will check it out, but I would not be surprised if this is a variant of the stack object overwrite bug which we have had for a while, but has been surfaced due to the increased frequency of memory reuse.

@niaow
Copy link
Member

niaow commented Mar 30, 2020

I forced the conservative collector to re-collect on every allocation and re-use memory as soon as possible, and the issue was replicated. It existed previously, but is rare with the conservative collector.

@bgould
Copy link
Member

bgould commented Apr 8, 2020

So far it looks like this will be mostly compatible with 2 embedded filesystem implementations that I have been working on:

https://github.com/bgould/go-littlefs
https://github.com/bgould/go-fatfs

At first glance it might be beneficial also to add Stat() and Readdir() to the Filesystem interface... both of those return os.FileInfo which conveniently is also an interface, and these methods would provide significant functionality.

I'm going to try out this branch and see if I can make it work with the 2 filesystems I mentioned above and then I may have some more comments as well.

Overall I think this looks good so far and I'm excited for this to eventually get included in a release, I do think have filesystem support will be very beneficial.

@aykevl
Copy link
Member Author

aykevl commented Apr 8, 2020

My idea was to keep the implementation as minimal as possible but allow more methods to be implemented. Those methods can be called using type asserts.

Stat may indeed be a good idea to support everywhere, if you can open a file then you should also be able to return some information about it (or that it doesn't exist).

Readdir I'm not so sure about. It would be nice if the interface allowed very minimal filesystems without subdirectory support, more like key/value storages. On the other hand, reading the root directory should not be difficult to support either.
I think the main reason I left it out was that I wasn't sure what the API should look like.

@bgould
Copy link
Member

bgould commented Apr 9, 2020

So first of all I was not thinking clearly, of course Readdir would need to be on the FileHandle, not on the Filesystem, and actually in the Go standard library Stat() exists at both levels.

As far as filesystems that do not support particular features, I think a simple solution could be for the implementation to just always return an error for Readdir() if it is not supported for instance. This is basically what the standard Go library does if you try to Readdir() on a regular file anyhow for instance:

$ cat main.go
package main

import (
        "log"
        "os"
)

func main() {
        f, err := os.Open("main.go")
        if err != nil {
                log.Fatal(err)
        }
        _, err = f.Readdir(0)
        if err != nil {
                log.Fatal(err)
        }
}
 $ go run main.go 
2020/04/09 08:29:40 readdirent: not a directory
exit status 1

That said, I've tried this PR in its current state with the embedded filesystems mentioned above and the abstraction is working well using very minimal glue code. It is definitely usable/useful in its current state even with Stat() and Readdir() - could be used for configuration files with known names on embedded platforms for instance. To your point, it might be a good idea to start with this, as it would be simple enough to use type asserts to add additional functionality later.

👍

@deadprogram
Copy link
Member

Now that the SPI Flash driver has been merged, it would be great to get this PR into shape and merged as well, probably.

@aykevl aykevl force-pushed the os-filesystem branch 2 times, most recently from 2f6d778 to 22b65b5 Compare May 9, 2020 13:28
This allows applications to mount filesystems in the os package. This is
useful for mounting external flash filesystems, for example.
@aykevl aykevl marked this pull request as ready for review May 9, 2020 13:38
@aykevl aykevl changed the title [WIP] os: implement virtual filesystem support os: implement virtual filesystem support May 9, 2020
@aykevl
Copy link
Member Author

aykevl commented May 9, 2020

I have rebased this PR and added some comments indicating that the interface is not yet finalized. I've also implemented some (limited) support for relative directories on regular operating systems, you will still need to always use absolute paths on a non-OS system. I think this is now ready.

Regarding the missing Stat and Readdir, @bgould what do you think of leaving those out for initial support and adding them in a separate PR? (That's why I marked the interfaces unstable).

@bgould
Copy link
Member

bgould commented May 13, 2020

@aykevl I think leaving those methods out for now is perfectly fine. I've been running with the changes in this PR for a while now for a project that I'm working on, and even without support for reading the contents of directories it is definitely useful. I have support for this PR already in my tinyfs repo here https://github.com/bgould/tinyfs/blob/master/tinygo_os.go and I am confident that the concept works well and is pretty easy to implement. I'm certainly in favor of moving forward with it.

@deadprogram
Copy link
Member

This is great progress, thank you @aykevl and @bgould for moving it forward!

Now merging. 📂

@deadprogram deadprogram merged commit 6bcb40f into dev May 13, 2020
@deadprogram deadprogram deleted the os-filesystem branch May 13, 2020 06:14
@niaow niaow added this to the v0.14 milestone Jun 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants