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

Added the ability to close a directory node #271

Merged
merged 1 commit into from Dec 7, 2020

Conversation

DrDeano
Copy link
Member

@DrDeano DrDeano commented Nov 30, 2020

Closes #270

src/kernel/filesystem/vfs.zig Outdated Show resolved Hide resolved
@@ -191,7 +191,7 @@ pub const FileNode = struct {

/// See the documentation for FileSystem.Close
pub fn close(self: *const FileNode) void {
return self.fs.close(self.fs, self);
return self.fs.close(self.fs, @ptrCast(*const Node, self));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of casting it would make more sense if this function becomes part of the Node struct.

Copy link
Member Author

Choose a reason for hiding this comment

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

This wouldn't work as you can't close a symlink. Plus openFile and openDir return FileNode and DirNode so there will need to be a cast at some point.

Copy link
Collaborator

@SamTebbs33 SamTebbs33 Dec 1, 2020

Choose a reason for hiding this comment

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

I reckon we could close a symlink, after all it's going to be kept around in memory by the FS and we'll want to get rid of it from memory once it isn't used. openFile and openDir do indeed return specialised nodes, but when converting a Node to a DirNode or FileNode we switch on them rather than cast since it's much safer and allows us to detect invalid nodes being passed.

Copy link
Member Author

Choose a reason for hiding this comment

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

How would you do back from a FileNode or DirNode to a Node?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well you wouldn't have to if the function is part of Node and takes a *Node or *const Node as self.

Copy link
Member Author

@DrDeano DrDeano Dec 2, 2020

Choose a reason for hiding this comment

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

I had a look, but changes a lot of types from FileNode and DirNode to just Node and mean that I will need to change the mount type to Node instead of DirNode. I'm not sure how to do this. Could you show me da way sensei.
Was thinking of having 2 close functions, one for FileNode and one for DirNode.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I see that the problem is that with this a close function that takes a Node we can't do

var file = vfs.openFile(...)
file.close()

A solution is to have a close function in in FIleNode and DirNode like you do now, then have a close function for each of those in FileSystem (i.e. closeFile, closeDir). This avoids the casting that could be dodgy. I'm not sure how to tackle closing symlinks, that could be tended to later on.

Copy link
Collaborator

@SamTebbs33 SamTebbs33 left a comment

Choose a reason for hiding this comment

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

Noice

@DrDeano DrDeano force-pushed the feature/vfs-close-dir branch 2 times, most recently from 9454a04 to 8eba38b Compare December 7, 2020 17:19
Updated docs
Closes #270


Update doc comment


Moved close to closeFile and closeDir in VFS


Moved back to @ptrCast()


Always forget to format


Added TODO comment
@DrDeano DrDeano merged commit 285b8d9 into develop Dec 7, 2020
@DrDeano DrDeano deleted the feature/vfs-close-dir branch December 7, 2020 22:49
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.

VFS close directories
2 participants