Flash errors appear to be ignored in a corner case of `lfs_dir_fetchmatch`

[cbiffle]

Hi! I'm not spectacularly familiar with this codebase, so my analysis here could be entirely wrong -- please let me know if I've missed something.

So I'm looking at the control flow through lfs_dir_fetchmatch, specifically around line 1164, which reads

                err = lfs_bd_read(lfs,
                        NULL, &lfs->rcache, lfs->cfg->block_size,
                        dir->pair[0], off+sizeof(tag), &temptail, 8);
                if (err) {
                    if (err == LFS_ERR_CORRUPT) {
                        dir->erased = false;
                        break;
                    }
                }

If lfs_bd_read (and by extension the flash driver layer) return an error code other than LFS_ERR_CORRUPT, it gets ignored. I've verified that the store to err is dead in that context, so it's not being returned somewhere else in the function, despite err being a function-scope mutable variable.

This seems suspicious because all the other error handling boilerplate in this function contains return statements, like you can see at line 1173:

                if (res < 0) {
                    if (res == LFS_ERR_CORRUPT) {
                        dir->erased = false;
                        break;
                    }
                    return res;  // <-- this here's a return statement!
                }

In those cases, if the flash driver layer were to return (say) LFS_ERR_IO, it would be reported up to the user, which is closer to the behavior I'd expect as a user.

[geky]

I think you're right, good catch!

Unfortunately we don't have great tests for non-LFS_ERR_CORRUPT block device errors at the moment. This probably deserves a dedicated effort at some point, The recently added coverage testing may help.

Though the non-LFS_ERR_CORRUPT bd error paths have been overlooked for a while as they are an exceptional case. State isn't cleaned up thoroughly in places so reentering the filesystem after an error may be risky without a remount. This should probably be improved in the future.

[geky]

Put up a quick PR here: #744, let me know if you see any issues with it.

If tests pass I don't see a reason to not bring it in on the next patch release.