[PATCH] vfs: refactor the *at() functions

[Waldemar Kozaczuk]

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 <jwkoz...@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)
{
- 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);
- }
-
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) {
+ 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);
+ }
+
+ 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");
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

[Nadav Har'El]

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?

I wonder what this was ;-)

 

--
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+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/20220518032543.76756-1-jwkozaczuk%40gmail.com.

[Waldek Kozaczuk]

Thanks for the review.

I think I will stick with the regular function. As you point out it comes with a overhead of 2 function calls

but on other hand it makes a binary smaller unlike templates. Plus these functions are normally not called thousand of times

by normal apps. But we can always change it later.

I need to handle it. 

I agree with your idea - I will improve it in the next patch. 

[Waldemar Kozaczuk]

Comparing to the 1st version, this one adds another helper
function - vfs_fun_at2() - which calls supplied lambda if
dirfd == AT_FDCWD or pathname is an absolute path and otherwise
delegates to vfs_fun_at(). It also checks if pathname is not null.
The __fxstatat() and futimesat() call vfs_fun_at() directly as their
logic is slightly different.

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

#Refs 1188

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
---

fs/vfs/main.cc | 216 ++++++++++++-------------------------------
tests/tst-readdir.cc | 9 ++
tests/tst-symlink.cc | 12 ++-
3 files changed, 81 insertions(+), 156 deletions(-)

diff --git a/fs/vfs/main.cc b/fs/vfs/main.cc
index ca357cc8..1b7eb588 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)

{

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

-
struct file *fp;
int error = fget(dirfd, &fp);
if (error) {

@@ -191,16 +178,48 @@ 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) {

+ strlcat(p, "/", PATH_MAX);
+ strlcat(p, pathname, PATH_MAX);
+ }

- error = open(p, flags, mode);
+ error = fun(p);

vn_unlock(vp);
fdrop(fp);

return error;
}
+

+static int vfs_fun_at2(int dirfd, const char *pathname, std::function<int(const char *)> fun)
+{
+ if (!pathname) {
+ errno = EINVAL;
+ return -1;

+ }
+
+ if (pathname[0] == '/' || dirfd == AT_FDCWD) {

+ return fun(pathname);
+ }
+
+ return vfs_fun_at(dirfd, pathname, fun);
+}

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

+ return vfs_fun_at2(dirfd, pathname, [flags, mode](const char *path) {
+ return open(path, flags, mode);

+ });
+}
LFS64(openat);

// open() has an optional third argument, "mode", which is only needed in

@@ -602,6 +621,11 @@ extern "C" OSV_LIBC_API

int __fxstatat(int ver, int dirfd, const char *pathname, struct stat *st,

int flags)
{
+ if (!pathname) {
+ errno = EINVAL;
+ return -1;

+ }
+
if (pathname[0] == '/' || dirfd == AT_FDCWD) {

return stat(pathname, st);
}
@@ -611,35 +635,14 @@ int __fxstatat(int ver, int dirfd, const char *pathname, struct stat *st,

@@ -870,37 +873,9 @@ int mkdirat(int dirfd, const char *pathname, mode_t mode)
{
mode = apply_umask(mode);

- if (pathname[0] == '/' || dirfd == AT_FDCWD) {

- // Supplied path is either absolute or relative to cwd
- return mkdir(pathname, mode);
- }
-

+ return vfs_fun_at2(dirfd, pathname, [mode](const char *path) {
+ return mkdir(path, mode);

+ });
}

TRACEPOINT(trace_vfs_rmdir, "\"%s\"", const char*);

@@ -1717,35 +1692,9 @@ int faccessat(int dirfd, const char *pathname, int mode, int flags)
UNIMPLEMENTED("faccessat() with AT_SYMLINK_NOFOLLOW");

}

- if (pathname[0] == '/' || dirfd == AT_FDCWD) {

- return access(pathname, mode);
- }
-

+ return vfs_fun_at2(dirfd, pathname, [mode](const char *path) {
+ return access(path, mode);

+ });
}

extern "C" OSV_LIBC_API

@@ -1928,35 +1877,9 @@ ssize_t readlink(const char *pathname, char *buf, size_t bufsize)
OSV_LIBC_API

ssize_t readlinkat(int dirfd, const char *pathname, char *buf, size_t bufsize)

{

- if (pathname[0] == '/' || dirfd == AT_FDCWD) {

- return readlink(pathname, buf, bufsize);
- }
-

+ return vfs_fun_at2(dirfd, pathname, [buf, bufsize](const char *path) {
+ return readlink(path, buf, bufsize);

+ });
}

TRACEPOINT(trace_vfs_fallocate, "%d %d 0x%x 0x%x", int, int, loff_t, loff_t);

@@ -2004,9 +1927,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 +1947,9 @@ int futimesat(int dirfd, const char *pathname, const struct timeval times[2])

[Commit Bot]

From: Waldemar Kozaczuk <jwkoz...@gmail.com>
Committer: Waldemar Kozaczuk <jwkoz...@gmail.com>
Branch: master

vfs: refactor the *at() functions

Comparing to the 1st version, this one adds another helper
function - vfs_fun_at2() - which calls supplied lambda if
dirfd == AT_FDCWD or pathname is an absolute path and otherwise
delegates to vfs_fun_at(). It also checks if pathname is not null.
The __fxstatat() and futimesat() call vfs_fun_at() directly as their
logic is slightly different.

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

#Refs 1188

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>

---

diff --git a/fs/vfs/main.cc b/fs/vfs/main.cc

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