Add value-range checks for user-definable macros at compile-time

BrianPugh commented 10 months ago

Added some sanity check for the user-providable #define macro values.

This was brought up due to a bug introduced by yours truly 💅 https://github.com/jrast/littlefs-python/issues/61

geky-bot commented 10 months ago

Tests passed ✓, Code: 16838 B (+0.0%), Stack: 1448 B (+0.0%), Structs: 800 B (+0.0%)

| | Code | Stack | Structs | | Coverage | |:--|-----:|------:|--------:|:--|---------:| | Default | 16838 B (+0.0%) | 1448 B (+0.0%) | 800 B (+0.0%) | Lines | 2357/2533 lines (+0.0%) | | Readonly | 6130 B (+0.0%) | 448 B (+0.0%) | 800 B (+0.0%) | Branches | 1202/1528 branches (+0.0%) | | Threadsafe | 17722 B (+0.0%) | 1448 B (+0.0%) | 808 B (+0.0%) | | **Benchmarks** | | Multiversion | 16898 B (+0.0%) | 1448 B (+0.0%) | 804 B (+0.0%) | Readed | 29369693876 B (+0.0%) | | Migrate | 18514 B (+0.0%) | 1752 B (+0.0%) | 804 B (+0.0%) | Proged | 1482874766 B (+0.0%) | | Error-asserts | 17498 B (+0.0%) | 1440 B (+0.0%) | 800 B (+0.0%) | Erased | 1568888832 B (+0.0%) |

BrianPugh commented 10 months ago

