Re: [PATCH v3 04/10] virtiofsd: Add lo_inode_fd() helper
On 06.08.21 20:25, Vivek Goyal wrote:
On Fri, Jul 30, 2021 at 05:01:28PM +0200, Max Reitz wrote:
[..]
@@ -1335,12 +1359,18 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t
parent,
return;
}
+res = lo_inode_fd(dir, _fd);
+if (res < 0) {
+saverr = -res;
+goto out;
+}
+
saverr = lo_change_cred(req, , lo->change_umask && !S_ISLNK(mode));
if (saverr) {
goto out;
}
-res = mknod_wrapper(dir->fd, name, link, mode, rdev);
+res = mknod_wrapper(dir_fd.fd, name, link, mode, rdev);
saverr = errno;
@@ -1388,6 +1418,8 @@ static void lo_symlink(fuse_req_t req, const char *link, fuse_ino_t parent,
static void lo_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent,
const char *name)
{
+g_auto(TempFd) inode_fd = TEMP_FD_INIT;
+g_auto(TempFd) parent_fd = TEMP_FD_INIT;
int res;
struct lo_data *lo = lo_data(req);
struct lo_inode *parent_inode;
@@ -1413,18 +1445,31 @@ static void lo_link(fuse_req_t req, fuse_ino_t ino,
fuse_ino_t parent,
goto out_err;
}
+res = lo_inode_fd(inode, _fd);
+if (res < 0) {
+errno = -res;
In previous function, we saved error to "saverr" and jumped to "out"
label, instead of overwriting to errno.
I would think that it will be good to use a single pattern. Either
save error in saverr or overwrite errno. I personally prefer saving
error into "saverr".
Absolutely, will do.
+goto out_err;
+}
+
+res = lo_inode_fd(parent_inode, _fd);
+if (res < 0) {
+errno = -res;
+goto out_err;
+}
+
memset(, 0, sizeof(struct fuse_entry_param));
e.attr_timeout = lo->timeout;
e.entry_timeout = lo->timeout;
-sprintf(procname, "%i", inode->fd);
-res = linkat(lo->proc_self_fd, procname, parent_inode->fd, name,
+sprintf(procname, "%i", inode_fd.fd);
+res = linkat(lo->proc_self_fd, procname, parent_fd.fd, name,
AT_SYMLINK_FOLLOW);
if (res == -1) {
goto out_err;
}
-res = fstatat(inode->fd, "", , AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
+res = fstatat(inode_fd.fd, "", ,
+ AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
if (res == -1) {
goto out_err;
}
@@ -1453,23 +1498,33 @@ out_err:
static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
const char *name)
{
+g_auto(TempFd) dir_fd = TEMP_FD_INIT;
int res;
uint64_t mnt_id;
struct stat attr;
struct lo_data *lo = lo_data(req);
struct lo_inode *dir = lo_inode(req, parent);
+struct lo_inode *inode = NULL;
if (!dir) {
-return NULL;
+goto out;
Should we continue to just call "return NULL". dir is NULL. That means
lo_inode() failed. That means we never got the reference. So we don't
have to put the reference. If we do "goto out", it will call
lo_inode_put() which is not needed.
Yes, but lo_inode_put() will handle this gracefully, so it isn’t wrong.
My personal preference is that if there is an clean-up path, it should
be used everywhere instead of having pure returns at the beginning of a
function (where not many resources have been initialized yet), so that
no clean-up will be forgotten. Like, if we were to add some resource
acquisition in the declarations above (and clean-up code in the clean-up
path), we would need to change the return to a goto here. Or maybe we’d
forget that, and then we’d leak something.
So I prefer having clean-up sections be generic enough that they can be
used from anywhere within the function, and then also use it from
anywhere within the function, even if they end up being no-ops.
}
-res = do_statx(lo, dir->fd, name, , AT_SYMLINK_NOFOLLOW, _id);
-lo_inode_put(lo, );
+res = lo_inode_fd(dir, _fd);
+if (res < 0) {
+goto out;
+}
+
+res = do_statx(lo, dir_fd.fd, name, , AT_SYMLINK_NOFOLLOW, _id);
if (res == -1) {
-return NULL;
+goto out;
}
-return lo_find(lo, , mnt_id);
+inode = lo_find(lo, , mnt_id);
+
+out:
+lo_inode_put(lo, );
+return inode;
}
Thanks
Vivek
Re: [PATCH v3 04/10] virtiofsd: Add lo_inode_fd() helper
On Fri, Jul 30, 2021 at 05:01:28PM +0200, Max Reitz wrote:
[..]
> @@ -1335,12 +1359,18 @@ static void lo_mknod_symlink(fuse_req_t req,
> fuse_ino_t parent,
> return;
> }
>
> +res = lo_inode_fd(dir, _fd);
> +if (res < 0) {
> +saverr = -res;
> +goto out;
> +}
> +
> saverr = lo_change_cred(req, , lo->change_umask && !S_ISLNK(mode));
> if (saverr) {
> goto out;
> }
>
> -res = mknod_wrapper(dir->fd, name, link, mode, rdev);
> +res = mknod_wrapper(dir_fd.fd, name, link, mode, rdev);
>
> saverr = errno;
>
> @@ -1388,6 +1418,8 @@ static void lo_symlink(fuse_req_t req, const char
> *link, fuse_ino_t parent,
> static void lo_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent,
> const char *name)
> {
> +g_auto(TempFd) inode_fd = TEMP_FD_INIT;
> +g_auto(TempFd) parent_fd = TEMP_FD_INIT;
> int res;
> struct lo_data *lo = lo_data(req);
> struct lo_inode *parent_inode;
> @@ -1413,18 +1445,31 @@ static void lo_link(fuse_req_t req, fuse_ino_t ino,
> fuse_ino_t parent,
> goto out_err;
> }
>
> +res = lo_inode_fd(inode, _fd);
> +if (res < 0) {
> +errno = -res;
In previous function, we saved error to "saverr" and jumped to "out"
label, instead of overwriting to errno.
I would think that it will be good to use a single pattern. Either
save error in saverr or overwrite errno. I personally prefer saving
error into "saverr".
> +goto out_err;
> +}
> +
> +res = lo_inode_fd(parent_inode, _fd);
> +if (res < 0) {
> +errno = -res;
> +goto out_err;
> +}
> +
> memset(, 0, sizeof(struct fuse_entry_param));
> e.attr_timeout = lo->timeout;
> e.entry_timeout = lo->timeout;
>
> -sprintf(procname, "%i", inode->fd);
> -res = linkat(lo->proc_self_fd, procname, parent_inode->fd, name,
> +sprintf(procname, "%i", inode_fd.fd);
> +res = linkat(lo->proc_self_fd, procname, parent_fd.fd, name,
> AT_SYMLINK_FOLLOW);
> if (res == -1) {
> goto out_err;
> }
>
> -res = fstatat(inode->fd, "", , AT_EMPTY_PATH |
> AT_SYMLINK_NOFOLLOW);
> +res = fstatat(inode_fd.fd, "", ,
> + AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
> if (res == -1) {
> goto out_err;
> }
> @@ -1453,23 +1498,33 @@ out_err:
> static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
> const char *name)
> {
> +g_auto(TempFd) dir_fd = TEMP_FD_INIT;
> int res;
> uint64_t mnt_id;
> struct stat attr;
> struct lo_data *lo = lo_data(req);
> struct lo_inode *dir = lo_inode(req, parent);
> +struct lo_inode *inode = NULL;
>
> if (!dir) {
> -return NULL;
> +goto out;
Should we continue to just call "return NULL". dir is NULL. That means
lo_inode() failed. That means we never got the reference. So we don't
have to put the reference. If we do "goto out", it will call
lo_inode_put() which is not needed.
> }
>
> -res = do_statx(lo, dir->fd, name, , AT_SYMLINK_NOFOLLOW, _id);
> -lo_inode_put(lo, );
> +res = lo_inode_fd(dir, _fd);
> +if (res < 0) {
> +goto out;
> +}
> +
> +res = do_statx(lo, dir_fd.fd, name, , AT_SYMLINK_NOFOLLOW, _id);
> if (res == -1) {
> -return NULL;
> +goto out;
> }
>
> -return lo_find(lo, , mnt_id);
> +inode = lo_find(lo, , mnt_id);
> +
> +out:
> +lo_inode_put(lo, );
> +return inode;
> }
Thanks
Vivek
[PATCH v3 04/10] virtiofsd: Add lo_inode_fd() helper
Once we let lo_inode.fd be optional, we will need its users to open the
file handle stored in lo_inode instead. This function will do that.
For now, it just returns lo_inode.fd, though.
Signed-off-by: Max Reitz
---
tools/virtiofsd/passthrough_ll.c | 150 +--
1 file changed, 125 insertions(+), 25 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index a444c3a7e2..86b901cf19 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -635,6 +635,16 @@ static struct lo_inode *lo_inode(fuse_req_t req,
fuse_ino_t ino)
return elem->inode;
}
+static int lo_inode_fd(const struct lo_inode *inode, TempFd *tfd)
+{
+*tfd = (TempFd) {
+.fd = inode->fd,
+.owned = false,
+};
+
+return 0;
+}
+
/*
* TODO Remove this helper and force callers to hold an inode refcount until
* they are done with the fd. This will be done in a later patch to make
@@ -822,11 +832,11 @@ static int lo_fi_fd(fuse_req_t req, struct fuse_file_info
*fi)
static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
int valid, struct fuse_file_info *fi)
{
+g_auto(TempFd) inode_fd = TEMP_FD_INIT;
int saverr;
char procname[64];
struct lo_data *lo = lo_data(req);
struct lo_inode *inode;
-int ifd;
int res;
int fd = -1;
@@ -836,7 +846,11 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino,
struct stat *attr,
return;
}
-ifd = inode->fd;
+res = lo_inode_fd(inode, _fd);
+if (res < 0) {
+saverr = -res;
+goto out_err;
+}
/* If fi->fh is invalid we'll report EBADF later */
if (fi) {
@@ -847,7 +861,7 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino,
struct stat *attr,
if (fi) {
res = fchmod(fd, attr->st_mode);
} else {
-sprintf(procname, "%i", ifd);
+sprintf(procname, "%i", inode_fd.fd);
res = fchmodat(lo->proc_self_fd, procname, attr->st_mode, 0);
}
if (res == -1) {
@@ -859,12 +873,13 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino,
struct stat *attr,
uid_t uid = (valid & FUSE_SET_ATTR_UID) ? attr->st_uid : (uid_t)-1;
gid_t gid = (valid & FUSE_SET_ATTR_GID) ? attr->st_gid : (gid_t)-1;
-saverr = drop_security_capability(lo, ifd);
+saverr = drop_security_capability(lo, inode_fd.fd);
if (saverr) {
goto out_err;
}
-res = fchownat(ifd, "", uid, gid, AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
+res = fchownat(inode_fd.fd, "", uid, gid,
+ AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
if (res == -1) {
saverr = errno;
goto out_err;
@@ -943,7 +958,7 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino,
struct stat *attr,
if (fi) {
res = futimens(fd, tv);
} else {
-sprintf(procname, "%i", inode->fd);
+sprintf(procname, "%i", inode_fd.fd);
res = utimensat(lo->proc_self_fd, procname, tv, 0);
}
if (res == -1) {
@@ -1058,7 +1073,8 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t
parent, const char *name,
struct fuse_entry_param *e,
struct lo_inode **inodep)
{
-int newfd;
+g_auto(TempFd) dir_fd = TEMP_FD_INIT;
+int newfd = -1;
int res;
int saverr;
uint64_t mnt_id;
@@ -1088,7 +1104,13 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t
parent, const char *name,
name = ".";
}
-newfd = openat(dir->fd, name, O_PATH | O_NOFOLLOW);
+res = lo_inode_fd(dir, _fd);
+if (res < 0) {
+saverr = -res;
+goto out;
+}
+
+newfd = openat(dir_fd.fd, name, O_PATH | O_NOFOLLOW);
if (newfd == -1) {
goto out_err;
}
@@ -1155,6 +1177,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t
parent, const char *name,
out_err:
saverr = errno;
+out:
if (newfd != -1) {
close(newfd);
}
@@ -1312,6 +1335,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t
parent,
const char *name, mode_t mode, dev_t rdev,
const char *link)
{
+g_auto(TempFd) dir_fd = TEMP_FD_INIT;
int res;
int saverr;
struct lo_data *lo = lo_data(req);
@@ -1335,12 +1359,18 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t
parent,
return;
}
+res = lo_inode_fd(dir, _fd);
+if (res < 0) {
+saverr = -res;
+goto out;
+}
+
saverr = lo_change_cred(req, , lo->change_umask && !S_ISLNK(mode));
if (saverr) {
goto out;
}
-res = mknod_wrapper(dir->fd, name, link, mode, rdev);
+res = mknod_wrapper(dir_fd.fd, name, link, mode, rdev);
saverr = errno;
@@ -1388,6 +1418,8 @@ static void
