Skip to content

Add registration struct #1

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

Open
wants to merge 1 commit into
base: basic-fs-abstractions
Choose a base branch
from

Conversation

foxhlchen
Copy link
Collaborator

It's an initial attempt to add Registration to rust fs.

It works as we discussed before: 1) Registration::new_pinned() creates a registration variable of a specific fs type, 2) Registration::register() registers that fs type, 3) and when the variable is out-of-scop, it will automatically unregister that fs type.

I also see you have a DeclaredFileSystemType type that embeds bindings::file_system_type into FileSystemBase. I'm not sure how to reconcile this.

This code has not been fully tested, because it looks like this branch is not complete, I will find a way to port this to main and test the code.

This commit adds registration struct to rust file system.

struct Registration will be used to register a filesystem type
as well as automatically unregister it when the Registration variable
is out-of-scop.

Signed-off-by: Fox Chen <[email protected]>
Copy link
Owner

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

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

Did you start on a recent rust commit? Our branches are out of date again, we should probably rebase before merging this PR :D

}

pub fn new_pinned() -> Result<Pin<Box<Self>>> {
let mut c_fs_type = FileSystemType::default(); // may use DEFAULT_FS_TYPE?
Copy link
Owner

Choose a reason for hiding this comment

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

The Default impl just returns mem::zeroed(). I am unsure whether we would want to make that more explicit - then again, this is what C code would do. I don't know if there are any safety concerns here, but that is something that we can leave for the upstream people to decide. Until then, default seems fine.

Ok(Pin::from(Box::try_new(Self::new(c_fs_type))?))
}

pub fn register(&mut self) -> Result {
Copy link
Owner

Choose a reason for hiding this comment

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

We really want to take self: Pin<&mut Self> here

Ok(())
}

fn unregister(&mut self) -> Result {
Copy link
Owner

Choose a reason for hiding this comment

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

Same here I think - this means that the Drop impl has to be implemented similar to https://doc.rust-lang.org/std/pin/index.html#drop-implementation - but that's a good thing in my opinion :)

fs_type: FileSystemType,
}

//Pin self
Copy link
Owner

Choose a reason for hiding this comment

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

Does this mean anything?

Suggested change
//Pin self

@foxhlchen
Copy link
Collaborator Author

Did you start on a recent rust commit? Our branches are out of date again, we should probably rebase before merging this PR :D

No, I was starting on this branch.

Can we rebase upstream now?? I found It hard switching between different rust versions :D.

@niklasmohrin
Copy link
Owner

Did you start on a recent rust commit? Our branches are out of date again, we should probably rebase before merging this PR :D

No, I was starting on this branch.

Can we rebase upstream now?? I found It hard switching between different rust versions :D.

I am afraid I don't have any time the next couple of days, but we can aim to rebase next week

@foxhlchen
Copy link
Collaborator Author

Did you start on a recent rust commit? Our branches are out of date again, we should probably rebase before merging this PR :D

No, I was starting on this branch.
Can we rebase upstream now?? I found It hard switching between different rust versions :D.

I am afraid I don't have any time the next couple of days, but we can aim to rebase next week

No worries.

I will submit the fix after the rebase.

thank you!

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