Littlefs: lfs_dir_read return 1 on success instead of LFS_ERR_OK

[hathach]

for consistency, should lfs_dir_read return LFS_ERR_OK on success

https://github.com/ARMmbed/littlefs/blob/master/lfs.c#L1059

[geky]

Hi @hathach, lfs_dir_read returns 1 if it finds a directory entry, and 0 (LFS_ERR_OK) when it encounters the end of the directory.

In POSIX, readdir returns either a dirent pointer or NULL. We don't return a pointer, but still need to indicate when we've reached the end of the directory, so we use 1 and 0 to roughly match the return values from lfs_file_read.

[hathach]

@geky thanks for quick reply. My bad, looking at the API, I was thinking the function will return LFS_ERR_NOENT when reaching the end of directory and LFS_ERR_OK when I could find one.

[geky]

Hmm interesting, I'm going to reopen this for further investigation. Sorry I didn't see this earlier. I'm wondering if what you propose would actually make a better API.

I was thinking the function will return LFS_ERR_NOENT when reaching the end of directory and LFS_ERR_OK when I could find one.

It doesn't match the return codes of lfs_file_read, but I don't know if that is expected.

[hathach]

Yeah, it is the first thought crossed my mind when looking at function header without reading its documentation :)

[FreddieChopin]

Same here, I did the same thing. And not only me - what I stumbled upon some time ago: https://meh.schizofreni.co/2019-04-09/lies-damned-lies-and-documentation

EDIT: If you would consider changing the API (a breaking change anyway), maybe just making it more POSIX-like would solve this problem? Mimicking readdir_r() seems like a good idea - instead of int lfs_dir_read(lfs_t *lfs, lfs_dir_t *dir, struct lfs_info *info) it would be int lfs_dir_read(lfs_t *lfs, lfs_dir_t *dir, struct lfs_info *info, struct lfs_info **result). The problem with changing just the value returned is that it is a very "silent" change - easy to miss by end users, but all their uses of this function will now need to change with no compiler warning or whatever. Changing the number of arguments is pretty "loud", no way to miss that. As a bonus it would be easy to provide a legacy warapper which would behave just the same as the old version of lfs_dir_read() (it would obviously need a different name, another problem easily solvable in C++ [; ).

[geky]

If you would consider changing the API (a breaking change anyway)

Yes, this will only go in if there is a major version bump. Unfortunately that means this issue may be hanging for a long time.

The problem with changing just the value returned is that it is a very "silent" change - easy to miss by end users

I agree it's unfortunate, but I'm not sure API decisions should be made for this reason alone. Though this change should definitely have a lot of notice around it.

[geky]

Made a PR for this here: https://github.com/littlefs-project/littlefs/pull/512, and only 2 years later. Though there's a number of things to take care of before making a new major release (non-disk-breaking), so it may be still be open for a bit.