On Wed, May 18, 2022 at 6:25 AM Waldemar Kozaczuk <jwkozac...@gmail.com>
wrote:
> The VFS functions like openat(), faccessat() and others alike
> take a directory descriptor argument intended to make a filesystem
> action happen relative to that directory.
>
> The implemetations of those function repeat almost the same code
> over and over. So this patch makes vfs more DRY by introducing
> a helper function - vfs_fun_at() - which implements common
> logic and executed a lambda function specific to given
> VFS action.
>
> This patch also adds some unit tests around readlinkat() and mkdirat().
>
> Signed-off-by: Waldemar Kozaczuk <jwkozac...@gmail.com>
> ---
> fs/vfs/main.cc | 188 +++++++++++--------------------------------
> tests/tst-readdir.cc | 9 +++
> tests/tst-symlink.cc | 12 ++-
> 3 files changed, 66 insertions(+), 143 deletions(-)
>
> diff --git a/fs/vfs/main.cc b/fs/vfs/main.cc
> index ca357cc8..a72042b2 100644
> --- a/fs/vfs/main.cc
> +++ b/fs/vfs/main.cc
> @@ -160,21 +160,8 @@ int open(const char *pathname, int flags, ...)
>
> LFS64(open);
>
> -OSV_LIBC_API
> -int openat(int dirfd, const char *pathname, int flags, ...)
> +static int vfs_fun_at(int dirfd, const char *pathname,
> std::function<int(const char *)> fun)
>
Nitpick: I think if you want zero runtime overhead for this function, you
can use a template
parameter for this "fun" instead of std::function. But I don't know, maybe
the compiler is actually
smart enough to optimize all this std::function stuff away when inlining
vfs_fun_at calls?
And it's probably negligible anyway compared to all those strlcat calls :-)
{
> - mode_t mode = 0;
> - if (flags & O_CREAT) {
> - va_list ap;
> - va_start(ap, flags);
> - mode = apply_umask(va_arg(ap, mode_t));
> - va_end(ap);
> - }
> -
> - if (pathname[0] == '/' || dirfd == AT_FDCWD) {
> - return open(pathname, flags, mode);
> - }
>
Unless I'm misunderstanding something, I think this if() should remain (and
of
course call fun(), not open()). More on this below.
-
> struct file *fp;
> int error = fget(dirfd, &fp);
> if (error) {
> @@ -191,16 +178,38 @@ int openat(int dirfd, const char *pathname, int
> flags, ...)
> /* build absolute path */
> strlcpy(p, fp->f_dentry->d_mount->m_path, PATH_MAX);
> strlcat(p, fp->f_dentry->d_path, PATH_MAX);
> - strlcat(p, "/", PATH_MAX);
> - strlcat(p, pathname, PATH_MAX);
> + if (pathname) {
>
Is any *at() function actually allowed to pass a null pathname? (which is
not the same as an empty string).
If Linux allows this, we should have a test for it.
+ strlcat(p, "/", PATH_MAX);
> + strlcat(p, pathname, PATH_MAX);
> + }
>
> - error = open(p, flags, mode);
> + error = fun(p);
>
> vn_unlock(vp);
> fdrop(fp);
>
> return error;
> }
> +
> +OSV_LIBC_API
> +int openat(int dirfd, const char *pathname, int flags, ...)
> +{
> + mode_t mode = 0;
> + if (flags & O_CREAT) {
> + va_list ap;
> + va_start(ap, flags);
> + mode = apply_umask(va_arg(ap, mode_t));
> + va_end(ap);
> + }
>
+
> + if (pathname[0] == '/' || dirfd == AT_FDCWD) {
> + return open(pathname, flags, mode);
> + }
>
I think this if() should be part of the general vfs_fun_at() code.
I looked at openat(2), mkdirat(2), fdaccessat(2) for example - and they
all support AT_FDCWD. Are there functions which don't?
+
> + return vfs_fun_at(dirfd, pathname, [flags, mode](const char
> *absolute_path) {
> + return open(absolute_path, flags, mode);
> + });
> +}
> LFS64(openat);
>
> // open() has an optional third argument, "mode", which is only needed in
> @@ -611,35 +620,14 @@ int __fxstatat(int ver, int dirfd, const char
> *pathname, struct stat *st,
> return fstat(dirfd, st);
> }
>
> - struct file *fp;
> - int error = fget(dirfd, &fp);
> - if (error) {
> - errno = error;
> - return -1;
> - }
> -
> - struct vnode *vp = fp->f_dentry->d_vnode;
> - vn_lock(vp);
> -
> - std::unique_ptr<char []> up (new char[PATH_MAX]);
> - char *p = up.get();
> - /* build absolute path */
> - strlcpy(p, fp->f_dentry->d_mount->m_path, PATH_MAX);
> - strlcat(p, fp->f_dentry->d_path, PATH_MAX);
> - strlcat(p, "/", PATH_MAX);
> - strlcat(p, pathname, PATH_MAX);
> -
> - if (flags & AT_SYMLINK_NOFOLLOW) {
> - error = lstat(p, st);
> - }
> - else {
> - error = stat(p, st);
> - }
> -
> - vn_unlock(vp);
> - fdrop(fp);
> -
> - return error;
> + return vfs_fun_at(dirfd, pathname, [flags,st](const char
> *absolute_path) {
> + if (flags & AT_SYMLINK_NOFOLLOW) {
> + return lstat(absolute_path, st);
> + }
> + else {
> + return stat(absolute_path, st);
> + }
> + });
> }
>
> LFS64(__fxstatat);
> @@ -875,32 +863,9 @@ int mkdirat(int dirfd, const char *pathname, mode_t
> mode)
> return mkdir(pathname, mode);
> }
>
> - // Supplied path is relative to folder specified by dirfd
> - struct file *fp;
> - int error = fget(dirfd, &fp);
> - if (error) {
> - errno = error;
> - return -1;
> - }
> -
> - struct vnode *vp = fp->f_dentry->d_vnode;
> - vn_lock(vp);
> -
> - std::unique_ptr<char []> up (new char[PATH_MAX]);
> - char *p = up.get();
> -
> - /* build absolute path */
> - strlcpy(p, fp->f_dentry->d_mount->m_path, PATH_MAX);
> - strlcat(p, fp->f_dentry->d_path, PATH_MAX);
> - strlcat(p, "/", PATH_MAX);
> - strlcat(p, pathname, PATH_MAX);
> -
> - error = mkdir(p, mode);
> -
> - vn_unlock(vp);
> - fdrop(fp);
> -
> - return error;
> + return vfs_fun_at(dirfd, pathname, [mode](const char *absolute_path) {
> + return mkdir(absolute_path, mode);
> + });
> }
>
> TRACEPOINT(trace_vfs_rmdir, "\"%s\"", const char*);
> @@ -1721,31 +1686,9 @@ int faccessat(int dirfd, const char *pathname, int
> mode, int flags)
> return access(pathname, mode);
> }
>
> - struct file *fp;
> - int error = fget(dirfd, &fp);
> - if (error) {
> - errno = error;
> - return -1;
> - }
> -
> - struct vnode *vp = fp->f_dentry->d_vnode;
> - vn_lock(vp);
> -
> - std::unique_ptr<char []> up (new char[PATH_MAX]);
> - char *p = up.get();
> -
> - /* build absolute path */
> - strlcpy(p, fp->f_dentry->d_mount->m_path, PATH_MAX);
> - strlcat(p, fp->f_dentry->d_path, PATH_MAX);
> - strlcat(p, "/", PATH_MAX);
> - strlcat(p, pathname, PATH_MAX);
> -
> - error = access(p, mode);
> -
> - vn_unlock(vp);
> - fdrop(fp);
> -
> - return error;
> + return vfs_fun_at(dirfd, pathname, [mode](const char *absolute_path) {
> + return access(absolute_path, mode);
> + });
> }
>
> extern "C" OSV_LIBC_API
> @@ -1932,31 +1875,9 @@ ssize_t readlinkat(int dirfd, const char *pathname,
> char *buf, size_t bufsize)
> return readlink(pathname, buf, bufsize);
> }
>
> - struct file *fp;
> - int error = fget(dirfd, &fp);
> - if (error) {
> - errno = error;
> - return -1;
> - }
> -
> - struct vnode *vp = fp->f_dentry->d_vnode;
> - vn_lock(vp);
> -
> - std::unique_ptr<char []> up (new char[PATH_MAX]);
> - char *p = up.get();
> -
> - /* build absolute path */
> - strlcpy(p, fp->f_dentry->d_mount->m_path, PATH_MAX);
> - strlcat(p, fp->f_dentry->d_path, PATH_MAX);
> - strlcat(p, "/", PATH_MAX);
> - strlcat(p, pathname, PATH_MAX);
> -
> - error = readlink(p, buf, bufsize);
> -
> - vn_unlock(vp);
> - fdrop(fp);
> -
> - return error;
> + return vfs_fun_at(dirfd, pathname, [buf, bufsize](const char
> *absolute_path) {
> + return readlink(absolute_path, buf, bufsize);
> + });
> }
>
> TRACEPOINT(trace_vfs_fallocate, "%d %d 0x%x 0x%x", int, int, loff_t,
> loff_t);
> @@ -2004,9 +1925,7 @@ OSV_LIBC_API
> int futimesat(int dirfd, const char *pathname, const struct timeval
> times[2])
> {
> struct stat st;
> - struct file *fp;
> int error;
> - char *absolute_path;
>
> if ((pathname && pathname[0] == '/') || dirfd == AT_FDCWD)
> return utimes(pathname, times);
> @@ -2026,24 +1945,9 @@ int futimesat(int dirfd, const char *pathname,
> const struct timeval times[2])
> }
> }
>
> - error = fget(dirfd, &fp);
> - if (error)
> - goto out_errno;
> -
> - /* build absolute path */
> - absolute_path = (char*)malloc(PATH_MAX);
> - strlcpy(absolute_path, fp->f_dentry->d_mount->m_path, PATH_MAX);
> - strlcat(absolute_path, fp->f_dentry->d_path, PATH_MAX);
> -
> - if (pathname) {
> - strlcat(absolute_path, "/", PATH_MAX);
> - strlcat(absolute_path, pathname, PATH_MAX);
> - }
> -
> - error = utimes(absolute_path, times);
> - free(absolute_path);
> -
> - fdrop(fp);
> + error = vfs_fun_at(dirfd, pathname, [times](const char
> *absolute_path) {
> + return utimes(absolute_path, times);
> + });
>
> if (error)
> goto out_errno;
> diff --git a/tests/tst-readdir.cc b/tests/tst-readdir.cc
> index 9154d252..0b1d2a8c 100644
> --- a/tests/tst-readdir.cc
> +++ b/tests/tst-readdir.cc
> @@ -216,6 +216,15 @@ int main(int argc, char **argv)
> report(unlink("/tmp/tst-readdir/d")==0, "unlink");
> report(rmdir("/tmp/tst-readdir")==0, "rmdir");
> report(rmdir("/tmp/tst-readdir-empty")==0, "rmdir empty");
> +
> + auto tmp_dir = opendir("/tmp");
> + report(tmp_dir,"opendir /tmp");
> + report(mkdirat(dirfd(tmp_dir), "tst-mkdirat", 0777)==0, "mkdirat");
> + auto mk_dir = opendir("/tmp/tst-mkdirat");
> + report(mk_dir,"opendir /tmp/tst-mkdirat");
> + report(rmdir("/tmp/tst-mkdirat")==0, "rmdir empty");
> + report(closedir(mk_dir)==0, "closedir /tmp/tst-mkdirat");
> + report(closedir(tmp_dir)==0, "closedir /tmp");
> #endif
>
> printf("SUMMARY: %d tests, %d failures\n", tests, fails);
> diff --git a/tests/tst-symlink.cc b/tests/tst-symlink.cc
> index 0eff52f3..978cfda3 100644
> --- a/tests/tst-symlink.cc
> +++ b/tests/tst-symlink.cc
> @@ -262,7 +262,6 @@ int main(int argc, char **argv)
> report(symlink(N1, path) == 0, "symlink");
> report(unlink(path) == 0, "unlink");
>
> - printf("-->Smok\n");
>
I wonder what this was ;-)
> fill_buf(path, 257);
> rc = symlink(N1, path);
> error = errno;
> @@ -366,12 +365,23 @@ int main(int argc, char **argv)
> report(search_dir(D2, N5) == true, "Symlink search");
>
> report(rename(D2, D3) == 0, "rename(d2, d3)");
> + auto test_dir = opendir(TESTDIR);
> + report(test_dir, "opendir");
> + rc = readlinkat(dirfd(test_dir), D3, path, sizeof(path));
> + report(rc >= 0, "readlinkat");
> + path[rc] = 0;
> + report(strcmp(path, D1) == 0, "readlinkat path");
> rc = readlink(D3, path, sizeof(path));
> report(rc >= 0, "readlink");
> path[rc] = 0;
> report(strcmp(path, D1) == 0, "readlink path");
>
> report(rename(D1, D4) == 0, "rename(d1, d4)");
> + rc = readlinkat(dirfd(test_dir), D3, path, sizeof(path));
> + report(rc >= 0, "readlinkat");
> + path[rc] = 0;
> + report(strcmp(path, D1) == 0, "readlinkat path");
> + report(closedir(test_dir) == 0, "closedir(test_dir)");
> rc = readlink(D3, path, sizeof(path));
> report(rc >= 0, "readlink");
> path[rc] = 0;
> --
> 2.34.1
>
> --
> You received this message because you are subscribed to the Google Groups
> "OSv Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to osv-dev+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/osv-dev/20220518032543.76756-1-jwkozaczuk%40gmail.com
> .
>
--
You received this message because you are subscribed to the Google Groups "OSv
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to osv-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/CANEVyjvyn3dr1Rq77_rvAiBms9qBAOoyBq40K%2BGANK8f1Ey6KA%40mail.gmail.com.
