From 942427dc8c42981e6f2f359926dd8b07b9675ccb Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Thu, 18 Jan 2024 15:16:00 -0600 Subject: [PATCH] Reworked lfsr_mtree_pathlookup a bit to better leverage internal errors This avoids implicit info, mid.mid=-1 implying a bad path, and mid.mid=0 implying the root directory, at a tradeoff of potentially making the returned error codes a bit confusing (0 means the file is NOT found!). Here are the now possible return codes, aside from lower-level errors (IO, CORRUPT, etc): - 0 => path is valid, file NOT found - EXIST => path is valid, file found - INVAL => path is valid, but points to root - NOENT => path is NOT valid, intermediate dir missing - NOTDIR => path is NOT valid, intermediate dir is not a dir Since the root has no real mdir entry, I think the special INVAL return code is warranted. It needs special behavior in relevant functions anyways. Note that orphaned files still need special handling. Code changes: code stack before: 33944 2944 after: 34036 (+0.3%) 2944 (+0.0%) --- lfs.c | 197 ++++++++++++++++++++---------------------- tests/test_dirs.toml | 4 +- tests/test_files.toml | 14 +-- 3 files changed, 102 insertions(+), 113 deletions(-) diff --git a/lfs.c b/lfs.c index 7de06af7..54bbe83e 100644 --- a/lfs.c +++ b/lfs.c @@ -4939,11 +4939,6 @@ static inline lfsr_srid_t lfsr_mid_rid(lfs_t *lfs, lfsr_smid_t mid) { | (mid & ((1 << lfs->mbits) - 1)); } -// we use the root's bookmark at 0.0 to represent root -static inline bool lfsr_mid_isroot(lfsr_smid_t mid) { - return mid == 0; -} - // metadata-pointer things @@ -5022,10 +5017,6 @@ static inline lfsr_srid_t lfsr_mdir_rid(lfs_t *lfs, const lfsr_mdir_t *mdir) { return lfsr_mid_rid(lfs, mdir->mid); } -static inline bool lfsr_mdir_isroot(const lfsr_mdir_t *mdir) { - return lfsr_mid_isroot(mdir->mid); -} - // mdir operations static int lfsr_mdir_fetch(lfs_t *lfs, lfsr_mdir_t *mdir, lfsr_smid_t mid, const lfsr_mptr_t *mptr) { @@ -6920,44 +6911,45 @@ enum { LFSR_DID_ROOT = 0, }; -// TODO this function may need another look over -// // lookup full paths in our mtree // +// note the errors here are a bit weird, because paths can have some weird +// corner-cases during lookup, and we want to report all the different +// conditions: +// +// - 0 => path is valid, file NOT found +// - EXIST => path is valid, file found +// - INVAL => path is valid, but points to root +// - NOENT => path is NOT valid, intermediate dir missing +// - NOTDIR => path is NOT valid, intermediate dir is not a dir +// // if not found, mdir_/did_/name_ will at least be set up // with what should be the parent static int lfsr_mtree_pathlookup(lfs_t *lfs, const char *path, - // TODO originally path itself was a double pointer, is that a - // better design? lfsr_mdir_t *mdir_, lfsr_tag_t *tag_, lfsr_did_t *did_, const char **name_, lfs_size_t *name_size_) { // setup root - lfsr_mdir_t mdir = {.mid = 0}; + lfsr_mdir_t mdir = {.mid = -1}; lfsr_tag_t tag = LFSR_TAG_DIR; lfsr_did_t did = LFSR_DID_ROOT; - - // use mid=-1 to indicate we can't even create the path - if (mdir_) { - mdir_->mid = -1; - } // we reduce path to a single name if we can find it const char *name = path; - + lfs_size_t name_size = 0; while (true) { // skip slashes - name += strspn(name, "/"); - lfs_size_t name_size = strcspn(name, "/"); + path += strspn(path, "/"); + lfs_size_t name_size__ = strcspn(path, "/"); // skip '.' and root '..' - if ((name_size == 1 && memcmp(name, ".", 1) == 0) - || (name_size == 2 && memcmp(name, "..", 2) == 0)) { - name += name_size; + if ((name_size__ == 1 && memcmp(path, ".", 1) == 0) + || (name_size__ == 2 && memcmp(path, "..", 2) == 0)) { + path += name_size__; goto next; } // skip if matched by '..' in name - const char *suffix = name + name_size; + const char *suffix = path + name_size__; lfs_size_t suffix_size; int depth = 1; while (true) { @@ -6970,7 +6962,7 @@ static int lfsr_mtree_pathlookup(lfs_t *lfs, const char *path, if (suffix_size == 2 && memcmp(suffix, "..", 2) == 0) { depth -= 1; if (depth == 0) { - name = suffix + suffix_size; + path = suffix + suffix_size; goto next; } } else { @@ -6981,16 +6973,35 @@ static int lfsr_mtree_pathlookup(lfs_t *lfs, const char *path, } // found end of path, we must be done parsing our path now - if (name[0] == '\0') { + if (path[0] == '\0') { + // the root dir doesn't have an mdir really, so it's always + // a special case + if (mdir.mid == -1) { + return LFS_ERR_INVAL; + } + if (mdir_) { *mdir_ = mdir; } if (tag_) { *tag_ = tag; } - return 0; + if (did_) { + *did_ = did; + } + if (name_) { + *name_ = name; + } + if (name_size_) { + *name_size_ = name_size; + } + return LFS_ERR_EXIST; } + // found another name + name = path; + name_size = name_size__; + // only continue if we hit a directory if (tag != LFSR_TAG_DIR) { return (tag == LFSR_TAG_ORPHAN) @@ -6999,7 +7010,7 @@ static int lfsr_mtree_pathlookup(lfs_t *lfs, const char *path, } // read the next did from the mdir if this is not the root - if (!lfsr_mid_isroot(mdir.mid)) { + if (mdir.mid != -1) { lfsr_data_t data; int err = lfsr_mdir_lookup(lfs, &mdir, mdir.mid, LFSR_TAG_DID, &data); @@ -7016,37 +7027,32 @@ static int lfsr_mtree_pathlookup(lfs_t *lfs, const char *path, // lookup up this name in the mtree int err = lfsr_mtree_namelookup(lfs, did, name, name_size, &mdir, &tag, NULL); - if (err && err != LFS_ERR_NOENT) { + if (err) { + // report where to insert if we are the last name in our path + if (err == LFS_ERR_NOENT && strchr(name, '/') == NULL) { + if (mdir_) { + *mdir_ = mdir; + } + if (tag_) { + *tag_ = tag; + } + if (did_) { + *did_ = did; + } + if (name_) { + *name_ = name; + } + if (name_size_) { + *name_size_ = name_size; + } + return 0; + } return err; } - // keep track of where to insert if we are the last name in our path - if (strchr(name, '/') == NULL) { - if (mdir_) { - *mdir_ = mdir; - } - if (tag_) { - *tag_ = tag; - } - if (did_) { - *did_ = did; - } - if (name_) { - *name_ = name; - } - if (name_size_) { - *name_size_ = name_size; - } - } - - // error if not found - if (err == LFS_ERR_NOENT) { - return LFS_ERR_NOENT; - } - // go on to next name - name += name_size; -next:; + path += name_size; + next:; } } @@ -8403,12 +8409,11 @@ int lfsr_mkdir(lfs_t *lfs, const char *path) { err = lfsr_mtree_pathlookup(lfs, path, &mdir, &tag, &did, &name, &name_size); - if (err && (err != LFS_ERR_NOENT || mdir.mid == -1)) { + if (err && err != LFS_ERR_EXIST) { return err; } - // already exists? note orphans don't really exist - bool exists = (err != LFS_ERR_NOENT); + bool exists = (err == LFS_ERR_EXIST); if (exists && tag != LFSR_TAG_ORPHAN) { return LFS_ERR_EXIST; } @@ -8539,21 +8544,14 @@ int lfsr_remove(lfs_t *lfs, const char *path) { err = lfsr_mtree_pathlookup(lfs, path, &mdir, &tag, &did, &name, &name_size); - if (err) { + if (err && err != LFS_ERR_EXIST) { return err; } - - // found a zombie? - if (tag == LFSR_TAG_ORPHAN) { - // don't worry, zombies aren't real and cannot hurt you + // doesn't exist? note orphans don't really exist + if (!err || tag == LFSR_TAG_ORPHAN) { return LFS_ERR_NOENT; } - // as funny as it would be, you can't remove the root - if (lfsr_mdir_isroot(&mdir)) { - return LFS_ERR_INVAL; - } - // if we're removing a directory, we need to also remove the // bookmark entry lfsr_grm_t grm = lfs->grm; @@ -8661,21 +8659,14 @@ int lfsr_rename(lfs_t *lfs, const char *old_path, const char *new_path) { err = lfsr_mtree_pathlookup(lfs, old_path, &old_mdir, &old_tag, NULL, NULL, NULL); - if (err) { + if (err && err != LFS_ERR_EXIST) { return err; } - - // found a zombie? - if (old_tag == LFSR_TAG_ORPHAN) { - // don't worry, zombies aren't real and cannot hurt you + // doesn't exist? note orphans don't really exist + if (!err || old_tag == LFSR_TAG_ORPHAN) { return LFS_ERR_NOENT; } - // as funny as it would be, you can't rename the root - if (lfsr_mdir_isroot(&old_mdir)) { - return LFS_ERR_INVAL; - } - // mark old entry for removal with a grm lfsr_grm_t grm = lfs->grm; lfsr_grm_pushrm(&grm, old_mdir.mid); @@ -8689,11 +8680,11 @@ int lfsr_rename(lfs_t *lfs, const char *old_path, const char *new_path) { err = lfsr_mtree_pathlookup(lfs, new_path, &new_mdir, &new_tag, &new_did, &new_name, &new_name_size); - if (err && (err != LFS_ERR_NOENT || new_mdir.mid == -1)) { + if (err && err != LFS_ERR_EXIST) { return err; } // already exists? - bool exists = (err != LFS_ERR_NOENT); + bool exists = (err == LFS_ERR_EXIST); // there are a few cases we need to watch out for if (!exists) { @@ -8866,17 +8857,16 @@ int lfsr_stat(lfs_t *lfs, const char *path, struct lfs_info *info) { int err = lfsr_mtree_pathlookup(lfs, path, &mdir, &tag, NULL, &name, &name_size); - if (err) { + if (err && err != LFS_ERR_EXIST && err != LFS_ERR_INVAL) { return err; } - - // pretend orphans don't exist - if (tag == LFSR_TAG_ORPHAN) { + // doesn't exist? note orphans don't really exist + if (!err || tag == LFSR_TAG_ORPHAN) { return LFS_ERR_NOENT; } // special case for root - if (lfsr_mdir_isroot(&mdir)) { + if (err == LFS_ERR_INVAL) { strcpy(info->name, "/"); info->type = LFS_TYPE_DIR; return 0; @@ -8895,26 +8885,24 @@ int lfsr_dir_open(lfs_t *lfs, lfsr_dir_t *dir, const char *path) { int err = lfsr_mtree_pathlookup(lfs, path, &mdir, &tag, NULL, NULL, NULL); - if (err) { + if (err && err != LFS_ERR_EXIST && err != LFS_ERR_INVAL) { return err; } - - // are we a directory? - if (tag != LFSR_TAG_DIR) { - if (tag == LFSR_TAG_ORPHAN) { - return LFS_ERR_NOENT; - } else { - return LFS_ERR_NOTDIR; - } + // doesn't exist? note orphans don't really exist + if (!err || tag == LFSR_TAG_ORPHAN) { + return LFS_ERR_NOENT; } - // setup dir state - dir->type = LFS_TYPE_DIR; - // read our did from the mdir, unless we're root - if (lfsr_mdir_isroot(&mdir)) { + if (err == LFS_ERR_INVAL) { dir->did = 0; + } else { + // not a directory? + if (tag != LFSR_TAG_DIR) { + return LFS_ERR_NOTDIR; + } + lfsr_data_t data; err = lfsr_mdir_lookup(lfs, &mdir, mdir.mid, LFSR_TAG_DID, &data); @@ -8936,6 +8924,7 @@ int lfsr_dir_open(lfs_t *lfs, lfsr_dir_t *dir, const char *path) { } // add to tracked mdirs + dir->type = LFS_TYPE_DIR; lfsr_addopened(lfs, (lfsr_opened_t*)dir); return 0; } @@ -9224,7 +9213,6 @@ int lfsr_file_opencfg(lfs_t *lfs, lfsr_file_t *file, } // setup file state - file->type = LFS_TYPE_REG; file->flags = flags; file->cfg = cfg; file->pos = 0; @@ -9239,12 +9227,12 @@ int lfsr_file_opencfg(lfs_t *lfs, lfsr_file_t *file, int err = lfsr_mtree_pathlookup(lfs, path, &file->mdir, &tag, &did, &name, &name_size); - if (err && (err != LFS_ERR_NOENT || file->mdir.mid == -1)) { + if (err && err != LFS_ERR_EXIST) { return err; } // creating a new entry? - if (err == LFS_ERR_NOENT || tag == LFSR_TAG_ORPHAN) { + if (!err || tag == LFSR_TAG_ORPHAN) { if (!lfsr_o_iscreat(flags)) { return LFS_ERR_NOENT; } @@ -9257,7 +9245,7 @@ int lfsr_file_opencfg(lfs_t *lfs, lfsr_file_t *file, // create an orphan entry if we don't have one, this reserves the // mid until first sync - if (err == LFS_ERR_NOENT) { + if (!err) { err = lfsr_mdir_commit(lfs, &file->mdir, LFSR_ATTRS( LFSR_ATTR(file->mdir.mid, ORPHAN, +1, CAT( @@ -9361,6 +9349,7 @@ int lfsr_file_opencfg(lfs_t *lfs, lfsr_file_t *file, } // add to tracked mdirs + file->type = LFS_TYPE_REG; lfsr_addopened(lfs, (lfsr_opened_t*)file); return 0; diff --git a/tests/test_dirs.toml b/tests/test_dirs.toml index 788b0b8b..0998a503 100644 --- a/tests/test_dirs.toml +++ b/tests/test_dirs.toml @@ -281,14 +281,14 @@ code = ''' } // try to make root, which doesn't make sense - lfsr_mkdir(&lfs, "/") => LFS_ERR_EXIST; + lfsr_mkdir(&lfs, "/") => LFS_ERR_INVAL; // make a directory err = lfsr_mkdir(&lfs, "ardvark"); assert(!err || (TEST_PLS && err == LFS_ERR_EXIST)); // try to make root, which doesn't make sense - lfsr_mkdir(&lfs, "/") => LFS_ERR_EXIST; + lfsr_mkdir(&lfs, "/") => LFS_ERR_INVAL; // remount? if (REMOUNT) { diff --git a/tests/test_files.toml b/tests/test_files.toml index 010c2d2f..bb821510 100644 --- a/tests/test_files.toml +++ b/tests/test_files.toml @@ -426,16 +426,16 @@ code = ''' // try reading our root as a file lfsr_file_t file; - lfsr_file_open(&lfs, &file, "/", LFS_O_RDONLY) => LFS_ERR_ISDIR; + lfsr_file_open(&lfs, &file, "/", LFS_O_RDONLY) => LFS_ERR_INVAL; // try writing our root as a file - lfsr_file_open(&lfs, &file, "/", LFS_O_WRONLY) => LFS_ERR_ISDIR; + lfsr_file_open(&lfs, &file, "/", LFS_O_WRONLY) => LFS_ERR_INVAL; lfsr_file_open(&lfs, &file, "/", - LFS_O_WRONLY | LFS_O_TRUNC) => LFS_ERR_ISDIR; + LFS_O_WRONLY | LFS_O_TRUNC) => LFS_ERR_INVAL; lfsr_file_open(&lfs, &file, "/", - LFS_O_WRONLY | LFS_O_CREAT) => LFS_ERR_ISDIR; + LFS_O_WRONLY | LFS_O_CREAT) => LFS_ERR_INVAL; lfsr_file_open(&lfs, &file, "/", - LFS_O_WRONLY | LFS_O_CREAT | LFS_O_TRUNC) => LFS_ERR_ISDIR; + LFS_O_WRONLY | LFS_O_CREAT | LFS_O_TRUNC) => LFS_ERR_INVAL; // try rename a file on top of our directory lfsr_file_open(&lfs, &file, "not_hello", @@ -446,7 +446,7 @@ code = ''' lfsr_file_write(&lfs, &file, wbuf, wsize) => wsize; lfsr_file_close(&lfs, &file) => 0; - lfsr_rename(&lfs, "not_hello", "/") => LFS_ERR_ISDIR; + lfsr_rename(&lfs, "not_hello", "/") => LFS_ERR_INVAL; // remount? if (REMOUNT) { @@ -1734,7 +1734,7 @@ code = ''' } // rename the file - lfsr_rename(&lfs, "amethyst", "/") => LFS_ERR_ISDIR; + lfsr_rename(&lfs, "amethyst", "/") => LFS_ERR_INVAL; // remount? if (REMOUNT) {