From 19a3736fe5dab5d01220660b318fed824a2d6e22 Mon Sep 17 00:00:00 2001 From: porta Date: Sun, 15 Sep 2024 18:01:42 +0300 Subject: [PATCH 1/2] [FL-3891] Folder rename fails (#3896) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix, refactor: storage is_subdir API * docs: fix incorrect comment * test: new storage apis * test: use temporary path * style: fix formatting * UnitTest: storage path macros naming * UnitTest: storage path macros naming part 2 Co-authored-by: あく --- .../unit_tests/tests/storage/storage_test.c | 87 +++++++++++++++---- applications/services/storage/storage.h | 25 +++--- .../services/storage/storage_external_api.c | 18 ++-- .../services/storage/storage_message.h | 2 +- .../services/storage/storage_processing.c | 18 +++- targets/f18/api_symbols.csv | 5 +- targets/f7/api_symbols.csv | 5 +- 7 files changed, 120 insertions(+), 40 deletions(-) diff --git a/applications/debug/unit_tests/tests/storage/storage_test.c b/applications/debug/unit_tests/tests/storage/storage_test.c index f317fbf68..75c52ef9a 100644 --- a/applications/debug/unit_tests/tests/storage/storage_test.c +++ b/applications/debug/unit_tests/tests/storage/storage_test.c @@ -6,9 +6,10 @@ // This is a hack to access internal storage functions and definitions #include -#define UNIT_TESTS_PATH(path) EXT_PATH("unit_tests/" path) +#define UNIT_TESTS_RESOURCES_PATH(path) EXT_PATH("unit_tests/" path) +#define UNIT_TESTS_PATH(path) EXT_PATH(".tmp/unit_tests/" path) -#define STORAGE_LOCKED_FILE EXT_PATH("locked_file.test") +#define STORAGE_LOCKED_FILE UNIT_TESTS_PATH("locked_file.test") #define STORAGE_LOCKED_DIR STORAGE_INT_PATH_PREFIX #define STORAGE_TEST_DIR UNIT_TESTS_PATH("test_dir") @@ -369,33 +370,78 @@ MU_TEST(storage_file_rename) { Storage* storage = furi_record_open(RECORD_STORAGE); File* file = storage_file_alloc(storage); - mu_check(write_file_13DA(storage, EXT_PATH("file.old"))); - mu_check(check_file_13DA(storage, EXT_PATH("file.old"))); + mu_check(write_file_13DA(storage, UNIT_TESTS_PATH("file.old"))); + mu_check(check_file_13DA(storage, UNIT_TESTS_PATH("file.old"))); mu_assert_int_eq( - FSE_OK, storage_common_rename(storage, EXT_PATH("file.old"), EXT_PATH("file.new"))); - mu_assert_int_eq(FSE_NOT_EXIST, storage_common_stat(storage, EXT_PATH("file.old"), NULL)); - mu_assert_int_eq(FSE_OK, storage_common_stat(storage, EXT_PATH("file.new"), NULL)); - mu_check(check_file_13DA(storage, EXT_PATH("file.new"))); - mu_assert_int_eq(FSE_OK, storage_common_remove(storage, EXT_PATH("file.new"))); + FSE_OK, + storage_common_rename(storage, UNIT_TESTS_PATH("file.old"), UNIT_TESTS_PATH("file.new"))); + mu_assert_int_eq( + FSE_NOT_EXIST, storage_common_stat(storage, UNIT_TESTS_PATH("file.old"), NULL)); + mu_assert_int_eq(FSE_OK, storage_common_stat(storage, UNIT_TESTS_PATH("file.new"), NULL)); + mu_check(check_file_13DA(storage, UNIT_TESTS_PATH("file.new"))); + mu_assert_int_eq(FSE_OK, storage_common_remove(storage, UNIT_TESTS_PATH("file.new"))); storage_file_free(file); furi_record_close(RECORD_STORAGE); } +static const char* dir_rename_tests[][2] = { + {UNIT_TESTS_PATH("dir.old"), UNIT_TESTS_PATH("dir.new")}, + {UNIT_TESTS_PATH("test_dir"), UNIT_TESTS_PATH("test_dir-new")}, + {UNIT_TESTS_PATH("test"), UNIT_TESTS_PATH("test-test")}, +}; + MU_TEST(storage_dir_rename) { Storage* storage = furi_record_open(RECORD_STORAGE); - storage_dir_create(storage, EXT_PATH("dir.old")); + for(size_t i = 0; i < COUNT_OF(dir_rename_tests); i++) { + const char* old_path = dir_rename_tests[i][0]; + const char* new_path = dir_rename_tests[i][1]; - mu_check(storage_dir_rename_check(storage, EXT_PATH("dir.old"))); + storage_dir_create(storage, old_path); + mu_check(storage_dir_rename_check(storage, old_path)); + + mu_assert_int_eq(FSE_OK, storage_common_rename(storage, old_path, new_path)); + mu_assert_int_eq(FSE_NOT_EXIST, storage_common_stat(storage, old_path, NULL)); + mu_check(storage_dir_rename_check(storage, new_path)); + + storage_dir_remove(storage, new_path); + mu_assert_int_eq(FSE_NOT_EXIST, storage_common_stat(storage, new_path, NULL)); + } + + furi_record_close(RECORD_STORAGE); +} + +MU_TEST(storage_equiv_and_subdir) { + Storage* storage = furi_record_open(RECORD_STORAGE); mu_assert_int_eq( - FSE_OK, storage_common_rename(storage, EXT_PATH("dir.old"), EXT_PATH("dir.new"))); - mu_assert_int_eq(FSE_NOT_EXIST, storage_common_stat(storage, EXT_PATH("dir.old"), NULL)); - mu_check(storage_dir_rename_check(storage, EXT_PATH("dir.new"))); + true, + storage_common_equivalent_path(storage, UNIT_TESTS_PATH("blah"), UNIT_TESTS_PATH("blah"))); + mu_assert_int_eq( + true, + storage_common_equivalent_path( + storage, UNIT_TESTS_PATH("blah/"), UNIT_TESTS_PATH("blah/"))); + mu_assert_int_eq( + false, + storage_common_equivalent_path( + storage, UNIT_TESTS_PATH("blah"), UNIT_TESTS_PATH("blah-blah"))); + mu_assert_int_eq( + false, + storage_common_equivalent_path( + storage, UNIT_TESTS_PATH("blah/"), UNIT_TESTS_PATH("blah-blah/"))); - storage_dir_remove(storage, EXT_PATH("dir.new")); - mu_assert_int_eq(FSE_NOT_EXIST, storage_common_stat(storage, EXT_PATH("dir.new"), NULL)); + mu_assert_int_eq( + true, storage_common_is_subdir(storage, UNIT_TESTS_PATH("blah"), UNIT_TESTS_PATH("blah"))); + mu_assert_int_eq( + true, + storage_common_is_subdir(storage, UNIT_TESTS_PATH("blah"), UNIT_TESTS_PATH("blah/blah"))); + mu_assert_int_eq( + false, + storage_common_is_subdir(storage, UNIT_TESTS_PATH("blah/blah"), UNIT_TESTS_PATH("blah"))); + mu_assert_int_eq( + false, + storage_common_is_subdir(storage, UNIT_TESTS_PATH("blah"), UNIT_TESTS_PATH("blah-blah"))); furi_record_close(RECORD_STORAGE); } @@ -403,10 +449,13 @@ MU_TEST(storage_dir_rename) { MU_TEST_SUITE(storage_rename) { MU_RUN_TEST(storage_file_rename); MU_RUN_TEST(storage_dir_rename); + MU_RUN_TEST(storage_equiv_and_subdir); Storage* storage = furi_record_open(RECORD_STORAGE); - storage_dir_remove(storage, EXT_PATH("dir.old")); - storage_dir_remove(storage, EXT_PATH("dir.new")); + for(size_t i = 0; i < COUNT_OF(dir_rename_tests); i++) { + storage_dir_remove(storage, dir_rename_tests[i][0]); + storage_dir_remove(storage, dir_rename_tests[i][1]); + } furi_record_close(RECORD_STORAGE); } @@ -653,7 +702,7 @@ MU_TEST(test_md5_calc) { Storage* storage = furi_record_open(RECORD_STORAGE); File* file = storage_file_alloc(storage); - const char* path = UNIT_TESTS_PATH("storage/md5.txt"); + const char* path = UNIT_TESTS_RESOURCES_PATH("storage/md5.txt"); const char* md5_cstr = "2a456fa43e75088fdde41c93159d62a2"; const uint8_t md5[MD5_HASH_SIZE] = { 0x2a, diff --git a/applications/services/storage/storage.h b/applications/services/storage/storage.h index 072db1305..ea0ff24ad 100644 --- a/applications/services/storage/storage.h +++ b/applications/services/storage/storage.h @@ -401,21 +401,26 @@ bool storage_common_exists(Storage* storage, const char* path); * - /int/Test and /int/test -> false (Case-sensitive storage), * - /ext/Test and /ext/test -> true (Case-insensitive storage). * - * If the truncate parameter is set to true, the second path will be - * truncated to be no longer than the first one. It is useful to determine - * whether path2 is a subdirectory of path1. - * * @param storage pointer to a storage API instance. * @param path1 pointer to a zero-terminated string containing the first path. * @param path2 pointer to a zero-terminated string containing the second path. - * @param truncate whether to truncate path2 to be no longer than path1. * @return true if paths are equivalent, false otherwise. */ -bool storage_common_equivalent_path( - Storage* storage, - const char* path1, - const char* path2, - bool truncate); +bool storage_common_equivalent_path(Storage* storage, const char* path1, const char* path2); + +/** + * @brief Check whether a path is a subpath of another path. + * + * This function respects storage-defined equivalence rules + * (see `storage_common_equivalent_path`). + * + * @param storage pointer to a storage API instance. + * @param parent pointer to a zero-terminated string containing the parent path. + * @param child pointer to a zero-terminated string containing the child path. + * @return true if `child` is a subpath of `parent`, or if `child` is equivalent + * to `parent`; false otherwise. + */ +bool storage_common_is_subdir(Storage* storage, const char* parent, const char* child); /******************* Error Functions *******************/ diff --git a/applications/services/storage/storage_external_api.c b/applications/services/storage/storage_external_api.c index 7803e8f6a..ecc5330af 100644 --- a/applications/services/storage/storage_external_api.c +++ b/applications/services/storage/storage_external_api.c @@ -493,13 +493,13 @@ FS_Error storage_common_rename(Storage* storage, const char* old_path, const cha } // Cannot rename a directory to itself or to a nested directory - if(storage_common_equivalent_path(storage, old_path, new_path, true)) { + if(storage_common_is_subdir(storage, old_path, new_path)) { error = FSE_INVALID_NAME; break; } // Renaming a regular file to itself does nothing and always succeeds - } else if(storage_common_equivalent_path(storage, old_path, new_path, false)) { + } else if(storage_common_equivalent_path(storage, old_path, new_path)) { error = FSE_OK; break; } @@ -816,11 +816,11 @@ bool storage_common_exists(Storage* storage, const char* path) { return storage_common_stat(storage, path, &file_info) == FSE_OK; } -bool storage_common_equivalent_path( +static bool storage_internal_equivalent_path( Storage* storage, const char* path1, const char* path2, - bool truncate) { + bool check_subdir) { furi_check(storage); S_API_PROLOGUE; @@ -829,7 +829,7 @@ bool storage_common_equivalent_path( .cequivpath = { .path1 = path1, .path2 = path2, - .truncate = truncate, + .check_subdir = check_subdir, .thread_id = furi_thread_get_current_id(), }}; @@ -839,6 +839,14 @@ bool storage_common_equivalent_path( return S_RETURN_BOOL; } +bool storage_common_equivalent_path(Storage* storage, const char* path1, const char* path2) { + return storage_internal_equivalent_path(storage, path1, path2, false); +} + +bool storage_common_is_subdir(Storage* storage, const char* parent, const char* child) { + return storage_internal_equivalent_path(storage, parent, child, true); +} + /****************** ERROR ******************/ const char* storage_error_get_desc(FS_Error error_id) { diff --git a/applications/services/storage/storage_message.h b/applications/services/storage/storage_message.h index cd45906d4..ba34aa7c0 100644 --- a/applications/services/storage/storage_message.h +++ b/applications/services/storage/storage_message.h @@ -72,7 +72,7 @@ typedef struct { typedef struct { const char* path1; const char* path2; - bool truncate; + bool check_subdir; FuriThreadId thread_id; } SADataCEquivPath; diff --git a/applications/services/storage/storage_processing.c b/applications/services/storage/storage_processing.c index 8d86dd385..7a328051d 100644 --- a/applications/services/storage/storage_processing.c +++ b/applications/services/storage/storage_processing.c @@ -694,7 +694,23 @@ void storage_process_message_internal(Storage* app, StorageMessage* message) { storage_path_trim_trailing_slashes(path2); storage_process_alias(app, path1, message->data->cequivpath.thread_id, false); storage_process_alias(app, path2, message->data->cequivpath.thread_id, false); - if(message->data->cequivpath.truncate) { + if(message->data->cequivpath.check_subdir) { + // by appending slashes at the end and then truncating the second path, we can + // effectively check for shared path components: + // example 1: + // path1: "/ext/blah" -> "/ext/blah/" -> "/ext/blah/" + // path2: "/ext/blah-blah" -> "/ect/blah-blah/" -> "/ext/blah-" + // results unequal, conclusion: path2 is not a subpath of path1 + // example 2: + // path1: "/ext/blah" -> "/ext/blah/" -> "/ext/blah/" + // path2: "/ext/blah/blah" -> "/ect/blah/blah/" -> "/ext/blah/" + // results equal, conclusion: path2 is a subpath of path1 + // example 3: + // path1: "/ext/blah/blah" -> "/ect/blah/blah/" -> "/ext/blah/blah/" + // path2: "/ext/blah" -> "/ext/blah/" -> "/ext/blah/" + // results unequal, conclusion: path2 is not a subpath of path1 + furi_string_push_back(path1, '/'); + furi_string_push_back(path2, '/'); furi_string_left(path2, furi_string_size(path1)); } message->return_data->bool_value = diff --git a/targets/f18/api_symbols.csv b/targets/f18/api_symbols.csv index 553cf1472..1d9ac2869 100644 --- a/targets/f18/api_symbols.csv +++ b/targets/f18/api_symbols.csv @@ -1,5 +1,5 @@ entry,status,name,type,params -Version,+,73.0,, +Version,+,74.0,, Header,+,applications/services/bt/bt_service/bt.h,, Header,+,applications/services/bt/bt_service/bt_keys_storage.h,, Header,+,applications/services/cli/cli.h,, @@ -2491,9 +2491,10 @@ Function,+,st25r3916_write_pttsn_mem,void,"FuriHalSpiBusHandle*, uint8_t*, size_ Function,+,st25r3916_write_reg,void,"FuriHalSpiBusHandle*, uint8_t, uint8_t" Function,+,st25r3916_write_test_reg,void,"FuriHalSpiBusHandle*, uint8_t, uint8_t" Function,+,storage_common_copy,FS_Error,"Storage*, const char*, const char*" -Function,+,storage_common_equivalent_path,_Bool,"Storage*, const char*, const char*, _Bool" +Function,+,storage_common_equivalent_path,_Bool,"Storage*, const char*, const char*" Function,+,storage_common_exists,_Bool,"Storage*, const char*" Function,+,storage_common_fs_info,FS_Error,"Storage*, const char*, uint64_t*, uint64_t*" +Function,+,storage_common_is_subdir,_Bool,"Storage*, const char*, const char*" Function,+,storage_common_merge,FS_Error,"Storage*, const char*, const char*" Function,+,storage_common_migrate,FS_Error,"Storage*, const char*, const char*" Function,+,storage_common_mkdir,FS_Error,"Storage*, const char*" diff --git a/targets/f7/api_symbols.csv b/targets/f7/api_symbols.csv index 5d5c2a707..cb3037e48 100644 --- a/targets/f7/api_symbols.csv +++ b/targets/f7/api_symbols.csv @@ -1,5 +1,5 @@ entry,status,name,type,params -Version,+,73.0,, +Version,+,74.0,, Header,+,applications/drivers/subghz/cc1101_ext/cc1101_ext_interconnect.h,, Header,+,applications/services/bt/bt_service/bt.h,, Header,+,applications/services/bt/bt_service/bt_keys_storage.h,, @@ -3168,9 +3168,10 @@ Function,+,st25tb_save,_Bool,"const St25tbData*, FlipperFormat*" Function,+,st25tb_set_uid,_Bool,"St25tbData*, const uint8_t*, size_t" Function,+,st25tb_verify,_Bool,"St25tbData*, const FuriString*" Function,+,storage_common_copy,FS_Error,"Storage*, const char*, const char*" -Function,+,storage_common_equivalent_path,_Bool,"Storage*, const char*, const char*, _Bool" +Function,+,storage_common_equivalent_path,_Bool,"Storage*, const char*, const char*" Function,+,storage_common_exists,_Bool,"Storage*, const char*" Function,+,storage_common_fs_info,FS_Error,"Storage*, const char*, uint64_t*, uint64_t*" +Function,+,storage_common_is_subdir,_Bool,"Storage*, const char*, const char*" Function,+,storage_common_merge,FS_Error,"Storage*, const char*, const char*" Function,+,storage_common_migrate,FS_Error,"Storage*, const char*, const char*" Function,+,storage_common_mkdir,FS_Error,"Storage*, const char*" From 913a86bbec7c9590692e00c1a7b0a74c36a4360c Mon Sep 17 00:00:00 2001 From: EntranceJew Date: Sun, 15 Sep 2024 10:07:04 -0500 Subject: [PATCH 2/2] kerel typo (#3901) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: あく --- furi/core/kernel.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/furi/core/kernel.h b/furi/core/kernel.h index 2973a90e5..8362746e2 100644 --- a/furi/core/kernel.h +++ b/furi/core/kernel.h @@ -79,7 +79,7 @@ void furi_delay_tick(uint32_t ticks); * * @warning This should never be called in interrupt request context. * - * @param[in] tick The tick until which kerel should delay task execution + * @param[in] tick The tick until which kernel should delay task execution * * @return The furi status. */