Github/littlefs
1
Fork 0
Code Actions Activity

Tweaked lfsr_file_open control flow, fixed a few things

Browse Source 
The above-mentioned few things:

- We weren't cleaning up orphans correctly if lfsr_file_open errored.

  I think at some point we relied on having no falible operations after
  the orphan creation, but various refactoring since moved buffer
  allocation after orphan creation.

  We could rearrange things so orphan creation is last, but I think it's
  safter to just deduplicate file cleanup into the new lfsr_file_close_
  function.

- LFS_O_TRUNC prevented attrs from being fetched.

  It's easy to see where this went wrong. LFS_O_TRUNC prevents data from
  being fetched, but we should still fetch attrs.

  This is a bit annoying to fix, for now just added a trunc flag to
  lfsr_file_fetch.

  Also added a couple tests to catch this if it regresses in the future.

- We tried to fetch attrs on orphans.

  This doesn't really hurt anything, but it's a waste of read cycles.

Moving all this stuff around added some code, but lfsr_file_fetch is a
bit easier to read now, which is a good thing:

           code          stack
  before: 38084           2624
  after:  38100 (+0.0%)   2624 (+0.0%)
This commit is contained in:
Christopher Haster
2024-08-23 00:40:39 -05:00
parent 9980323e3f
commit b4da78993b
2 changed files with 198 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lfs.c
+55 -46
View File
@@ -11346,12 +11346,17 @@ static inline lfs_off_t lfsr_file_size_(const lfsr_file_t *file) {
static lfs_ssize_t lfsr_file_read_(lfs_t *lfs, const lfsr_file_t *file,
lfs_off_t pos, uint8_t *buffer, lfs_size_t size);
static int lfsr_file_fetch(lfs_t *lfs, lfsr_file_t *file) {
static int lfsr_file_fetch(lfs_t *lfs, lfsr_file_t *file, bool trunc) {
// default data state
file->o.bshrub_ = LFSR_BSHRUB_BNULL();
file->o.bshrub = LFSR_BSHRUB_BNULL();
// discard the current buffer
file->buffer.pos = 0;
file->buffer.size = 0;
// mark as flushed
file->o.o.flags &= ~LFS_O_UNFLUSH;
// don't bother reading disk if we're an orphan
if (!lfsr_o_isorphan(file->o.o.flags)) {
// don't bother reading disk if we're an orphan or truncating
if (!lfsr_o_isorphan(file->o.o.flags) && !trunc) {
// lookup the file struct, if there is one
lfsr_tag_t tag;
lfsr_data_t data;
@@ -11364,6 +11369,7 @@ static int lfsr_file_fetch(lfs_t *lfs, lfsr_file_t *file) {
// note many of these functions leave bshrub undefined if
// there is an error, so we first read into the staging
// bshrub
file->o.bshrub_ = file->o.bshrub;
// may be a sprout (simple inlined data)
if (err != LFS_ERR_NOENT && tag == LFSR_TAG_DATA) {
@@ -11393,19 +11399,13 @@ static int lfsr_file_fetch(lfs_t *lfs, lfsr_file_t *file) {
return err;
}
}
}
// mark as flushed and synced if we're not an orphan
file->o.o.flags &= ~(
LFS_O_UNFLUSH
| ((!lfsr_o_isorphan(file->o.o.flags))
? LFS_O_UNSYNC
: 0));
// update the bshrub
file->o.bshrub = file->o.bshrub_;
// discard the current buffer
file->buffer.pos = 0;
file->buffer.size = 0;
// update the bshrub
file->o.bshrub = file->o.bshrub_;
// mark as synced
file->o.o.flags &= ~LFS_O_UNSYNC;
}
// if our file is small, try to keep the whole thing in our buffer
//
@@ -11433,6 +11433,14 @@ static int lfsr_file_fetch(lfs_t *lfs, lfsr_file_t *file) {
continue;
}
// don't bother reading disk if we're an orphan
if (lfsr_o_isorphan(file->o.o.flags)) {
if (file->cfg->attrs[i].size) {
*file->cfg->attrs[i].size = LFS_ERR_NOATTR;
}
continue;
}
// lookup the attr
lfsr_data_t data;
int err = lfsr_mdir_lookup(lfs, &file->o.o.mdir,
@@ -11467,6 +11475,7 @@ static int lfsr_file_fetch(lfs_t *lfs, lfsr_file_t *file) {
}
// needed in lfsr_file_opencfg
static void lfsr_file_close_(lfs_t *lfs, const lfsr_file_t *file);
static int lfsr_file_ck(lfs_t *lfs, const lfsr_file_t *file,
uint32_t flags);
@@ -11518,8 +11527,6 @@ int lfsr_file_opencfg(lfs_t *lfs, lfsr_file_t *file,
| (lfs->flags & (LFS_M_FLUSH | LFS_M_SYNC))
// default to unflushed for orphans/truncated files
| LFS_O_UNFLUSH;
// default data state
file->o.bshrub = LFSR_BSHRUB_BNULL();
file->pos = 0;
file->eblock = 0;
file->eoff = -1;
@@ -11599,12 +11606,11 @@ int lfsr_file_opencfg(lfs_t *lfs, lfsr_file_t *file,
file->buffer.pos = 0;
file->buffer.size = 0;
// fetch the file struct if we're not truncating
if (!lfsr_o_istrunc(file->o.o.flags)) {
err = lfsr_file_fetch(lfs, file);
if (err) {
goto failed;
}
// fetch the file struct and custom attrs
err = lfsr_file_fetch(lfs, file,
lfsr_o_istrunc(file->o.o.flags));
if (err) {
goto failed;
}
// check metadata/data for errors?
@@ -11620,11 +11626,8 @@ int lfsr_file_opencfg(lfs_t *lfs, lfsr_file_t *file,
return 0;
failed:;
// clean up memory
if (!file->cfg->buffer) {
lfs_free(file->buffer.buffer);
}
// clean up resources
lfsr_file_close_(lfs, file);
return err;
}
@@ -11636,22 +11639,8 @@ int lfsr_file_open(lfs_t *lfs, lfsr_file_t *file,
return lfsr_file_opencfg(lfs, file, path, flags, &lfsr_file_defaults);
}
// needed in lfsr_file_close
int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file);
int lfsr_file_close(lfs_t *lfs, lfsr_file_t *file) {
LFS_ASSERT(lfsr_omdir_isopen(lfs, &file->o.o));
// don't call lfsr_file_sync if we're readonly or desynced
int err = 0;
if (!lfsr_o_isrdonly(file->o.o.flags)
&& !lfsr_o_isdesync(file->o.o.flags)) {
err = lfsr_file_sync(lfs, file);
}
// remove from tracked mdirs
lfsr_omdir_close(lfs, &file->o.o);
// clean up resources
static void lfsr_file_close_(lfs_t *lfs, const lfsr_file_t *file) {
// clean up memory
if (!file->cfg->buffer) {
lfs_free(file->buffer.buffer);
@@ -11675,6 +11664,26 @@ int lfsr_file_close(lfs_t *lfs, lfsr_file_t *file) {
lfs->flags |= LFS_I_HASORPHANS;
}
}
}
// needed in lfsr_file_close
int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file);
int lfsr_file_close(lfs_t *lfs, lfsr_file_t *file) {
LFS_ASSERT(lfsr_omdir_isopen(lfs, &file->o.o));
// don't call lfsr_file_sync if we're readonly or desynced
int err = 0;
if (!lfsr_o_isrdonly(file->o.o.flags)
&& !lfsr_o_isdesync(file->o.o.flags)) {
err = lfsr_file_sync(lfs, file);
}
// remove from tracked mdirs
lfsr_omdir_close(lfs, &file->o.o);
// clean up resources
lfsr_file_close_(lfs, file);
return err;
}
@@ -13202,7 +13211,7 @@ int lfsr_file_resync(lfs_t *lfs, lfsr_file_t *file) {
// do nothing if already in-sync
if (lfsr_o_isunsync(file->o.o.flags)) {
// refetch the file struct from disk
err = lfsr_file_fetch(lfs, file);
err = lfsr_file_fetch(lfs, file, false);
if (err) {
goto failed;
}
tests/test_attrs.toml
+143
View File
@@ -1611,6 +1611,148 @@ code = '''
lfsr_unmount(&lfs) => 0;
'''
# catch LFS_O_TRUNC mistakes, this has created issues before
[cases.test_attrs_fattr_otrunc]
defines.MODE = ['LFS_A_RDWR']
defines.MUTSIZE = [false, true]
code = '''
lfs_t lfs;
lfsr_format(&lfs, LFS_F_RDWR, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
// create a file
lfsr_file_t file;
lfsr_file_open(&lfs, &file, "cat", LFS_O_WRONLY | LFS_O_CREAT) => 0;
lfsr_file_write(&lfs, &file, "meow", strlen("meow")) => strlen("meow");
lfsr_file_close(&lfs, &file) => 0;
// create some attrs
const char *a = "One 18.25 ounce package chocolate cake mix.";
lfsr_setattr(&lfs, "cat", 'a', a, strlen(a)) => 0;
const char *b = "One can prepared coconut pecan frosting.";
lfsr_setattr(&lfs, "cat", 'b', b, strlen(b)) => 0;
const char *c = "Three slash four cup vegetable oil.";
lfsr_setattr(&lfs, "cat", 'c', c, strlen(c)) => 0;
// try opening a file with these attrs
uint8_t a_buf[256];
lfs_ssize_t a_size = -1;
uint8_t b_buf[256];
lfs_ssize_t b_size = -1;
uint8_t c_buf[256];
lfs_ssize_t c_size = -1;
struct lfs_attr attrs[] = {
{
.type = 'a',
.flags = MODE,
.buffer = a_buf,
.buffer_size = sizeof(a_buf),
.size = (MUTSIZE) ? &a_size : NULL,
},
{
.type = 'b',
.flags = MODE,
.buffer = b_buf,
.buffer_size = sizeof(b_buf),
.size = (MUTSIZE) ? &b_size : NULL,
},
{
.type = 'c',
.flags = MODE,
.buffer = c_buf,
.buffer_size = sizeof(c_buf),
.size = (MUTSIZE) ? &c_size : NULL,
}
};
struct lfs_file_config filecfg = {
.attrs = attrs,
.attr_count = 3,
};
lfsr_file_opencfg(&lfs, &file, "cat",
MODE | LFS_O_TRUNC, &filecfg) => 0;
// did we read the attrs correctly?
if (MUTSIZE) {
assert(a_size == strlen(a));
}
assert(memcmp(a_buf, a, strlen(a)) == 0);
if (MUTSIZE) {
assert(b_size == strlen(b));
}
assert(memcmp(b_buf, b, strlen(b)) == 0);
if (MUTSIZE) {
assert(c_size == strlen(c));
}
assert(memcmp(c_buf, c, strlen(c)) == 0);
lfsr_file_close(&lfs, &file) => 0;
lfsr_unmount(&lfs) => 0;
'''
# orphan files should zero attributes
[cases.test_attrs_fattr_orphan]
defines.MODE = ['LFS_A_RDWR']
defines.MUTSIZE = [false, true]
code = '''
lfs_t lfs;
lfsr_format(&lfs, LFS_F_RDWR, CFG) => 0;
lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0;
// _don't_ create a file
// try opening a file with these attrs
uint8_t a_buf[256];
lfs_ssize_t a_size = -1;
uint8_t b_buf[256];
lfs_ssize_t b_size = -1;
uint8_t c_buf[256];
lfs_ssize_t c_size = -1;
struct lfs_attr attrs[] = {
{
.type = 'a',
.flags = MODE,
.buffer = a_buf,
.buffer_size = sizeof(a_buf),
.size = (MUTSIZE) ? &a_size : NULL,
},
{
.type = 'b',
.flags = MODE,
.buffer = b_buf,
.buffer_size = sizeof(b_buf),
.size = (MUTSIZE) ? &b_size : NULL,
},
{
.type = 'c',
.flags = MODE,
.buffer = c_buf,
.buffer_size = sizeof(c_buf),
.size = (MUTSIZE) ? &c_size : NULL,
}
};
struct lfs_file_config filecfg = {
.attrs = attrs,
.attr_count = 3,
};
lfsr_file_t file;
lfsr_file_opencfg(&lfs, &file, "cat",
MODE | LFS_O_CREAT, &filecfg) => 0;
// did we read the attrs correctly?
if (MUTSIZE) {
assert(a_size == LFS_ERR_NOATTR);
}
if (MUTSIZE) {
assert(b_size == LFS_ERR_NOATTR);
}
if (MUTSIZE) {
assert(c_size == LFS_ERR_NOATTR);
}
lfsr_file_close(&lfs, &file) => 0;
lfsr_unmount(&lfs) => 0;
'''
# test that file-attached attrs are written correctly
[cases.test_attrs_fattr_set]
defines.MODE = ['LFS_A_WRONLY', 'LFS_A_RDWR']
@@ -2378,6 +2520,7 @@ code = '''
#[cases.test_attrs_fattr_zombie_resync] TODO do we test file zombie resync?
#[cases.test_attrs_fattr_pl] ?
#[cases.test_attrs_fattr_orphan] ?