Merge branch 'bugfix/vfs_fixes' into 'master'

VFS fixes

See merge request !901

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];

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);

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();

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();

components/vfs/README.rst

@@ -70,15 +70,20 @@ Paths
 
 Each registered FS has a path prefix associated with it. This prefix may be considered a "mount point" of this partition.
 
-Registering mount points which have another mount point as a prefix is not supported and results in undefined behavior. For instance, the following is correct and supported:
-
-- FS 1 on /data/fs1
-- FS 2 on /data/fs2
-
-This **will not work** as expected:
+In case when mount points are nested, the mount point with the longest matching path prefix is used when opening the file. For instance, suppose that the following filesystems are registered in VFS:
 
 - FS 1 on /data
-- FS 2 on /data/fs2
+- FS 2 on /data/static
+
+Then:
+
+- FS 1 will be used when opening a file called ``/data/log.txt``
+- FS 2 will be used when opening a file called ``/data/static/index.html``
+- Even if ``/index.html"`` doesn't exist in FS 2, FS 1 will *not* be searched for ``/static/index.html``.
+
+As a general rule, mount point names must start with the path separator (``/``) and must contain at least one character after path separator. However an empty mount point name is also supported, and may be used in cases when application needs to provide "fallback" filesystem, or override VFS functionality altogether. Such filesystem will be used if no prefix matches the path given.
+
+VFS does not handle dots (``.``) in path names in any special way. VFS does not treat ``..`` as a reference to the parent directory. I.e. in the above example, using a path ``/data/static/../log.txt`` will not result in a call to FS 1 to open ``/log.txt``. Specific FS drivers (such as FATFS) may handle dots in file names differently.
 
 When opening files, FS driver will only be given relative path to files. For example:
 

components/vfs/test/test_vfs_paths.c

@@ -0,0 +1,241 @@
+// 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 <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include <sys/fcntl.h>
+#include <sys/dirent.h>
+#include "esp_vfs.h"
+#include "unity.h"
+#include "esp_log.h"
+
+/* 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");
+
+    // Test nested VFS entries
+    dummy_vfs_t inst_foobar = {
+        .match_path = "",
+        .called = false
+    };
+    esp_vfs_t desc_foobar = DUMMY_VFS();
+    TEST_ESP_OK( esp_vfs_register("/foo/bar", &desc_foobar, &inst_foobar) );
+
+    dummy_vfs_t inst_toplevel = {
+        .match_path = "",
+        .called = false
+    };
+    esp_vfs_t desc_toplevel = DUMMY_VFS();
+    TEST_ESP_OK( esp_vfs_register("", &desc_toplevel, &inst_toplevel) );
+
+    inst_foo.match_path = "/bar/file";
+    inst_foobar.match_path = "/file";
+    test_not_called(&inst_foo, "/foo/bar/file");
+    test_opened(&inst_foobar, "/foo/bar/file");
+    test_dir_not_called(&inst_foo, "/foo/bar/file");
+    test_dir_opened(&inst_foobar, "/foo/bar/file");
+    inst_toplevel.match_path = "/tmp/foo";
+    test_opened(&inst_toplevel, "/tmp/foo");
+
+    TEST_ESP_OK( esp_vfs_unregister("/foo") );
+    TEST_ESP_OK( esp_vfs_unregister("/foo1") );
+    TEST_ESP_OK( esp_vfs_unregister("/foo/bar") );
+    TEST_ESP_OK( esp_vfs_unregister("") );
+}
+
+
+void test_vfs_register(const char* prefix, bool expect_success, int line)
+{
+    dummy_vfs_t inst;
+    esp_vfs_t desc = DUMMY_VFS();
+    esp_err_t err = esp_vfs_register(prefix, &desc, &inst);
+    if (expect_success) {
+        UNITY_TEST_ASSERT_EQUAL_INT(ESP_OK, err, line, "esp_vfs_register should succeed");
+    } else {
+        UNITY_TEST_ASSERT_EQUAL_INT(ESP_ERR_INVALID_ARG,
+                err, line, "esp_vfs_register should fail");
+    }
+    if (err == ESP_OK) {
+        TEST_ESP_OK( esp_vfs_unregister(prefix) );
+    }
+}
+
+#define test_register_ok(prefix) test_vfs_register(prefix, true, __LINE__)
+#define test_register_fail(prefix) test_vfs_register(prefix, false, __LINE__)
+
+TEST_CASE("vfs checks mount point path", "[vfs]")
+{
+    test_register_ok("");
+    test_register_fail("/");
+    test_register_fail("a");
+    test_register_fail("aa");
+    test_register_fail("aaa");
+    test_register_ok("/a");
+    test_register_ok("/aa");
+    test_register_ok("/aaa/bbb");
+    test_register_fail("/aaa/");
+    test_register_fail("/aaa/bbb/");
+    test_register_ok("/23456789012345");
+    test_register_fail("/234567890123456");
+}

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.
@@ -55,10 +55,10 @@ static size_t s_vfs_count = 0;
 esp_err_t esp_vfs_register(const char* base_path, const esp_vfs_t* vfs, void* ctx)
 {
     size_t len = strlen(base_path);
-    if (len < 2 || len > ESP_VFS_PATH_MAX) {
+    if ((len != 0 && len < 2)|| len > ESP_VFS_PATH_MAX) {
         return ESP_ERR_INVALID_ARG;
     }
-    if (base_path[0] != '/' || base_path[len - 1] == '/') {
+    if ((len > 0 && base_path[0] != '/') || base_path[len - 1] == '/') {
         return ESP_ERR_INVALID_ARG;
     }
     vfs_entry_t *entry = (vfs_entry_t*) malloc(sizeof(vfs_entry_t));
@@ -91,6 +91,9 @@ esp_err_t esp_vfs_unregister(const char* base_path)
 {
     for (size_t i = 0; i < s_vfs_count; ++i) {
         vfs_entry_t* vfs = s_vfs[i];
+        if (vfs == NULL) {
+            continue;
+        }
         if (memcmp(base_path, vfs->path_prefix, vfs->path_prefix_len) == 0) {
             free(vfs);
             s_vfs[i] = NULL;
@@ -117,29 +120,46 @@ 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;
 }
 
 static const vfs_entry_t* get_vfs_for_path(const char* path)
 {
+    const vfs_entry_t* best_match = NULL;
+    ssize_t best_match_prefix_len = -1;
     size_t len = strlen(path);
     for (size_t i = 0; i < s_vfs_count; ++i) {
         const vfs_entry_t* vfs = s_vfs[i];
         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
+        // 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;
         }
-        if (path[vfs->path_prefix_len] != '/') {   // don't match "/data" prefix for "/data1/foo.txt"
-            continue;
+        // Out of all matching path prefixes, select the longest one;
+        // i.e. if "/dev" and "/dev/uart" both match, for "/dev/uart/1" path,
+        // choose "/dev/uart",
+        // This causes all s_vfs_count VFS entries to be scanned when opening
+        // a file by name. This can be optimized by introducing a table for
+        // FS search order, sorted so that longer prefixes are checked first.
+        if (best_match_prefix_len < (ssize_t) vfs->path_prefix_len) {
+            best_match_prefix_len = (ssize_t) vfs->path_prefix_len;
+            best_match = vfs;
         }
-        return vfs;
     }
-    return NULL;
+    return best_match;
 }
 
 /*