Github/littlefs
1
Fork 0
Code Actions Activity

Made LFS_O_EXCL error if file is open but uncreated

Browse Source 
One of the unexpected side-effects of lazy file creation is that
suddenly LFS_O_EXCL doesn't make sense.

The standard definition: "Fail if the file exists", is easy enough to
implement, but doesn't really match what the user expects.

The user expects one of these calls to fail:

  lfsr_file_open(&lfs, &file_a, "file.txt",
          LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
  lfsr_file_open(&lfs, &file_b, "file.txt",
          LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;

But because we create files lazily (to prevent zero-length files after
powerloss), these both succeed.

---

I considered deferring the "file exists" check until we actually would
create the file, but while this _technically_ satisfies the
exclusitivity requirement, I decided against it as I think it just makes
the API way too confusing:

  lfsr_file_open(&lfs, &file_a, "file.txt",
          LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
  lfsr_file_open(&lfs, &file_b, "file.txt",
          LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
  lfsr_file_close(&lfs, &file_a) => 0;
  lfsr_file_close(&lfs, &file_b) => LFS_ERR_EXIST;

---

Instead, a simpler, more pragmatic approach: Fail if the file exists
_or_ if the file is open in a mode that will create the file:

  lfsr_file_open(&lfs, &file_a, "file.txt",
          LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
  lfsr_file_open(&lfs, &file_b, "file.txt",
          LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => LFS_ERR_EXIST;

This explicitly does _not_ error on zombie/desync files:

  lfsr_file_open(&lfs, &file_a, "file.txt",
          LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
  lfsr_file_desync(&lfs, &file_a) => 0;
  lfsr_file_open(&lfs, &file_b, "file.txt",
          LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;

And it does mean we aren't necessarily guaranteeing the file will be
created, but I think this does more-or-less what the user expects:

- open(a) -> desync(a) -> open(b) -> resync(a) is roughly equivalent to
  opening a after creating b, which is perfectly fine with LFS_O_EXCL.

- open(a) -> open(b) (errors) -> desync(a) is one way to not actually
  create the file, but is somewhat similar to removing the file after
  creation.

  If you're using desync files you should probably have a good
  understanding of littlefs's sync model anyways.

And of course the user can always sync immediately after open to
guarantee file creation, while opting into the possibility of
zero-length files after powerloss.

Code changes:

           code          stack          ctx
  before: 38084           2624          752
  after:  38128 (+0.1%)   2624 (+0.0%)  752 (+0.0%)
This commit is contained in:
Christopher Haster
2025-01-02 16:06:22 -06:00
parent 00e13375e1
commit 5055a40d8b
2 changed files with 39 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lfs.c
+35 -7
View File
@@ -7329,6 +7329,22 @@ static bool lfsr_omdir_ismidopen(lfs_t *lfs, lfsr_smid_t mid) {
return false;
}
// like lfsr_omdir_ismidopen but ignores zombies/desynced files
static bool lfsr_omdir_ismidalive(lfs_t *lfs, lfsr_smid_t mid) {
for (lfsr_omdir_t *o = lfs->omdirs; o; o = o->next) {
// we really only care about regular open files here, all
// others are either transient (dirs) or fake (orphans)
if (lfsr_o_type(o->flags) == LFS_TYPE_REG
&& o->mdir.mid == mid
&& !lfsr_o_iszombie(o->flags)
&& !lfsr_o_isdesync(o->flags)) {
return true;
}
}
return false;
}
// traversal invalidation things
// needed in lfsr_omdir_clobber
@@ -10744,7 +10760,6 @@ int lfsr_rename(lfs_t *lfs, const char *old_path, const char *new_path) {
if (err && !(err == LFS_ERR_NOENT && lfsr_path_islast(new_path))) {
return err;
}
// already exists?
bool exists = (err != LFS_ERR_NOENT);
// there are a few cases we need to watch out for
@@ -11569,9 +11584,10 @@ int lfsr_file_opencfg(lfs_t *lfs, lfsr_file_t *file,
if (err && !(err == LFS_ERR_NOENT && lfsr_path_islast(path))) {
return err;
}
bool exists = err != LFS_ERR_NOENT;
// creating a new entry?
if (err == LFS_ERR_NOENT || tag == LFSR_TAG_STICKYNOTE) {
if (!exists || tag == LFSR_TAG_STICKYNOTE) {
if (!lfsr_o_iscreat(flags)) {
return LFS_ERR_NOENT;
}
@@ -11582,15 +11598,27 @@ int lfsr_file_opencfg(lfs_t *lfs, lfsr_file_t *file,
return LFS_ERR_NOTDIR;
}
// check that name fits
lfs_size_t name_len = lfsr_path_namelen(path);
if (name_len > lfs->name_limit) {
return LFS_ERR_NAMETOOLONG;
// if we're EXCL and we found a stickynote, check if the file
// is open and not zombied/desynced
//
// we error here even though the file isn't created yet so
// EXCL only lets one create through (ignoring desync+sync
// shenanigans)
if (exists
&& lfsr_o_isexcl(flags)
&& lfsr_omdir_ismidalive(lfs, file->o.o.mdir.mid)) {
return LFS_ERR_EXIST;
}
// create a stickynote entry if we don't have one, this reserves the
// mid until first sync
if (err == LFS_ERR_NOENT) {
if (!exists) {
// check that name fits
lfs_size_t name_len = lfsr_path_namelen(path);
if (name_len > lfs->name_limit) {
return LFS_ERR_NAMETOOLONG;
}
lfs_alloc_ckpoint(lfs);
err = lfsr_mdir_commit(lfs, &file->o.o.mdir, LFSR_RATS(
LFSR_RAT_NAME(
tests/test_forphans.toml
+4 -4
View File
@@ -468,7 +468,7 @@ code = '''
// but we should still recieve sync broadcasts on sync/close
lfsr_file_t file__;
lfsr_file_open(&lfs, &file__, "batman",
LFS_O_RDWR | LFS_O_CREAT | LFS_O_EXCL) => 0;
LFS_O_RDWR | LFS_O_CREAT) => 0;
// mkconsistent should have no effect
if (MKCONSISTENT) {
@@ -609,7 +609,7 @@ code = '''
// open a second reference
lfsr_file_t file__;
lfsr_file_open(&lfs, &file__, "batman",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
LFS_O_WRONLY | LFS_O_CREAT) => 0;
// mkconsistent should have no effect
if (MKCONSISTENT) {
@@ -814,7 +814,7 @@ code = '''
// and a third for checking sync broadcasts
lfsr_file_t file___;
lfsr_file_open(&lfs, &file___, "batman",
LFS_O_RDWR | LFS_O_CREAT | LFS_O_EXCL) => 0;
LFS_O_RDWR | LFS_O_CREAT) => 0;
// mkconsistent should have no effect
if (MKCONSISTENT) {
@@ -1275,7 +1275,7 @@ code = '''
// and a third for checking sync broadcasts
lfsr_file_t file___;
lfsr_file_open(&lfs, &file___, "batman",
LFS_O_RDWR | LFS_O_CREAT | LFS_O_EXCL) => 0;
LFS_O_RDWR | LFS_O_CREAT) => 0;
// mkconsistent should have no effect
if (MKCONSISTENT) {