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

Add Filesystem::mount_or_else #57

Merged
merged 1 commit into from
May 22, 2024
Merged

Conversation

robin-nitrokey
Copy link
Member

@robin-nitrokey robin-nitrokey commented Apr 24, 2024

When mounting a filesystem, often code like this is used:

if !Filesystem::is_mountable(alloc, storage) {
    Filesystem::format(storage).ok();
}
Filesystem::mount(alloc, storage)

This mounts the filesystem twice because Filesystem::is_mountable is equivalent to Filesystem::mount(...).is_ok(). Depending on the storage implementation, mounting the filesystem can have significant cost. But directly calling Filesystem::mount and re-mounting in the error case is prohibited by the borrow checker.

This patch adds a mount_or_else method that accepts a callable that is called on mount error. Afterwards, mounting is re-tried.

Copy link
Contributor

@sosthene-nitrokey sosthene-nitrokey left a comment

Choose a reason for hiding this comment

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

nit: I don't feel like try_mount is the proper name.

Maybe mount_with_prepare ?

@robin-nitrokey
Copy link
Member Author

I’m not entirely happy with try_mount either, but I did not see a better alternative. mount_with_prepare sounds like it would always call the prepare function. Maybe mount_with_recover?

@sosthene-nitrokey
Copy link
Contributor

Is this going to be used with anything other than format?

I would have initially just gone with mount_or_format.

@robin-nitrokey
Copy link
Member Author

Even if it’s just called with format, I would like to be able to know whether the initial mount worked. This could also encoded in the return type, though it would be more complicated.

A more interesting case would be journaling. Here we first want to try to mount the filesystem, and if it fails, recover from the journal. I’m not sure if we will end up using this method for IFS recovery in the nitrokey-3-firmware, but I think it would make sense to support the use case.

@robin-nitrokey
Copy link
Member Author

@sosthene-nitrokey How about mount_or_else?

@sosthene-nitrokey
Copy link
Contributor

Looks good!

When mounting a filesystem, often code like this is used:

    if !Filesystem::is_mountable(alloc, storage) {
        Filesystem::format(storage).ok();
    }
    Filesystem::mount(alloc, storage)

This mounts the filesystem twice because Filesystem::is_mountable is
equivalent to Filesystem::mount(...).is_ok().  Depending on the storage
implementation, mounting the filesystem can have significant cost.
But directly calling Filesystem::mount and re-mounting in the error case
is prohibited by the borrow checker.

This patch adds a mount_or_else method that accepts a callable that is
called on mount error.  Afterwards, mounting is re-tried.
@robin-nitrokey robin-nitrokey changed the title Add Filesystem::try_mount Add Filesystem::mount_or_else May 22, 2024
@robin-nitrokey robin-nitrokey merged commit 2ad7785 into trussed-dev:main May 22, 2024
4 checks passed
@robin-nitrokey robin-nitrokey deleted the try-mount branch May 22, 2024 16:34
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.

3 participants