On Wed, May 18, 2022 at 6:25 AM Waldemar Kozaczuk <[email protected]> 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 <[email protected]> > --- > 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 [email protected]. > 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 [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/CANEVyjvyn3dr1Rq77_rvAiBms9qBAOoyBq40K%2BGANK8f1Ey6KA%40mail.gmail.com.
