-
Notifications
You must be signed in to change notification settings - Fork 794
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
Removed invalid overwrite for return value. #638
Conversation
Hi @lmapii, thanks for the PR. This definitely shouldn't mask all errors like it is. The original motivation for that condition was to handle a difficult case where the file name doesn't fit in our metadata blocks. For example a 256 byte file name will never fit in a 128 byte block (which is in theory allowed for littlefs but causes problems like this one). Looking into this more, maybe we should consider adding a new error ( At the very least, this shouldn't catch/hide all errors. We should change this to only change the error for |
@@ -2514,7 +2514,6 @@ static int lfs_file_rawopencfg(lfs_t *lfs, lfs_file_t *file, | |||
{LFS_MKTAG(LFS_TYPE_REG, file->id, nlen), path}, | |||
{LFS_MKTAG(LFS_TYPE_INLINESTRUCT, file->id, 0), NULL})); | |||
if (err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider change this to if (err == LFS_ERR_NOSPC) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was about to add the following change:
// it can happen that the file name doesn't fit in the metadata blocks, e.g., a 256 byte file name will
// not fit in a 128 byte block
err = (err == LFS_ERR_NOSPC) ? LFS_ERR_NAMETOOLONG : err;
if (err) {
goto cleanup;
}
But I'm not sure if that would resolve the issue that I've stumbled upon, since then again I would receive LFS_ERR_NAMETOOLONG
. My scenario was quite special though in the sense that I've had bugs in my integration (both block device driver and the CRC function, which I've changed to use available CPU instructions). My favourite resolution would simply be a comment documenting your case and return the original error code.
To be honest I'm not too familiar with the details of
In both cases Adding a comment to describe either scenario would be very helpful. |
I think the problem is that In the first case In the second case (The reason In hindsight we probably should have used a different error, perhaps
I may be biased as a maintainer, but the number of issues I've seen so far about mostly empty filesystems returning |
Ok I see how there are other use-cases which might affect more users. I've updated the PR to only convert the |
Thanks for the PR! |
While working on a
littlefs
integration I've received the return valueLFS_ERR_NAMETOOLONG
for several error scenarios where it doesn't apply. I think the affected line may have been copied by mistake.E.g., I've received this return value for an invalid block device driver where I've messed up the offsets within the memory and also with bugs in an optimized CRC32 calculation.