Thanks for the review. On Wednesday, May 18, 2022 at 2:45:34 AM UTC-4 Nadav Har'El wrote:
> 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 :-) > > 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. > { > > >> - 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. > I need to handle 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 agree with your idea - I will improve it in the next patch. > > + >> + 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/4611f9d9-e10b-42dc-9243-bbb735440f16n%40googlegroups.com.
