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

Extend filesystem interface #264

Merged
merged 1 commit into from
Nov 10, 2024

Conversation

aol-nnov
Copy link
Contributor

@aol-nnov aol-nnov commented Nov 9, 2024

Interface is extended as a part of ext4 improvements effort
#9 (comment)

Also, Remove method is introduced for all supported filesystems.

@deitch should we keep ext4.Rm() or we can drop it alltogether infavour of newly introduced filesystem.Remove()?

@aol-nnov aol-nnov force-pushed the extend-filesystem-interface branch 3 times, most recently from 3b148db to 305d4c1 Compare November 9, 2024 10:59
@aol-nnov
Copy link
Contributor Author

aol-nnov commented Nov 9, 2024

Oh, just stumbled upon #233!

What do you think about adding Rename() to the interface as well?

@deitch
Copy link
Collaborator

deitch commented Nov 10, 2024

I rather like this. Thank you for doing it.

Oh, just stumbled upon #233!

Yup, although that didn't complete the interface, and you did here. So let's complete this one, and then that can just be the specific implementation.

What do you think about adding Rename() to the interface as well?

Sure. There is a parallel in os.Rename() , so it works well.

should we keep ext4.Rm() or we can drop it alltogether infavour of newly introduced filesystem.Remove()?

For now, let's deprecate it. Mark it as deprecated in the comments and refer to Remove(), and have it just call ext4.Remove(). It also parallels os.Remove(). In the future, we can remove it entirely.

The only other comment I had was about the returned error. I will comment inline. Once that is in, I will kick off CI and this can run and get merged in.

Thanks @aol-nnov !

filesystem/squashfs/squashfs.go Outdated Show resolved Hide resolved
@aol-nnov
Copy link
Contributor Author

Also, what do you think about adding chown and chmod as well? It seems, the interface will be complete then.

@deitch
Copy link
Collaborator

deitch commented Nov 10, 2024

Good call.

@aol-nnov
Copy link
Contributor Author

Ok, will mock something as soon as I get to my laptop.

@aol-nnov aol-nnov force-pushed the extend-filesystem-interface branch from 305d4c1 to 3389941 Compare November 10, 2024 12:59
@aol-nnov
Copy link
Contributor Author

Added Rename, Chown, Chmod to the Filesystem interface, used newly introduced ErrReadonlyFilesystem error where applicable.

@aol-nnov aol-nnov force-pushed the extend-filesystem-interface branch from 3389941 to 904aebc Compare November 10, 2024 13:05
deitch
deitch previously approved these changes Nov 10, 2024
@deitch
Copy link
Collaborator

deitch commented Nov 10, 2024

This is great. Let's let CI go, and if green, we can merge it in.

@aol-nnov
Copy link
Contributor Author

Bummer!... Linter does not like parameters in unimplemented functions, but I'd rather keep them for documentation reasons...

Is there any annotation for such cases?

@deitch
Copy link
Collaborator

deitch commented Nov 10, 2024

Some of the lint errors are duplication, e.g.:

func (fs *FileSystem) Link(oldpath string, newpath string) error {

Should be:

func (fs *FileSystem) Link(oldpath, newpath string) error {

Some of the others are shadow imports, like:

func (fs *FileSystem) Mknod(path string, mode uint32, dev int) error {

Which shadows the path package, so change the name of the parameter to something non-duplicate like name or p, etc.

For the last, unused parameter, just replace it with _, like:

func (fs *FileSystem) Rename(_, newpath string) error {

Or, if you want to keep it, you can put in a nolint for it. E,g,:

// Rename rename a file from oldpath to newpath
//
//nolint:unused-parameter // keep it around to make it easier in the future
func (fs *FileSystem) Rename(oldpath, newpath string) error {

@aol-nnov
Copy link
Contributor Author

Thank you for the hints! Will fix this up shortly!

@aol-nnov
Copy link
Contributor Author

For all the ErrNotSupported cases I've replaced parameter names with _ placeholder.

As for ErrNotImplemented cases, unfortunately, I had no luck with //nolint:unused-parameter, but I've worked it around the other way:

func (fs *FileSystem) Remove(_ /*pathname*/ string) error {
	return filesystem.ErrNotImplemented
}

The coder to implement this will then remove the placeholder and uncomment the actual parameter name.

@aol-nnov aol-nnov force-pushed the extend-filesystem-interface branch from 904aebc to 896b1a5 Compare November 10, 2024 16:12
@aol-nnov aol-nnov requested a review from deitch November 10, 2024 16:14
@aol-nnov aol-nnov force-pushed the extend-filesystem-interface branch from 896b1a5 to c20b1cf Compare November 10, 2024 16:20
@deitch
Copy link
Collaborator

deitch commented Nov 10, 2024

I had no luck with //nolint:unused-parameter

My mistake, it should be //nolint:revive. See this func for example.

CI looks clean. Let's replace that, and then we can merge it in.

Interface is extended as a part of ext4 improvements effort
diskfs#9 (comment)

New methods introduced:

* Mknod
* Link
* Symlink
* Chmod
* Chown
* Rename
* Remove

filesystem.ErrNotSupported is returned if FS does not support a method.

Methods lacking implementation return filesystem.ErrNotImplemented.
@aol-nnov aol-nnov force-pushed the extend-filesystem-interface branch from c20b1cf to ffe4083 Compare November 10, 2024 18:14
@aol-nnov
Copy link
Contributor Author

Okay, done.

@deitch deitch merged commit 6894b5a into diskfs:master Nov 10, 2024
20 checks passed
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