Would be nice, to reference the design/spec for the 1022 value (I know, where it comes from, but without that ref, a casual reader of the source may believe this to be in error.

It's actually not referenced in SPEC.md or anywhere else beyond lfs.h. I could say "reference lfs.h" in the warning if we'd like.

BenBE commented 10 months ago

Would be nice, to reference the design/spec for the 1022 value (I know, where it comes from, but without that ref, a casual reader of the source may believe this to be in error.

It's actually not referenced in SPEC.md or anywhere else beyond lfs.h. I could say "reference lfs.h" in the warning if we'd like.

Rechecked the DESIGN.md and SPEC.md and they both only mention the bit-field size this value is used in and some special values that need to be avoided. Would be nice to have those consequences to limits pointed out a bit more directly in the design/spec docs; not just in the header/implementation.

geky commented 10 months ago

Point taken SPEC.md could document these limits better, but I don't think lfs.h/lfs.c should reference SPEC.md. They should both contain copies of all relevant documentation so a reader doesn't need to jump between documents.

If lfs.h/lfs.c and SPEC.md fall out of sync you have bigger problems.

I'm curious, is there an issue with putting these in lfs.h? Then they could be next to the relevant comments.

If not (multiple include warning spam?), then maybe just a comment saying something like "Must fit in 10-bits with 1023 reserved, i.e. <= 1022." is sufficient.

BrianPugh commented 10 months ago

I'm curious, is there an issue with putting these in lfs.h? Then they could be next to the relevant comments.

I wanted to keep the header file clean and focus on the library API. I didn't want to crowd the header file with sanity checks, but I'm open to either option.

geky-bot commented 10 months ago

Tests passed ✓, Code: 16838 B (+0.0%), Stack: 1448 B (+0.0%), Structs: 800 B (+0.0%)

| | Code | Stack | Structs | | Coverage | |:--|-----:|------:|--------:|:--|---------:| | Default | 16838 B (+0.0%) | 1448 B (+0.0%) | 800 B (+0.0%) | Lines | 2357/2533 lines (+0.0%) | | Readonly | 6130 B (+0.0%) | 448 B (+0.0%) | 800 B (+0.0%) | Branches | 1202/1528 branches (+0.0%) | | Threadsafe | 17722 B (+0.0%) | 1448 B (+0.0%) | 808 B (+0.0%) | | **Benchmarks** | | Multiversion | 16898 B (+0.0%) | 1448 B (+0.0%) | 804 B (+0.0%) | Readed | 29369693876 B (+0.0%) | | Migrate | 18514 B (+0.0%) | 1752 B (+0.0%) | 804 B (+0.0%) | Proged | 1482874766 B (+0.0%) | | Error-asserts | 17498 B (+0.0%) | 1440 B (+0.0%) | 800 B (+0.0%) | Erased | 1568888832 B (+0.0%) |

geky commented 8 months ago

I wanted to keep the header file clean and focus on the library API. I didn't want to crowd the header file with sanity checks, but I'm open to either option.

That makes sense, I think you're right these should probably stay in the .c file.

Controversial thought: What if these were runtime asserts and live in lfs_init? That way they live with all of the other configuration-level asserts. And while they do have a cost at runtime... in theory runtime asserts have no cost when it matters.

Unrelated, how badly do you want this in the next minor release? I think think the next minor release can be cut pretty much whenever.

BrianPugh commented 8 months ago

What if these were runtime asserts and live in lfs_init? That way they live with all of the other configuration-level asserts.

You mean just from an organizational standpoint? Seems fine by me. This wouldn't even make it to the compiled code since these all get handled by the preprocessor, so there wouldn't be any runtime impact.

how badly do you want this in the next minor release?

I don't have any strong opinions, would just like these asserts to make it into the codebase at some point so other people avoid making stupid mistakes 😄

geky commented 8 months ago

You mean just from an organizational standpoint? Seems fine by me.

I was thinking more make these actual runtime asserts, but I've compromised by just moving them near lfs_init. Compile-configuration needs a rework anyways (https://github.com/littlefs-project/littlefs/pull/491 is related).

I've updated this PR with what was discussed in the PR. Feel free to let me know if anything looks wrong. I think this can make it into the next minor release.

One maybe controversial change: I removed the check against <=0:

LFS_FILE_MAX <= 0, zero-sized files are technically different from not-created files. It is theoretically possible to have a filesystem where every file is effectively a boolean flag of does/does-not exist. Is this reasonable? probably not. But no reason we should disallow it on principle.
LFS_NAME_MAX <= 0, a little bit harder to argue for, but technically you could create a zero-length filename in each directory. As long as the directory's filename is also zero-length. Which would imply the entire filesystem is a linked-list of zero-length directories terminating in a single file, but reasonableness aside, I don't think we should assert in this case. It's not an error as far as littlefs is concerned, just crippling and weird.

Anyways, let me know if anything looks concerning, otherwise I'm happy to merge this PR into the next minor release.

geky-bot commented 8 months ago

Tests passed ✓, Code: 16828 B (+0.0%), Stack: 1448 B (+0.0%), Structs: 800 B (+0.0%)

| | Code | Stack | Structs | | Coverage | |:--|-----:|------:|--------:|:--|---------:| | Default | 16828 B (+0.0%) | 1448 B (+0.0%) | 800 B (+0.0%) | Lines | 2357/2533 lines (+0.0%) | | Readonly | 6130 B (+0.0%) | 448 B (+0.0%) | 800 B (+0.0%) | Branches | 1202/1528 branches (+0.0%) | | Threadsafe | 17696 B (+0.0%) | 1448 B (+0.0%) | 808 B (+0.0%) | | **Benchmarks** | | Multiversion | 16892 B (+0.0%) | 1448 B (+0.0%) | 804 B (+0.0%) | Readed | 29369693876 B (+0.0%) | | Migrate | 18508 B (+0.0%) | 1752 B (+0.0%) | 804 B (+0.0%) | Proged | 1482874766 B (+0.0%) | | Error-asserts | 17484 B (+0.0%) | 1440 B (+0.0%) | 800 B (+0.0%) | Erased | 1568888832 B (+0.0%) |

BrianPugh commented 8 months ago

Looks good to me, I think your reasoning is sound. The upper limits are much more easily broken by accident, and are the more important checks.

geky commented 8 months ago

This is on master now, thanks for the PR!

littlefs-project / littlefs

Add value-range checks for user-definable macros at compile-time #886