Assert that the file isnt open in lfs_file_opencfg #443

NoahGorny · 2020-06-17T17:57:44Z

Closes #429

Also fixed a magic number in opencfg

e107steved · 2020-06-18T13:29:42Z

I think I'd prefer the operation failed by returning an error, rather than doing an assert

NoahGorny · 2020-06-18T14:13:32Z

I think I'd prefer the operation failed by returning an error, rather than doing an assert

I think that checking every time for something that is really a user error is not needed, but I will accept this if more people prefer checks. If your program hangs in littlefs I expect that you will compile littlefs in debug and then you will see the problem.

In a different matter, I really need to find a way to check if the struct was initialized, because right now I access uninitialized memory :(

judgeh · 2020-06-26T21:58:34Z

I definitely prefer the return code. I think asserts are heavily overused in lfs. there are many cases where my application would benefit from handling return codes rather than the asserting. It is a user api. Errors should be propagated to the user. library code should only assert in fatal cases, not misuse.

Also FWIW, it is not necessarily a user error to open an already open file. this is a perfectly valid case. For instance, we have a logging module that logs to the filesystem. We also have a remote protocol that can perform filesystem operations. It is a valid scenario to attempt to remotely read a file that may or may not be open by the logging framework. (this is exactly what brought me to this issue). As of right now, the remote read reports the file has zero bytes, which is not correct. An error reporting it is already open would be very useful.

diederikvdv

Besides a check whether a file is already opened also a check whether a dir is already opened is suggested.

Error reporting rather than an Assert sounds better to me. For our multi-threading system the same files can be attempted to be opened. An internal check whether opening is possible can give added value.

diederikvdv · 2020-07-01T12:26:33Z

lfs.c

                 ".buffer=%p, .attrs=%p, .attr_count=%"PRIu32"})",
            (void*)lfs, (void*)file, path, flags,
            (void*)cfg, cfg->buffer, (void*)cfg->attrs, cfg->attr_count);
+    LFS_ASSERT(is_node_not_in_list(lfs->mlist, (struct lfs_mlist*)file));


This check can also be added to the lfs_dir_open function:
LFS_ASSERT(is_node_not_in_list(lfs->mlist, (struct lfs_mlist*)dir));

This requires the static function to be declared earlier. Other static functions are declared at the top of the file

added, thanks for the feedback!

NoahGorny · 2020-07-02T18:21:25Z

Guys, I am not sure why you should return error on such cases. Iterating over all open files in each open call seems costly, so I think we can stick to an assert. You can open the same file twice, just not with the same struct...

judgeh · 2020-07-02T18:25:28Z

You can open the same file twice, just not with the same struct...

Oh in our testing, we were unable to open and read a file twice with different structures.
we

opened a file with hdl1
wrote data using hdl1
synced hdl1 (but not close)
opened same file with hdl2
read hd2 (this always reported zero bytes)

Do you expect, in this scenario, the hdl2 read to return the bytes written by hdl1 ? Maybe our test case was buggy, but we concluded that we could not read a file that was already open. If this is not the case, then thats great.

judgeh · 2020-07-02T19:17:46Z

Also, isnt it still checking every open file in the assert? How is asserting any different than returning in terms of complexity ?

NoahGorny · 2020-07-02T19:32:41Z

I will check the test case you mentioned, thanks!
I currently only check that the struct is not reused, in order to test that the file is not already opened, I will need to add a new assert.

And assert is better because it is not complied in, unless on debug

geky · 2020-11-16T23:52:40Z

Hi @NoahGorny, thanks for creating a PR! Sorry to leave this hanging for so long. This is a good addition.

