Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: Adds tests to prove we allow fs.FS to create new files #1015

Merged
merged 3 commits into from
Jan 9, 2023

Conversation

codefromthecrypt
Copy link
Contributor

@codefromthecrypt codefromthecrypt commented Jan 7, 2023

This ensures those who cannot use writefs.DirFS can use a fs.FS implementation that creates or opens files as read/write. This does so by making a hacked fs.FS and testing it.

type hackFS string

func (dir hackFS) Open(name string) (fs.File, error) {
	path := ensureTrailingPathSeparator(string(dir)) + name

	if f, err := os.OpenFile(path, os.O_RDWR, 0); err == nil {
		return f, nil
	} else if errors.Is(err, syscall.EISDIR) {
		return os.OpenFile(path, os.O_RDONLY, 0)
	} else {
		return nil, err
	}
}

This also removes type filtering to ensure hacked filesystems aren't accidentally masked. Instead we get more specific on error codes returned by functions such as os.File.Write.


Note: This is meant to extend the life of fs.FS for custom implementations, though we expect eventually to have our own API. However, such an API will remain internal until at least the end of the month due to a lot of prerequisite work. See #1013 cc @wjam

if err != nil {
return nil, err
}

if flag == 0 || flag == os.O_RDONLY {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of masking here, we are verifying the errno on Write. seems to work e.g. flags are honored to prohibit writes.

// maskForReads masks the file with read-only interfaces used by wazero.
//
// This technique was adapted from similar code in zipkin-go.
func maskForReads(f fs.File) fs.File {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was moved as it is only used here now

Signed-off-by: Adrian Cole <adrian@tetrate.io>
Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt codefromthecrypt changed the title fs: Adds tests to prove we allow non-compliant FS implementations fs: Adds tests to prove we allow fs.FS to create new files Jan 7, 2023
ref
Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt codefromthecrypt merged commit 8f0967e into main Jan 9, 2023
@codefromthecrypt codefromthecrypt deleted the hackfs branch January 9, 2023 04:31
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.

2 participants