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 special handling for "/default" filesystem #7924

Merged
merged 3 commits into from Sep 6, 2018

Conversation

Projects
None yet
10 participants
@kjbracey-arm
Contributor

kjbracey-arm commented Aug 29, 2018

Description

Allow a FileBase (normally a FileSystemLike) to be set as the default, so it can be looked up as "/default" as well as its actual name.

Intended to be used to extend the implementation in #7774. Once that is merged, we can add a commit to do that in this PR.

Expected pattern is

FileSystem *FileSystem::get_default_instance()
{
    static FATFileSystem sdcard("sd", sdcard_block_device);
    sdcard.set_as_default();
    return &sdcard;
}

This will allow simple code using "/default" to interwork with custom-configured setups preferring to use explicit mount names.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Breaking change

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-storage Aug 29, 2018

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:filebase_default branch from c048d92 to 6a2d56a Aug 29, 2018

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:filebase_default branch from 6a2d56a to da4ae15 Aug 31, 2018

@yossi2le

This comment has been minimized.

Contributor

yossi2le commented Sep 2, 2018

The code is OK but, I do have a problem with doing things under the hood. This feature is actually aiming to cloak the system paths. It actually means that while an application assumes it writes to default/ path, it will go to different one and not exactly a predictable one.


if (_default == this) {
_default == NULL;
}

if (getPathType() == FilePathType) {

This comment has been minimized.

@offirko

offirko Sep 2, 2018

Contributor

This part was not locked by _mutex in the original code. Is it locked now by ScopedLock ?
If so, then note that 'remove_filehandle()' uses a lock of its own: filehandle_mutex->lock();
Consider the risk of deadlock.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Sep 3, 2018

Contributor

Ta for pointing that out - hadn't considered it. It seems okay, as mbed_retarget.cpp does not call out to File objects while holding its filehandle_mutex - it's lower-level.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 2, 2018

This needs review and consideration, as its functionality change, might be considered for 5.11 rather?

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Sep 3, 2018

The cloaking is already happening manually in #7774 - a filing system is being designated as the default, and is responding to "/default", so someone requesting via that name already doesn't know what it will be.

This improves the situation by allowing the device to still have a proper name. Without this, and just #7774, you might have this situation:

Platform A:

  • /default = FAT sd card
  • /littlefs = onboard storage

Platform B:

  • /default = onboard storage
  • /sd = FAT sd card

Like that, a portable program wanting to explicitly write to SD is going to struggle, as there's no consistent name - the default concept has got in the way.

With this change, we can do this, which seems better:

Platform A:

  • /default or /sd = FAT sd card
  • /littlefs = onboard storage

Platform B:

  • /sd = FAT sd card
  • /default or /littlefs = onboard storage

Programs can now reasonably request "/sd", "/littlefs" or "/default".

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Sep 3, 2018

This needs review and consideration, as its functionality change, might be considered for 5.11 rather?

Would be nice to get it in, as I fear that without it I fear #7774 may somewhat distort the effective API. A default shouldn't get in the way of explicit selection.

But on the other hand, we'd still need the extension to 7774 to do the renaming as per the previous comment, so maybe it's too late now.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 3, 2018

@davidsaada

Looks good to me.

@yossi2le

This comment has been minimized.

Contributor

yossi2le commented Sep 3, 2018

@kjbracey-arm . Ok, I see your point and agreed.
I just think that in that case, you should add the call to set_as_default also to the default implementation found in SystemSorage.cpp.

@offirko

offirko approved these changes Sep 3, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 3, 2018

/morph build

@adbridge adbridge added needs: CI and removed needs: review labels Sep 3, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 3, 2018

Build : SUCCESS

Build number : 2995
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7924/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 4, 2018

/morph test

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Sep 4, 2018

I just think that in that case, you should add the call to set_as_default also to the default implementation found in SystemSorage.cpp.

Just been creating a PR to do that, but I've noticed that 7774 went in using "/fs", rather than "/default", My comment calling "/fs" a tautology got thumbs up so I assumed you went with that.

Not sure what to do now. Here are the 3 possibilities I see, in my order of preference:

  1. Keep this as is, change tests (and docs?) to reference "/default".
  2. Extend this to have both /default and /fs as aliases.
  3. Change this to set /fs as the alias instead of /default.
@yossi2le

This comment has been minimized.

Contributor

yossi2le commented Sep 4, 2018

I just think that in that case, you should add the call to set_as_default also to the default >>implementation found in SystemSorage.cpp.

Just been creating a PR to do that, but I've noticed that 7774 went in using "/fs", rather than "/default", >My comment calling "/fs" a tautology got thumbs up so I assumed you went with that.

Sorry I missed the fs/ I have thought the big issue was to move back to the original naming pattern and use get_default_instance()

Not sure what to do now. Here are the 3 possibilities I see, in my order of preference:

Keep this as is, change tests (and docs?) to reference "/default".
Extend this to have both /default and /fs as aliases.
Change this to set /fs as the alias instead of /default.

It very easy to change the default to default/. need to fix SystemStorage.cpp which I believe should be fixed anyway to include a call to set_as_default().
and also change the tests at features/storage/TESTS/filesystem/general_filesyste/main.cpp to use default/ and not fs/

However, your solution does work even if it left "fs/" and "default/" will be an alias of "fs/". i thought this was the all idea?

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Sep 4, 2018

Just created #7977, which extends on this and flips everything to "/default". If you're okay with that, it would be awesome to try to get that into this release. I think the final result should make more sense to the end users.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Sep 4, 2018

However, your solution does work even if it left "fs/" and "default/" will be an alias of "fs/". i thought this was the all idea?

The idea was that we would use the mechanism maintain "physical" names like "/sd" or "/flash", in addition to supporting logical names. The initial only logical name is "/default", but we could have others in future with a general scheme. I want the devices' primary name - visible in config and errors - to be their physical one, so instantiate as "sd".

We could maintain "/fs" as a duplicate logical name for backwards compatibility, but at this point it would be cleaner to quickly switch before release, if people are okay with "/default".

If you look at 7977's final version, I think we have a great level of extra clarity on what's happening:

static LittleFileSystem flash("flash", BlockDevice::get_default_instance());
flash.set_as_default();

return &flash;

or

static FATFileSystem sdcard("sd", BlockDevice::get_default_instance());
sdcard.set_as_default();

return &sdcard;

The default mechanism is, intentionally, an obfuscation, so we've got to take care we don't "over-obfuscate". We still need some real names backing the system - even if "good code" doesn't reference it directly, having standard conventions allowing people to re-use config files specifying "/sd/xxx" is worthwhile.

@mbed-ci

This comment has been minimized.

@yossi2le

This comment has been minimized.

Contributor

yossi2le commented Sep 4, 2018

@kjbracey-arm it sounds Ok to me.

@geky

This comment has been minimized.

Member

geky commented Sep 4, 2018

This is one of the APIs that needs a long-term change. My current thoughts are to split the FileBase out of the inheritance tree into it's own MountPoint class (the current FileBase class has already caused the duplication of all major file system interfaces, FileLike/FileHandle, FileSystemLike/FileSystemHandle).

For the short term why not just add "/default" to the config system? "platform.default_mount_point" or something?

I'm also not sure it's a big rush because moving "/default" to the config system should be a backwards-compatible change. But it's possible I'm missing something.

return NULL;
}