I wonder if it would also be good to replace LFS_F_OPENED (#237) so we have the same check for open/close files that doesn't get tripped up by uninitialized memory. Thoughts?

My opinion is this check should assert. For a code size reason, not a usability reason. Asserts can be removed during compilation, return codes can not. And for microcontroller-scale programs, you really don't want to pay for code that should never run.

Also the current behavior for these types of user checks is to assert, if we want to change this behavior it should be done via a different issue or PR.

geky · 2020-11-16T23:52:59Z

lfs.c


    // deorphan if we haven't yet, needed at most once after poweron
-    if ((flags & 3) != LFS_O_RDONLY) {
+    if ((flags & LFS_O_RDWR) != LFS_O_RDONLY) {


I changed in all places now 😄

Ah I misread this. Well, I don't really have an opinion on this change. It is a curious mask.

I think this is better than a magic num

Can't argue with that

geky · 2020-11-16T23:53:55Z

lfs.c

 }

+static inline bool is_node_not_in_list(struct lfs_mlist *head,
+    struct lfs_mlist *node) {


style nit: Two indents when inside parens (I realize I don't have this documented anywhere...)

geky · 2020-11-16T23:56:01Z

lfs.c

    superblock->attr_max    = lfs_tole32(superblock->attr_max);
 }

+static inline bool is_node_not_in_list(struct lfs_mlist *head,


nit: Prefix with lfs_. Thoughts on lfs_mlist_isopen for consistency with the other functions?

done, changed to lfs_mlist_isused

diederikvdv · 2020-11-17T08:07:01Z

I wonder if it would also be good to replace LFS_F_OPENED (#237) so we have the same check for open/close files that doesn't get tripped up by uninitialized memory. Thoughts?

If LFS_F_OPENED (#237) is replaced, it might also be a good addition to add the isOpened API mentioned in #442. But then based on lfs_is_node_not_in_list instead of LFS_F_OPENED

NoahGorny · 2020-11-17T22:42:43Z

Hi @NoahGorny, thanks for creating a PR! Sorry to leave this hanging for so long. This is a good addition.

I wonder if it would also be good to replace LFS_F_OPENED (#237) so we have the same check for open/close files that doesn't get tripped up by uninitialized memory. Thoughts?

I agree, and added commits that deprecate LFS_F_OPENED. It required me to sometimes add files to the mlist when we did not do so before, so you might wanna take a look at 0250015 and 0ca3012 to tell me whether my solution is okay or not.
In any case, I did not add support for asserting of file that was opened twice using a different file struct, so we might want to take a look at it. Reusing an existing struct will now raise assert errors 😄
By the way, are there any ways to "catch' asserts in the tests? I want to deliberately see the asserts trigger in the tests.

Thanks alot for your feedback btw @geky

NoahGorny · 2020-11-17T22:55:10Z

I wonder if it would also be good to replace LFS_F_OPENED (#237) so we have the same check for open/close files that doesn't get tripped up by uninitialized memory. Thoughts?

If LFS_F_OPENED (#237) is replaced, it might also be a good addition to add the isOpened API mentioned in #442. But then based on lfs_is_node_not_in_list instead of LFS_F_OPENED

I think @geky should voice his opinion here. I think that currently with the assert not fully catching all cases (opened with different struct) it should not be public API yet... maybe after we catch/handle all such cases

NoahGorny · 2020-11-17T22:57:24Z

You can open the same file twice, just not with the same struct...

Oh in our testing, we were unable to open and read a file twice with different structures.
we
1. opened a file with hdl1

2. wrote data using hdl1

3. synced hdl1 (but not close)

4. opened same file with hdl2

5. read hd2 (this always reported zero bytes)
Do you expect, in this scenario, the hdl2 read to return the bytes written by hdl1 ? Maybe our test case was buggy, but we concluded that we could not read a file that was already open. If this is not the case, then thats great.

@judgeh It indeed fails with my tests, I created a test (on a separate PR) that fails on this (ad995ce)
I am not sure what is the correct approach to this, but I think @geky can have some insights 😄

geky · 2020-11-18T15:29:35Z

Oh in our testing, we were unable to open and read a file twice with different structures.

@judgeh It indeed fails with my tests, I created a test (on a separate PR) that fails on this (ad995ce)

Ah, this looks like the same issue being discussed in #483. Thumbing up #483 would help know who's ran into this. Also thanks for the test case!

TLDR: Currently, littlefs is transactional in the sense that each open file handle gets its own snapshot. Each file handle can be read/written independently (and independently from disk). Something that came up in #483, we could go through all open files and throw out other changes on sync/close, it would just take more code size. More info in #483.

geky · 2020-11-18T15:40:15Z

lfs.c

+    return false;
+}
+
+static inline void lfs_mlist_add(lfs_t *lfs, struct lfs_mlist *mlist) {


If we add this, lfs_mlist_remove would be more valuable:
https://github.com/littlefs-project/littlefs/blob/4c9146ea539f72749d6cc3ea076372a81b12cb11/lfs.c#L2553-L2559

Also a nit: I think lfs_mlist_append captures the data structure operation better.

geky · 2020-11-18T15:41:14Z

lfs.h

    LFS_F_ERRED   = 0x080000, // An error occured during write
    LFS_F_INLINE  = 0x100000, // Currently inlined in directory entry
-    LFS_F_OPENED  = 0x200000, // File has been opened
+    LFS_F_OPENED  = 0x200000, // DEPRECATED: File has been opened


Since these are marked as "internally used flags", I think it's safe to remove without deprecation.

removed the change entirely

geky · 2020-11-18T15:41:57Z

lfs.c

    superblock->attr_max    = lfs_tole32(superblock->attr_max);
 }

+static inline bool lfs_mlist_isused(struct lfs_mlist *head,


nit: I think this would be better as lfs_mlist_isopen, it is directly related to lfs_file_open and lfs_dir_open.

I can change this on merge if it's a bother.

its okay, changed it

It looks like this is still lfs_mlist_isused, or am I missing a commit?

sorry, updated it now 😄

geky · 2020-11-18T16:00:11Z

lfs.c

                .cache = lfs->rcache,
            };
+            // So we could use lfs_read on the file
+            lfs_mlist_add(lfs, (struct lfs_mlist *)&orig);


Ah, would not add to the mlist here. The original LFS_F_OPENED flag wasn't strictly necessary.

This internal file is only created for code reuse, adding to the mlist may cause problems if wear-leveling needs to scan open files during lfs_file_write.

geky · 2020-11-18T16:04:32Z

lfs.c

+    // get id, add to list of mdirs to catch update changes
+    file->type = LFS_TYPE_REG;
+    lfs_mlist_add(lfs, (struct lfs_mlist *)file);
+


I wouldn't move this either, the lfs_mdir_t is not initialized until the lfs_dir_find, and I'm not sure having an uninitialized mdir on in mlist is safe.

Unfortunately the mlist is a bit sensitive because all wear-leveling/allocation logic depend on it. It would be better if we prefer changing the LFS_F_OPENED behavior slightly to avoid changing the mlist logic, since LFS_F_OPENED asserts are just protection against already invalid API usage.

its honestly better to leave alone the LFS_F_OPENED flag honestly. It causes those problems with the assert and the isused check can be utilized only when need (in file_opencfg and dir_open) and the rest of the API can stay with the flag

geky · 2020-11-18T16:13:17Z

it might also be a good addition to add the isOpened API mentioned in #442

I'm still not convinced on #442. The behavior of an lfs_file_isopened could be accomplished with an external flag without requiring an additional API into the filesystem. It could also be added later if there is interest.

Also thanks for the changes!

geky · 2020-11-18T18:56:47Z

By the way, are there any ways to "catch' asserts in the tests?

Almost missed this, unfortunately no, the test script currently only treats asserts as failures. The test script should also be documented better...

…_open

geky

This looks great, thanks for this! Will merge into the next release.

Assert that the file isnt open in lfs_file_opencfg

geky · 2020-12-04T07:28:28Z

I didn't notice until after merging that this reverted the LFS_F_OPENED flag, so I went ahead and re-added that change to #495. But can re-revert if needed.

I think I ran into some of the issues you did with the change, but one thing that helped was the new separate API-layer, I could move the check there and it solved some of complex internal situations, as well as reduced the calls to lfs_mlist_isopen to the API layer.

NoahGorny force-pushed the add-already-opened-assert branch from 647bfd1 to 3701b2a Compare June 18, 2020 15:51

diederikvdv suggested changes Jul 1, 2020

View reviewed changes

NoahGorny force-pushed the add-already-opened-assert branch from 3701b2a to a36f0a9 Compare July 2, 2020 18:19

geky mentioned this pull request Jul 19, 2020

API to check if file is currently open #442

Open

geky added the v2.3 label Nov 14, 2020

geky added this to the v2.3 milestone Nov 14, 2020

geky added the next minor label Nov 14, 2020

geky reviewed Nov 16, 2020

View reviewed changes

NoahGorny changed the title ~~Assert that the file isnt open is lfs_file_opencfg~~ Assert that the file isnt open in lfs_file_opencfg Nov 17, 2020

NoahGorny force-pushed the add-already-opened-assert branch from a36f0a9 to eff46f1 Compare November 17, 2020 22:35

geky reviewed Nov 18, 2020

View reviewed changes

NoahGorny force-pushed the add-already-opened-assert branch from eff46f1 to 95a81da Compare November 18, 2020 22:31

Noah Gorny added 2 commits November 19, 2020 01:51

Assert that file/dir struct is not reused in lfs_file_opencfg/lfs_dir…

4bd653d

…_open

Use LFS_O_RDWR instead of magic number in lfs_file_* asserts

6303558

Noah Gorny added 2 commits November 19, 2020 01:51

Add lfs_mlist_append helper

d1b2e27

Add lfs_mlist_remove helper

37ef506

NoahGorny force-pushed the add-already-opened-assert branch from 95a81da to 37ef506 Compare November 18, 2020 23:51

geky approved these changes Nov 20, 2020

View reviewed changes

geky changed the base branch from master to devel December 4, 2020 04:35

geky merged commit 2621b59 into littlefs-project:devel Dec 4, 2020

geky added a commit that referenced this pull request Dec 4, 2020

Merge pull request #443 from NoahGorny/add-already-opened-assert

584eb26

Assert that the file isnt open in lfs_file_opencfg

geky mentioned this pull request Dec 4, 2020

Minor release: v2.3 #495

Merged

geky mentioned this pull request Feb 8, 2021

No space left on device, but write data success #409

Open

Assert that the file isnt open in lfs_file_opencfg #443

Assert that the file isnt open in lfs_file_opencfg #443

Uh oh!

Conversation

NoahGorny commented Jun 17, 2020

Uh oh!

e107steved commented Jun 18, 2020

Uh oh!

NoahGorny commented Jun 18, 2020

Uh oh!

judgeh commented Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diederikvdv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NoahGorny commented Jul 2, 2020

Uh oh!

judgeh commented Jul 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

judgeh commented Jul 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NoahGorny commented Jul 2, 2020

Uh oh!

geky commented Nov 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diederikvdv commented Nov 17, 2020

Uh oh!

NoahGorny commented Nov 17, 2020

Uh oh!

NoahGorny commented Nov 17, 2020

Uh oh!

NoahGorny commented Nov 17, 2020

Uh oh!

geky commented Nov 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

judgeh commented Jun 26, 2020 •

edited

Loading

judgeh commented Jul 2, 2020 •

edited

Loading

judgeh commented Jul 2, 2020 •

edited

Loading

geky Nov 18, 2020 •

edited

Loading

geky commented Nov 18, 2020 •

edited

Loading