From 4c54bd8bdb03fbbf9a8f7d2e93a8cda05b7ffab0 Mon Sep 17 00:00:00 2001 From: chrysn Date: Tue, 15 Feb 2022 10:20:00 +0100 Subject: [PATCH 1/8] vfs: Document that open directories are counted The addition is helpful because directories are visibly different from open files (and generally are not treated like files). --- sys/include/vfs.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sys/include/vfs.h b/sys/include/vfs.h index 334b5f959d..9ecec3057a 100644 --- a/sys/include/vfs.h +++ b/sys/include/vfs.h @@ -304,7 +304,7 @@ struct vfs_mount_struct { const vfs_file_system_t *fs; /**< The file system driver for the mount point */ const char *mount_point; /**< Mount point, e.g. "/mnt/cdrom" */ size_t mount_point_len; /**< Length of mount_point string (set by vfs_mount) */ - atomic_int open_files; /**< Number of currently open files */ + atomic_int open_files; /**< Number of currently open files and directories */ void *private_data; /**< File system driver private data, implementation defined */ }; @@ -889,7 +889,7 @@ int vfs_rename(const char *from_path, const char *to_path); /** * @brief Unmount a mounted file system * - * This will fail if there are any open files on the mounted file system + * This will fail if there are any open files or directories on the mounted file system * * @param[in] mountp pointer to the mount structure of the file system to unmount * From ff1f81aac8fa692f21d412e87af7cc0570ca8795 Mon Sep 17 00:00:00 2001 From: chrysn Date: Tue, 15 Feb 2022 11:42:18 +0100 Subject: [PATCH 2/8] sys/vfs: Drop per-filesystem fstatvfs No current file system implements it, there is no defined semantic difference between running fstatfs and the fallback currently implemented, and there is practically no optimization gained from not just running it through a single statvfs. --- sys/include/vfs.h | 17 ----------------- sys/vfs/vfs.c | 10 +++------- 2 files changed, 3 insertions(+), 24 deletions(-) diff --git a/sys/include/vfs.h b/sys/include/vfs.h index 9ecec3057a..5600094451 100644 --- a/sys/include/vfs.h +++ b/sys/include/vfs.h @@ -660,23 +660,6 @@ struct vfs_file_system_ops { * @return <0 on error */ int (*statvfs) (vfs_mount_t *mountp, const char *restrict path, struct statvfs *restrict buf); - - /** - * @brief Get file system status of an open file - * - * @p path is only passed for consistency against the POSIX statvfs function. - * @c vfs_statvfs calls this function only when it has determined that - * @p path belongs to this file system. @p path is a file system relative - * path and does not necessarily name an existing file. - * - * @param[in] mountp file system mount to operate on - * @param[in] filp pointer to an open file on the file system being queried - * @param[out] buf pointer to statvfs struct to fill - * - * @return 0 on success - * @return <0 on error - */ - int (*fstatvfs) (vfs_mount_t *mountp, vfs_file_t *filp, struct statvfs *buf); }; /** diff --git a/sys/vfs/vfs.c b/sys/vfs/vfs.c index bb81cf71bc..c8fed3c4ab 100644 --- a/sys/vfs/vfs.c +++ b/sys/vfs/vfs.c @@ -216,15 +216,11 @@ int vfs_fstatvfs(int fd, struct statvfs *buf) } vfs_file_t *filp = &_vfs_open_files[fd]; memset(buf, 0, sizeof(*buf)); - if (filp->mp->fs->fs_op->fstatvfs == NULL) { - /* file system driver does not implement fstatvfs() */ - if (filp->mp->fs->fs_op->statvfs != NULL) { - /* Fall back to statvfs */ - return filp->mp->fs->fs_op->statvfs(filp->mp, "/", buf); - } + if (filp->mp->fs->fs_op->statvfs == NULL) { + /* file system driver does not implement statvfs() */ return -EINVAL; } - return filp->mp->fs->fs_op->fstatvfs(filp->mp, filp, buf); + return filp->mp->fs->fs_op->statvfs(filp->mp, "/", buf); } off_t vfs_lseek(int fd, off_t off, int whence) From 599eade49522cd5ea756b1ed9f6964527ec45c9a Mon Sep 17 00:00:00 2001 From: chrysn Date: Tue, 15 Feb 2022 11:56:15 +0100 Subject: [PATCH 3/8] sys/vfs: Add vfs_dstatvfs --- sys/include/vfs.h | 11 +++++++++++ sys/vfs/vfs.c | 14 ++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/sys/include/vfs.h b/sys/include/vfs.h index 5600094451..dc444dcdff 100644 --- a/sys/include/vfs.h +++ b/sys/include/vfs.h @@ -715,6 +715,17 @@ int vfs_fstat(int fd, struct stat *buf); */ int vfs_fstatvfs(int fd, struct statvfs *buf); +/** + * @brief Get file system status of the file system containing an open directory + * + * @param[in] dirp pointer to open directory + * @param[out] buf pointer to statvfs struct to fill + * + * @return 0 on success + * @return <0 on error + */ +int vfs_dstatvfs(vfs_DIR *dirp, struct statvfs *buf); + /** * @brief Seek to position in file * diff --git a/sys/vfs/vfs.c b/sys/vfs/vfs.c index c8fed3c4ab..227e087b88 100644 --- a/sys/vfs/vfs.c +++ b/sys/vfs/vfs.c @@ -223,6 +223,20 @@ int vfs_fstatvfs(int fd, struct statvfs *buf) return filp->mp->fs->fs_op->statvfs(filp->mp, "/", buf); } +int vfs_dstatvfs(vfs_DIR *dirp, struct statvfs *buf) +{ + DEBUG("vfs_dstatvfs: %p, %p\n", (void*)dirp, (void *)buf); + if (buf == NULL) { + return -EFAULT; + } + memset(buf, 0, sizeof(*buf)); + if (dirp->mp->fs->fs_op->statvfs == NULL) { + /* file system driver does not implement statvfs() */ + return -EINVAL; + } + return dirp->mp->fs->fs_op->statvfs(dirp->mp, "/", buf); +} + off_t vfs_lseek(int fd, off_t off, int whence) { DEBUG("vfs_lseek: %d, %ld, %d\n", fd, (long)off, whence); From 9a319a9b4f0a1e4311c3457ac7be20f74cb3e19c Mon Sep 17 00:00:00 2001 From: chrysn Date: Tue, 15 Feb 2022 14:53:24 +0100 Subject: [PATCH 4/8] sys/vfs: Iterate over file systems in sequence This is a cosmetic change when considering vfs_iterate_mounts alone (which now reports FSs in precise mount order rather than reporting the most recently mounted, followed by the rest in mount order), but is helpful for uses of it that need some guarantees in order to produce reliable results even when iteration and (un)mounting interact. --- sys/vfs/vfs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sys/vfs/vfs.c b/sys/vfs/vfs.c index 227e087b88..fdc91bfd56 100644 --- a/sys/vfs/vfs.c +++ b/sys/vfs/vfs.c @@ -896,10 +896,11 @@ const vfs_mount_t *vfs_iterate_mounts(const vfs_mount_t *cur) /* empty list */ return NULL; } + node = node->next; } else { node = cur->list_entry.next; - if (node == _vfs_mounts_list.next) { + if (node == _vfs_mounts_list.next->next) { return NULL; } } From 90069197f703dd8b38c4346f72cad3549e07cd9b Mon Sep 17 00:00:00 2001 From: chrysn Date: Tue, 15 Feb 2022 14:48:54 +0100 Subject: [PATCH 5/8] sys/vfs: Introduce vfs_iterate_mount_dirs ... as thread-safe replacement for vfs_iterate_mounts --- sys/include/vfs.h | 29 +++++++++++++++++++++++++++++ sys/vfs/vfs.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 75 insertions(+), 1 deletion(-) diff --git a/sys/include/vfs.h b/sys/include/vfs.h index dc444dcdff..62714afc9f 100644 --- a/sys/include/vfs.h +++ b/sys/include/vfs.h @@ -1007,6 +1007,35 @@ int vfs_normalize_path(char *buf, const char *path, size_t buflen); */ const vfs_mount_t *vfs_iterate_mounts(const vfs_mount_t *cur); +/** + * @brief Iterate through all mounted file systems by their root directories + * + * Unlike @ref vfs_iterate_mounts, this is thread safe, and allows thread safe + * access to the mount point's stats through @ref vfs_dstatvfs. If mounts or + * unmounts happen while iterating, this is guaranteed to report all file + * systems that stayed mounted, and may report any that are transiently + * mounted. Note that the volume being reported can not be unmounted as @p dir + * is an open directory. + * + * Zero-initialize @p dir to start. As long as @c true is returned, @p dir is a + * valid directory on which the user can call @ref vfs_readdir or @ref + * vfs_dstatvfs (or even peek at its `.mp` if they dare ignore the warning in + * @ref vfs_DIR). + * + * Users MUST NOT call @ref vfs_closedir if they intend to keep iterating, but + * MUST call it when aborting iteration. + * + * Note that this requires all enumerated file systems to support the `opendir` + * @ref vfs_dir_ops; any file system that does not support that will + * prematurely terminate the mount point enumeration. + * + * @param[inout] dir The root directory of the discovered mount point + * + * @return @c true if another file system is mounted; @p dir then contains an open directory. + * @return @c false if the file system list is exhausted; @p dir is uninitialized then. + */ +bool vfs_iterate_mount_dirs(vfs_DIR *dir); + /** * @brief Get information about the file for internal purposes * diff --git a/sys/vfs/vfs.c b/sys/vfs/vfs.c index fdc91bfd56..6b8c482d54 100644 --- a/sys/vfs/vfs.c +++ b/sys/vfs/vfs.c @@ -522,7 +522,7 @@ int vfs_mount(vfs_mount_t *mountp) } } } - /* insert last in list */ + /* Insert last in list. This property is relied on by vfs_iterate_mount_dirs. */ clist_rpush(&_vfs_mounts_list, &mountp->list_entry); mutex_unlock(&_mount_mutex); DEBUG("vfs_mount: mount done\n"); @@ -907,6 +907,51 @@ const vfs_mount_t *vfs_iterate_mounts(const vfs_mount_t *cur) return container_of(node, vfs_mount_t, list_entry); } +/* General implementation note: This heavily relies on the produced opened dir + * to lock keep the underlying mount point from closing. */ +bool vfs_iterate_mount_dirs(vfs_DIR *dir) +{ + /* This is NULL after the prescribed initialization, or a valid (and + * locked) mount point otherwise */ + vfs_mount_t *last_mp = dir->mp; + + /* This is technically violating vfs_iterate_mounts' API, as that says no + * mounts or unmounts on the chain while iterating. However, as we know + * that the current dir's mount point is still on, the equivalent procedure + * of starting a new round of `vfs_iterate_mounts` from NULL and calling it + * until it produces `last_mp` (all while holding _mount_mutex) would leave + * us with the very same situation as if we started iteration with last_mp. + * + * On the cast discarding const: vfs_iterate_mounts's type is more for + * public use */ + vfs_mount_t *next = (vfs_mount_t *)vfs_iterate_mounts(last_mp); + if (next == NULL) { + /* Ignoring errors, can't help with them */ + vfs_closedir(dir); + return false; + } + + /* Even if we held the mutex up to here (see above comment on the fiction + * of acquiring it, iterating to where we are, and releasing it again), + * we'd need to let go of it now to actually open the directory. This + * temporary count ensures that the file system will stick around for the + * directory open step that follows immediately */ + atomic_fetch_add(&next->open_files, 1); + + /* Ignoring errors, can't help with them */ + vfs_closedir(dir); + + int err = vfs_opendir(dir, next->mount_point); + /* No matter the success, the open_files lock has done its duty */ + atomic_fetch_sub(&next->open_files, 1); + + if (err != 0) { + DEBUG("vfs_iterate_mount opendir erred: vfs_opendir(\"%s\") = %d\n", next->mount_point, err); + return false; + } + return true; +} + const vfs_file_t *vfs_file_get(int fd) { if (_fd_is_valid(fd) == 0) { From f0e80ee10c169c2d5fdf4a7f2c63f98ab395399e Mon Sep 17 00:00:00 2001 From: chrysn Date: Tue, 15 Feb 2022 15:42:07 +0100 Subject: [PATCH 6/8] tests: Cover vfs_iterate_mount_dirs in a new test ... adding precision to the documentation where a corner case was discovered during testing and is permitted. The test is too large for one small board, just like the other existing VFS test. --- sys/include/vfs.h | 4 +- tests/vfs_iterate_mount/Makefile | 6 ++ tests/vfs_iterate_mount/Makefile.ci | 3 + tests/vfs_iterate_mount/main.c | 124 ++++++++++++++++++++++++ tests/vfs_iterate_mount/tests/01-run.py | 23 +++++ 5 files changed, 158 insertions(+), 2 deletions(-) create mode 100644 tests/vfs_iterate_mount/Makefile create mode 100644 tests/vfs_iterate_mount/Makefile.ci create mode 100644 tests/vfs_iterate_mount/main.c create mode 100755 tests/vfs_iterate_mount/tests/01-run.py diff --git a/sys/include/vfs.h b/sys/include/vfs.h index 62714afc9f..1f3ebedbd9 100644 --- a/sys/include/vfs.h +++ b/sys/include/vfs.h @@ -1014,8 +1014,8 @@ const vfs_mount_t *vfs_iterate_mounts(const vfs_mount_t *cur); * access to the mount point's stats through @ref vfs_dstatvfs. If mounts or * unmounts happen while iterating, this is guaranteed to report all file * systems that stayed mounted, and may report any that are transiently - * mounted. Note that the volume being reported can not be unmounted as @p dir - * is an open directory. + * mounted for up to as often as they are (re)mounted. Note that the volume + * being reported can not be unmounted as @p dir is an open directory. * * Zero-initialize @p dir to start. As long as @c true is returned, @p dir is a * valid directory on which the user can call @ref vfs_readdir or @ref diff --git a/tests/vfs_iterate_mount/Makefile b/tests/vfs_iterate_mount/Makefile new file mode 100644 index 0000000000..c05a41a42b --- /dev/null +++ b/tests/vfs_iterate_mount/Makefile @@ -0,0 +1,6 @@ +include ../Makefile.tests_common + +USEMODULE += vfs +USEMODULE += constfs + +include $(RIOTBASE)/Makefile.include diff --git a/tests/vfs_iterate_mount/Makefile.ci b/tests/vfs_iterate_mount/Makefile.ci new file mode 100644 index 0000000000..b9ff275375 --- /dev/null +++ b/tests/vfs_iterate_mount/Makefile.ci @@ -0,0 +1,3 @@ +BOARD_INSUFFICIENT_MEMORY := \ + nucleo-l011k4 \ + # diff --git a/tests/vfs_iterate_mount/main.c b/tests/vfs_iterate_mount/main.c new file mode 100644 index 0000000000..6d415fa4bd --- /dev/null +++ b/tests/vfs_iterate_mount/main.c @@ -0,0 +1,124 @@ +/* + * Copyright (C) Christian Amsüss + * + * This file is subject to the terms and conditions of the GNU Lesser + * General Public License v2.1. See the file LICENSE in the top level + * directory for more details. + */ + +/** + * Mount and unmount a few file systems, demonstrating that + * vfs_iterate_mount_dirs performs as advertised. + * + * @author Christian Amsüss + */ + +#include +#include +#include + +#include +#include + +static constfs_file_t constfs_files[1] = { + /* Not completely empty -- that'd be a hassle around empty arrays and + * their size */ + { + .path = "some-file", + .size = 0, + .data = (void*)"", + }, +}; + +static constfs_t constfs_desc = { + .nfiles = ARRAY_SIZE(constfs_files), + .files = constfs_files, +}; + +static vfs_mount_t mount1 = { + .fs = &constfs_file_system, + .mount_point = "/const1", + .private_data = &constfs_desc, +}; + +static vfs_mount_t mount2 = { + .fs = &constfs_file_system, + .mount_point = "/const2", + .private_data = &constfs_desc, +}; + +static vfs_mount_t mount3 = { + .fs = &constfs_file_system, + .mount_point = "/const3", + .private_data = &constfs_desc, +}; + +static vfs_mount_t mount4 = { + .fs = &constfs_file_system, + .mount_point = "/const4", + .private_data = &constfs_desc, +}; + +/* Crank the iterator, reporting "N%s" for the next entry, or "O\n" for the end + * of the iterator (avoiding the letter "E" which may be misread for an error + * in a casual look at the error output) */ +static void iter_and_report(vfs_DIR *iter) { + bool result = vfs_iterate_mount_dirs(iter); + if (result) { + printf("N(%s)", iter->mp->mount_point); + } else { + printf("O\n"); + /* Zero out so we're ready for next round immediately */ + memset(iter, 0, sizeof(*iter)); + } +} + +int main(void) { + int res = 0; + + vfs_DIR iter; + memset(&iter, 0, sizeof(iter)); + + res |= vfs_mount(&mount1); + res |= vfs_mount(&mount2); + res |= vfs_mount(&mount3); + res |= vfs_mount(&mount4); + assert(res == 0); + printf("Mounted 1234\n"); + + /* N1N2N3N4E */ + iter_and_report(&iter); + iter_and_report(&iter); + iter_and_report(&iter); + iter_and_report(&iter); + iter_and_report(&iter); + + /* N1N2, unmount 3, N4E */ + iter_and_report(&iter); + iter_and_report(&iter); + res |= vfs_umount(&mount3); + iter_and_report(&iter); + iter_and_report(&iter); + + /* N1, unmount 2, (3 is already unmounted), N4, mount 3 N3, unmount 1 and remount it at the end N1, O */ + /* It is OK that 1 is reported twice, because its first occurrence is its + * old mounting, and later it reappears */ + iter_and_report(&iter); + res |= vfs_umount(&mount2); + iter_and_report(&iter); + res |= vfs_mount(&mount3); + iter_and_report(&iter); + res |= vfs_umount(&mount1); + res |= vfs_mount(&mount1); + iter_and_report(&iter); + iter_and_report(&iter); + + /* This ensures we're not leaking locks */ + res |= vfs_umount(&mount1); + res |= vfs_umount(&mount3); + res |= vfs_umount(&mount4); + printf("All unmounted\n"); + + /* Only O */ + iter_and_report(&iter); +} diff --git a/tests/vfs_iterate_mount/tests/01-run.py b/tests/vfs_iterate_mount/tests/01-run.py new file mode 100755 index 0000000000..5a705e8d86 --- /dev/null +++ b/tests/vfs_iterate_mount/tests/01-run.py @@ -0,0 +1,23 @@ +#!/usr/bin/env python3 + +# Copyright (C) 2021 Christian Amsüss +# +# This file is subject to the terms and conditions of the GNU Lesser +# General Public License v2.1. See the file LICENSE in the top level +# directory for more details. + +import sys +from testrunner import run + + +def testfunc(child): + child.expect_exact("Mounted 1234") + child.expect_exact("N(/const1)N(/const2)N(/const3)N(/const4)O") + child.expect_exact("N(/const1)N(/const2)N(/const4)O") + child.expect_exact("N(/const1)N(/const4)N(/const3)N(/const1)O") + child.expect_exact("All unmounted") + child.expect_exact("O") + + +if __name__ == "__main__": + sys.exit(run(testfunc)) From b9b0ca8972bd148c958178545ff65b0a50defc82 Mon Sep 17 00:00:00 2001 From: chrysn Date: Wed, 16 Feb 2022 17:00:20 +0100 Subject: [PATCH 7/8] sys/shell/vfs: Use vfs_iter_mount_dirs instead of vfs_iter_mounts This solves highly theoretical race conditions of file systems being unmounted in an application while a shell `df` runs, fixes the previous weird behavior that `/mountpoint/non-existant-path` could be df'd and would even report that non-existant path as a file name, but more practically ensures that an example of vfs_iter_mount_dirs is around. --- sys/include/vfs.h | 4 ++-- sys/shell/commands/sc_vfs.c | 25 ++++++++++++++++++------- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/sys/include/vfs.h b/sys/include/vfs.h index 1f3ebedbd9..31da8d5011 100644 --- a/sys/include/vfs.h +++ b/sys/include/vfs.h @@ -998,8 +998,6 @@ int vfs_normalize_path(char *buf, const char *path, size_t buflen); * * Set @p cur to @c NULL to start from the beginning * - * @see @c sc_vfs.c (@c df command) for a usage example - * * @param[in] cur current iterator value * * @return Pointer to next mounted file system in list after @p cur @@ -1029,6 +1027,8 @@ const vfs_mount_t *vfs_iterate_mounts(const vfs_mount_t *cur); * @ref vfs_dir_ops; any file system that does not support that will * prematurely terminate the mount point enumeration. * + * @see @c sc_vfs.c (@c df command) for a usage example + * * @param[inout] dir The root directory of the discovered mount point * * @return @c true if another file system is mounted; @p dir then contains an open directory. diff --git a/sys/shell/commands/sc_vfs.c b/sys/shell/commands/sc_vfs.c index 93bb7f855d..2612ac550c 100644 --- a/sys/shell/commands/sc_vfs.c +++ b/sys/shell/commands/sc_vfs.c @@ -102,11 +102,11 @@ static int _errno_string(int err, char *buf, size_t buflen) } #undef _case_snprintf_errno_name -static void _print_df(const char *path) +static void _print_df(vfs_DIR *dir) { struct statvfs buf; - int res = vfs_statvfs(path, &buf); - printf("%-16s ", path); + int res = vfs_dstatvfs(dir, &buf); + printf("%-16s ", dir->mp->mount_point); if (res < 0) { char err[16]; _errno_string(res, err, sizeof(err)); @@ -123,13 +123,24 @@ static int _df_handler(int argc, char **argv) puts("Mountpoint Total Used Available Capacity"); if (argc > 1) { const char *path = argv[1]; - _print_df(path); + /* Opening a directory just to statfs is somewhat odd, but it is the + * easiest to support with a single _print_df function */ + vfs_DIR dir; + int res = vfs_opendir(&dir, path); + if (res == 0) { + _print_df(&dir); + vfs_closedir(&dir); + } else { + char err[16]; + _errno_string(res, err, sizeof(err)); + printf("Failed to open `%s`: %s\n", path, err); + } } else { /* Iterate through all mount points */ - const vfs_mount_t *it = NULL; - while ((it = vfs_iterate_mounts(it)) != NULL) { - _print_df(it->mount_point); + vfs_DIR it = { 0 }; + while (vfs_iterate_mount_dirs(&it)) { + _print_df(&it); } } return 0; From d60f7aee7200e2f81917ff1deb9d71cb7a51f199 Mon Sep 17 00:00:00 2001 From: chrysn Date: Wed, 16 Feb 2022 17:05:41 +0100 Subject: [PATCH 8/8] sys/vfs: Deprecate public use of vfs_iterate_mounts --- sys/include/vfs.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sys/include/vfs.h b/sys/include/vfs.h index 31da8d5011..22fc797871 100644 --- a/sys/include/vfs.h +++ b/sys/include/vfs.h @@ -998,6 +998,9 @@ int vfs_normalize_path(char *buf, const char *path, size_t buflen); * * Set @p cur to @c NULL to start from the beginning * + * @deprecated This will become an internal-only function after the 2022.04 + * release, use @ref vfs_iterate_mount_dirs instead. + * * @param[in] cur current iterator value * * @return Pointer to next mounted file system in list after @p cur