FileBase *FileBase::get(int n)
{
_mutex->lock();
ScopedLock<PlatformMutex> lock(*_mutex);

This comment has been minimized.

@geky

geky Sep 4, 2018

Member

This is the wrong PR to change these locking calls.

I'm not opposed to this change, but every change invites bugs.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Sep 5, 2018

Contributor

You're not wrong - I certainly wouldn't do this in a patch/fix. Probably shouldn't be doing it in rc2 either.

I can undo this, but as the current test set has passed, and we might be accepting this version otherwise, is it worth restarting CI just to take it out? @0xc0170 ?

I'll prepare a removal anyway in case I do another spin, but won't push yet.

This comment has been minimized.

@geky

geky Sep 5, 2018

Member

I agree not to block it now, it's just not a good habit to get into

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Sep 5, 2018

Contributor

Actually another CI round is required anyway for part 2 - #7977, so we could still change it.

* @returns
* A reference to the singleton
*/
T &operator*()

This comment has been minimized.

@geky

geky Sep 4, 2018

Member

Isn't this taken care of by the T *operator->() operator?

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Sep 5, 2018

Contributor

No - the alternative here would be lock(*_mutex.get()), which seemed unnecessarily awkward.

This comment has been minimized.

@geky

geky Sep 5, 2018

Member

Ah you're right, I though operator-> was full magic, but it's only half magic 👍

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Sep 5, 2018

Contributor

That does cover the "I want a reference" case, but I wonder if we should also have a conversion operator to T * for people who want the pointer. Does the get() really have to be explicit? Is there any C++ trap there for the conversion operator?

@JaniSuonpera

Tested changes with mbed-os-5.10.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Sep 5, 2018

I agree that FileBase would be due for a significant rework - not clear how much a File and FilingSystem have in common - and a proper mount point system could lead to that.

For the short term why not just add "/default" to the config system? "platform.default_mount_point" or something?

So change the model "get_default_instance()" implementations to name them as "/sd" or "/flash", and make the JSON have a setting telling you what that name is?

That would be possible, but it's not quite as flexible - "FileSystem::get_default_instance()" is a run-time call, so the default mount point can be a run-time decision (eg depending on if an SD card is present), or it can be based on conceptual #ifdefs like the default weak version in #7774. A config name specification couldn't mirror that. And I was trying to preserve the design choice of #7774 that the default mount point has a well-known name - I think it's a useful convenience.

You could have a separate FileSystem::get_default_mount_name() call alongside the get_default_instance(), but it's less convenient - you have to start doing the strcats.

I'm imagining app config files like

"key-storage" = "/default/keys/",
"image-storage" = "/default/images/",
"target_overrides" = {
       "platform_x" = {
            "key-storage" = "/secure/keys/",
            "image-storage" = "/flash/images/"

That app can run on anything that's got some sort of general-purpose filing system, but for specific platforms it has specific set-up code and configuration.

My view is that these two modes of operation - explicitly specified and "default" for the mount point should be able to co-exist, and 7774 as it stands makes that tricky - if you make the sdcard be the default you can't any longer explicitly refer to it as /sd, and if you name it as "/sd" for explicit configuration, you can't coexist with code expecting to just use "/default" - you have to explicitly configure everything or nothing.

The somewhat app-specific boot-up code for platform_x to back that would contain

SecureFS secure("secure", ...);
LittleFS flash("flash", ....);
flash.set_as_default();

FilingSystem *FilingSystem::get_default_instance() {
    return &flash;
}

So all filing systems are present and named for config to refer to, and one of them is still marked as default so the basic get_default_instance API and "/default" remains functional. And this setup has to be somewhat app-specific as you have to know which filing systems are worth instantiating for a particular app, and you may have app-specific FS configuration requirements (eg speed vs lifetime tuning parameters).

Add special handling for "/default" filesystem
Allow a FileBase (normally a FileSystemLike) to be set as the default,
so it can be looked up as "/default" as well as its actual name.
@geky

This comment has been minimized.

Member

geky commented Sep 5, 2018

That would be possible, but it's not quite as flexible - "FileSystem::get_default_instance()" is a run-time call, so the default mount point can be a run-time decision

Though note that choosing the name of the mount point is not a runtime call:

FileSystem *fs = FileSystem::get_default_instance();

There's nothing there that specifies what the FS's name is. At the moment this is a compile time decision as far as I can tell.

My view is that these two modes of operation - explicitly specified and "default" for the mount point should be able to co-exist, and 7774 as it stands makes that tricky

I agree, but I don't think we're there yet. At the moment the underlying retarget code only supports 1 mount point per FS.


Another short-term option, what if we add "/default" to the config system, and make the default "/default" NULL. This effectively does not mount the FS to the tree.

That would leave things open to add multiple mount points and reintroduce "/default" properly next release.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Sep 5, 2018

There's nothing there that specifies what the FS's name is. At the moment this is a compile time decision as far as I can tell.

This came up during review, and the agreement was that as there was no way to pass a FilingSystem to fopen etc, to make this usage possible there would instead be a convention, which those initial implementations are setting down - the default filing system has a well-known name, so after making that call, code or config can assume the name.

 // make sure we have it, and it's initialised and linked in
 FilingSystem *fs = FilingSystem::get_default_instance();
 if (!fs) {
       error("no filing system!");
 }
 // use it - either via well-known name (but wouldn't work without the call above)
 FILE *log = fopen("/default/log");
 // or explicitly from return value
 File *log = fs->open("log");

So given that, that means that the sdcard is called "default" (or "fs"), not "sd", because without this PR there's no way to present "default" as an alias. I'm happy with the "default" name, but I'm regretting the loss of "sd".

The other possibility here would be to junk the idea of a well-known name and make code extract it. I'm not sure this option was properly considered:

 FilingSystem *fs = FilingSystem::get_default_instance();   
 const char *fs_name = fs->getName();

 sprintf(fname, "/%s/log", fs_name)
 FILE *log = fopen(fname);

I'd be okay with that in code, but does preclude the short-cut of just doing "/default/foo" in the config file - using the default and using something explicit would actually be different code at each call site, not just flipping config between "/default/foo" and "/sd/foo". I do like the concept of it ultimately always just coming down to a string constant.

Another short-term option, what if we add "default" to the config system, and make the default "default" NULL. This effectively does not mount the FS to the tree.

Sorry, you've lost me now. Can you be more specific?

That would leave things open to add multiple mount points properly next release.

Once something like that is in place, the set_as_default() call just becomes a wrapper around mount(fs, "/default"), and /default would be just 1 well-known mount name.

kjbracey-arm added some commits Sep 4, 2018

Change default filesystem name to /default
"/fs" is a tautology - not a good name for the default filing system, as
whereever we use it, we know we're specifying a filing system. Rename to
"/default".
Make "/default" an alias - add real names too
Make the built-in FileSystem::get_default_instance() implementation
instantiate storage as "flash" or "sd", with "default" as an alias.

This will aid interworking between simple and advanced filesystem code
on various platforms. The expectation is that the names "sd" or "flash"
will be always available if the device is available and configured,
regardless of what "default" represents.
@geky

This comment has been minimized.

Member

geky commented Sep 5, 2018

the default filing system has a well-known name, so after making that call, code or config can assume the name.

Doesn't that still make this a compile time decision?

I'm happy with the "default" name, but I'm regretting the loss of "sd".

This is a new API, and it's provided at the target level. Unless this target is mounting the FS as both "default" and "sd" (which I don't believe it is?), we won't have "sd".

Unless the user declares an FS as "sd", but then it won't be a part of the get_default_instance.

Actually that raises a separate question. Does FileBase::set_as_default change what FileSystem::get_default_instance returns?

"default" NULL.... Sorry, you've lost me now. Can you be more specific?

FileBase supports NULL, which FileSystem::get_default_instance() does not need to create a mount point. Though it sounds like from your comment this isn't what we're looking for.

Once something like that is in place, the set_as_default() call just becomes a wrapper around mount(fs, "/default"), and /default would be just 1 well-known mount name.

True, I originally thought the cost was 1 mount point per filesystem, but after looking at it again we only need a global one. So it's a minor cost.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Sep 5, 2018

Doesn't that still make this a compile time decision?

I would say it's a design-time decision, if it's a decision at all - it's was intended to be an API invariant, not something that would change between programs, builds or platforms.

Unless the user declares an FS as "sd", but then it won't be a part of the get_default_instance.

But why not? Why can't someone have 2 or 3 explicitly-mounted filing systems, and have one of them be the default? That's what I've shown in several of my examples, and that is the motivation for my wanting a change here. See this comment here: #7924 (comment)

I don't want there to be this exclusion - either you have exactly 1 filing system which is accessible via the "get_default" or you have multiple, but there's no default. The very name "get_default" suggests it's 1 of potentially several. For the simple case there is this default weak instantiation of "get_default_instance" that spawns the (only?) FS on demand, but the intent is that you can override to point at something you've explicitly set-up yourself, as per this comment: #7924 (comment)

But when you do that, the current API contract is that the thing you point the get_default_instance to has to be given the well-known name. Without aliases you have the situation I don't want which is "the sdcard is conventionally mounted as /sd, unless it's the default." Surely it's simpler that it's conventionally explicitly accessible as "/sd", whether or not it's the default?

Actually that raises a separate question. Does FileBase::set_as_default change what FileSystem::get_default_instance returns?

No, the causality is in the other direction - whoever is providing FileSystem::get_default_instance() can call FileBase::set_as_default() on what they provide to satisfy the "must be named as /default" postcondition. Maybe the method could have a clearer name - I did consider "name_as_default()".

The simple get_default_instance() implementation has to be a "pull" API that instantiates on-demand for linker exclusion. If you had it just return what was pushed in by "set_as_default", then the linker would include a filing system due to the set whether or not anyone called get_default_instance.

In the more complex examples I've given previously, the assumption is that you have some application knowledge so you know what filing systems are worth mounting and hence pulling into the image up-front.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:filebase_default branch from da4ae15 to c0dbc49 Sep 5, 2018

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Sep 5, 2018

I needed to correct an error in the string comparison, so redone. This time without the ScopedLock and SingletonPtr changes, and adding in the extra completion from #7977.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 5, 2018

@geky Alright with the spinout of #8001?

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 5, 2018

/morph build

@geky

geky approved these changes Sep 5, 2018

Yeah, thanks for taking those out 👍

My comments are concerns but not blockers

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 5, 2018

Build : SUCCESS

Build number : 3014
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7924/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr added ready for merge and removed needs: CI labels Sep 6, 2018

@cmonr cmonr merged commit 6c30b2f into ARMmbed:master Sep 6, 2018

14 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0.0%) RAM(+0.0%)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job: successful
Details
travis-ci/astyle Passed, 598 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9957 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the ready for merge label Sep 6, 2018

@kjbracey-arm kjbracey-arm deleted the kjbracey-arm:filebase_default branch Sep 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment