From 2ccfe3da96cba837c2cf0a9695d1595d377b5b7d Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 21 Jun 2017 01:21:14 +0800 Subject: [PATCH] vfs: fix opendir of a filesystem root directory Previously opendir("/data") would fail if filesystem with "data" prefix was registered in VFS, while opendir("/data/") would succeed. This change fixes handling for the former case and adds relevant tests. --- components/fatfs/test/test_fatfs_common.c | 25 +++ components/fatfs/test/test_fatfs_common.h | 2 + components/fatfs/test/test_fatfs_sdmmc.c | 7 + components/fatfs/test/test_fatfs_spiflash.c | 7 + components/vfs/test/test_vfs_paths.c | 178 ++++++++++++++++++++ components/vfs/vfs.c | 18 +- 6 files changed, 231 insertions(+), 6 deletions(-) create mode 100644 components/vfs/test/test_vfs_paths.c diff --git a/components/fatfs/test/test_fatfs_common.c b/components/fatfs/test/test_fatfs_common.c index 47dca17e93..a2f35c3db7 100644 --- a/components/fatfs/test/test_fatfs_common.c +++ b/components/fatfs/test/test_fatfs_common.c @@ -227,6 +227,31 @@ void test_fatfs_mkdir_rmdir(const char* filename_prefix) TEST_ASSERT_EQUAL(0, rmdir(name_dir2)); } +void test_fatfs_can_opendir(const char* path) +{ + char name_dir_file[64]; + const char * file_name = "test_opd.txt"; + snprintf(name_dir_file, sizeof(name_dir_file), "%s/%s", path, file_name); + unlink(name_dir_file); + test_fatfs_create_file_with_text(name_dir_file, "test_opendir\n"); + DIR* dir = opendir(path); + TEST_ASSERT_NOT_NULL(dir); + bool found = false; + while (true) { + struct dirent* de = readdir(dir); + if (!de) { + break; + } + if (strcasecmp(de->d_name, file_name) == 0) { + found = true; + break; + } + } + TEST_ASSERT_TRUE(found); + TEST_ASSERT_EQUAL(0, closedir(dir)); + unlink(name_dir_file); +} + void test_fatfs_opendir_readdir_rewinddir(const char* dir_prefix) { char name_dir_inner_file[64]; diff --git a/components/fatfs/test/test_fatfs_common.h b/components/fatfs/test/test_fatfs_common.h index 84258c1dab..dcc31e37d4 100644 --- a/components/fatfs/test/test_fatfs_common.h +++ b/components/fatfs/test/test_fatfs_common.h @@ -53,6 +53,8 @@ void test_fatfs_concurrent(const char* filename_prefix); void test_fatfs_mkdir_rmdir(const char* filename_prefix); +void test_fatfs_can_opendir(const char* path); + void test_fatfs_opendir_readdir_rewinddir(const char* dir_prefix); void test_fatfs_rw_speed(const char* filename, void* buf, size_t buf_size, size_t file_size, bool write); diff --git a/components/fatfs/test/test_fatfs_sdmmc.c b/components/fatfs/test/test_fatfs_sdmmc.c index bc77d94c81..0c6d41aff5 100644 --- a/components/fatfs/test/test_fatfs_sdmmc.c +++ b/components/fatfs/test/test_fatfs_sdmmc.c @@ -130,6 +130,13 @@ TEST_CASE("(SD) can create and remove directories", "[fatfs][ignore]") test_teardown(); } +TEST_CASE("(WL) can opendir root directory of FS", "[fatfs][ignore]") +{ + test_setup(); + test_fatfs_can_opendir("/sdcard"); + test_teardown(); +} + TEST_CASE("(SD) opendir, readdir, rewinddir, seekdir work as expected", "[fatfs][ignore]") { test_setup(); diff --git a/components/fatfs/test/test_fatfs_spiflash.c b/components/fatfs/test/test_fatfs_spiflash.c index 971838db96..bcc396b93c 100644 --- a/components/fatfs/test/test_fatfs_spiflash.c +++ b/components/fatfs/test/test_fatfs_spiflash.c @@ -125,6 +125,13 @@ TEST_CASE("(WL) can create and remove directories", "[fatfs][wear_levelling]") test_teardown(); } +TEST_CASE("(WL) can opendir root directory of FS", "[fatfs][wear_levelling]") +{ + test_setup(); + test_fatfs_can_opendir("/spiflash"); + test_teardown(); +} + TEST_CASE("(WL) opendir, readdir, rewinddir, seekdir work as expected", "[fatfs][wear_levelling]") { test_setup(); diff --git a/components/vfs/test/test_vfs_paths.c b/components/vfs/test/test_vfs_paths.c new file mode 100644 index 0000000000..033a376db3 --- /dev/null +++ b/components/vfs/test/test_vfs_paths.c @@ -0,0 +1,178 @@ +// Copyright 2015-2017 Espressif Systems (Shanghai) PTE LTD +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include +#include +#include +#include +#include +#include +#include +#include "esp_vfs.h" +#include "unity.h" +#include "esp_log.h" + +static const char* TAG = "test_vfs_paths"; + +/* Dummy VFS implementation to check if VFS is called or not with expected path + */ +typedef struct { + const char* match_path; + bool called; +} dummy_vfs_t; + +static int dummy_open(void* ctx, const char * path, int flags, int mode) +{ + dummy_vfs_t* dummy = (dummy_vfs_t*) ctx; + dummy->called = true; + if (strcmp(dummy->match_path, path) == 0) { + return 1; + } + errno = ENOENT; + return -1; +} + +static int dummy_close(void* ctx, int fd) +{ + dummy_vfs_t* dummy = (dummy_vfs_t*) ctx; + dummy->called = true; + if (fd == 1) { + return 0; + } + errno = EBADF; + return -1; +} + +static DIR* dummy_opendir(void* ctx, const char* path) +{ + dummy_vfs_t* dummy = (dummy_vfs_t*) ctx; + dummy->called = true; + if (strcmp(dummy->match_path, path) == 0) { + DIR* result = calloc(1, sizeof(DIR)); + TEST_ASSERT_NOT_NULL(result); + return result; + } + errno = ENOENT; + return NULL; +} + +static int dummy_closedir(void* ctx, DIR* pdir) +{ + dummy_vfs_t* dummy = (dummy_vfs_t*) ctx; + dummy->called = true; + free(pdir); + return 0; +} + +/* Initializer for this dummy VFS implementation + */ + +#define DUMMY_VFS() { \ + .flags = ESP_VFS_FLAG_CONTEXT_PTR, \ + .open_p = dummy_open, \ + .close_p = dummy_close, \ + .opendir_p = dummy_opendir, \ + .closedir_p = dummy_closedir \ + } + +/* Helper functions to test VFS behavior + */ + +static void test_open(dummy_vfs_t* instance, const char* path, + bool should_be_called, bool should_be_opened, int line) +{ + const int flags = O_CREAT | O_TRUNC | O_RDWR; + instance->called = false; + int fd = esp_vfs_open(__getreent(), path, flags, 0); + UNITY_TEST_ASSERT_EQUAL_INT(should_be_called, instance->called, line, + "should_be_called check failed"); + if (should_be_called) { + if (should_be_opened) { + UNITY_TEST_ASSERT(fd >= 0, line, "should be opened"); + } else { + UNITY_TEST_ASSERT(fd < 0, line, "should not be opened"); + } + } + esp_vfs_close(__getreent(), fd); +} + +static void test_opendir(dummy_vfs_t* instance, const char* path, + bool should_be_called, bool should_be_opened, int line) +{ + instance->called = false; + DIR* dir = opendir(path); + UNITY_TEST_ASSERT_EQUAL_INT(should_be_called, instance->called, line, + "should_be_called check failed"); + if (should_be_called) { + if (should_be_opened) { + UNITY_TEST_ASSERT(dir != NULL, line, "should be opened"); + } else { + UNITY_TEST_ASSERT(dir == 0, line, "should not be opened"); + } + } + if (dir) { + closedir(dir); + } +} + +/* Helper macros which forward line number to assertion macros inside test_open + * and test_opendir + */ + +#define test_opened(instance, path) test_open(instance, path, true, true, __LINE__) +#define test_not_opened(instance, path) test_open(instance, path, true, false, __LINE__) +#define test_not_called(instance, path) test_open(instance, path, false, false, __LINE__) + +#define test_dir_opened(instance, path) test_opendir(instance, path, true, true, __LINE__) +#define test_dir_not_opened(instance, path) test_opendir(instance, path, true, false, __LINE__) +#define test_dir_not_called(instance, path) test_opendir(instance, path, false, false, __LINE__) + + + +TEST_CASE("vfs parses paths correctly", "[vfs]") +{ + dummy_vfs_t inst_foo = { + .match_path = "", + .called = false + }; + esp_vfs_t desc_foo = DUMMY_VFS(); + TEST_ESP_OK( esp_vfs_register("/foo", &desc_foo, &inst_foo) ); + + dummy_vfs_t inst_foo1 = { + .match_path = "", + .called = false + }; + esp_vfs_t desc_foo1 = DUMMY_VFS(); + TEST_ESP_OK( esp_vfs_register("/foo1", &desc_foo1, &inst_foo1) ); + + inst_foo.match_path = "/file"; + test_opened(&inst_foo, "/foo/file"); + test_not_opened(&inst_foo, "/foo/file1"); + test_not_called(&inst_foo, "/foo1/file"); + test_not_called(&inst_foo, "/foo1"); + test_not_opened(&inst_foo, "/foo"); + inst_foo.match_path = "/junk"; + test_dir_opened(&inst_foo, "/foo/junk"); + inst_foo.match_path = "/"; + test_dir_opened(&inst_foo, "/foo/"); + test_dir_opened(&inst_foo, "/foo"); + test_dir_not_called(&inst_foo1, "/foo"); + test_dir_not_opened(&inst_foo, "/foo/1"); + test_dir_not_called(&inst_foo, "/foo1"); + + inst_foo1.match_path = "/file1"; + test_not_called(&inst_foo1, "/foo/file1"); + test_opened(&inst_foo1, "/foo1/file1"); + test_not_opened(&inst_foo1, "/foo1/file"); +} diff --git a/components/vfs/vfs.c b/components/vfs/vfs.c index fff3a93a1d..b656cb22b9 100644 --- a/components/vfs/vfs.c +++ b/components/vfs/vfs.c @@ -1,4 +1,4 @@ -// Copyright 2015-2016 Espressif Systems (Shanghai) PTE LTD +// Copyright 2015-2017 Espressif Systems (Shanghai) PTE LTD // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -117,6 +117,10 @@ static int translate_fd(const vfs_entry_t* vfs, int fd) static const char* translate_path(const vfs_entry_t* vfs, const char* src_path) { assert(strncmp(src_path, vfs->path_prefix, vfs->path_prefix_len) == 0); + if (strlen(src_path) == vfs->path_prefix_len) { + // special case when src_path matches the path prefix exactly + return "/"; + } return src_path + vfs->path_prefix_len; } @@ -128,13 +132,15 @@ static const vfs_entry_t* get_vfs_for_path(const char* path) if (!vfs) { continue; } - if (len < vfs->path_prefix_len + 1) { // +1 is for the trailing slash after base path + // match path prefix + if (len < vfs->path_prefix_len || + memcmp(path, vfs->path_prefix, vfs->path_prefix_len) != 0) { continue; } - if (memcmp(path, vfs->path_prefix, vfs->path_prefix_len) != 0) { // match prefix - continue; - } - if (path[vfs->path_prefix_len] != '/') { // don't match "/data" prefix for "/data1/foo.txt" + // if path is not equal to the prefix, expect to see a path separator + // i.e. don't match "/data" prefix for "/data1/foo.txt" path + if (len > vfs->path_prefix_len && + path[vfs->path_prefix_len] != '/') { continue; } return vfs;