Skip to content

Conversation

@roykuper13
Copy link
Contributor

Since lfs_init validates lfs config attributes, it should also check that the attributes are initiated properly, in order to not crash, for example, if read_size or prog_size or cache_size are not set (floating point excpetion).

Also, those attributes are really critical for lfs, and if the user didn't set those values, he must be aware of that (assertion error is pretty indicative, unlike a weird floating point exception..).

@roykuper13 roykuper13 force-pushed the validate-lfs-cfg-sizes branch from be0561d to 7f000cb Compare August 31, 2019 13:52
@roykuper13 roykuper13 force-pushed the validate-lfs-cfg-sizes branch from 7f000cb to 0c77123 Compare August 31, 2019 13:58
@geky
Copy link
Member

geky commented Sep 4, 2019

Looks good to me, should we also assert that read/prog/erase/sync are non-null?

@roykuper13
Copy link
Contributor Author

Looks good to me, should we also assert that read/prog/erase/sync are non-null?

we should, but maybe not in lfs_init, because if, for example, someone wants to implement a read-only filesystem using littlefs, a prog() function is irrelevant (I hope this example makes any sense, correct me if i'm wrong). therefore we can't assert that prog is non-null in lfs_init.

I suggest to assert each function where they are being used. for example, lfs_bd_flush should first check that lfs->cfg->prog != NULL.

tell me what you think.

@geky geky merged commit fd204ac into littlefs-project:master Sep 19, 2019
@geky
Copy link
Member

geky commented Sep 19, 2019

I've gone ahead and merged the original PR, thanks for pushing it up. Feel free to create another PR if you think we should add more checks.

The main issue with asserting where used is that I think that is basically every entry-point to the filesystem, which is quite a number of functions. Including mount. So we might as well assert during mount/format and assume the user won't change the config struct while the filesystem is mount (the user could also change geometry such as block_size, which would likely break the filesystem).

We don't want too many asserts as then we risk making asserts to expensive to enable on smaller MCUs.

That's a good point about a read-only filesystem though. Given the way the filesystem API works with files and flags, we already would need a build-time define to configure read-only mode, so we could use that to decide if we assert on write functions.

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