| Message ID | 033ba6f2e3f0daccb31e1930a1c65cf7a18ae66c.1527310210.git.keno@alumni.harvard.edu |
|---|---|
| State | New |
| Headers | show |
| Series | 9p: Add support for Darwin | expand |
On Sat, 26 May 2018 01:23:05 -0400 keno@juliacomputing.com wrote: > From: Keno Fischer <keno@alumni.harvard.edu> > > These functions will need custom implementations on Darwin. Since the > implementation is very similar among all of them, and 9p-util already > has the _nofollow version of fgetxattrat, let's move them all there. > I'm ok with this move, but if the functions need to have distinct implementations, and they really do according to patch 10, then I'd rather have distinct files and rely on conditional building in the makefile. Maybe rename the current file to 9p-util-linux.c and introduce a 9p-util-darwin.c later. > Additionally, introduce a _follow version of fgetxattr and use it. > On darwin, fgetxattr has a more general interface, so we'll need to factor > it out anyway. > No need for the _follow version in this case. > Signed-off-by: Keno Fischer <keno@juliacomputing.com> > --- > hw/9pfs/9p-local.c | 11 +++++++---- > hw/9pfs/9p-util.c | 39 +++++++++++++++++++++++++++++++++++++++ > hw/9pfs/9p-util.h | 6 ++++++ > hw/9pfs/9p-xattr.c | 33 --------------------------------- > 4 files changed, 52 insertions(+), 37 deletions(-) > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > index 7592f8d..fd65d04 100644 > --- a/hw/9pfs/9p-local.c > +++ b/hw/9pfs/9p-local.c > @@ -776,16 +776,19 @@ static int local_fstat(FsContext *fs_ctx, int fid_type, > mode_t tmp_mode; > dev_t tmp_dev; > > - if (fgetxattr(fd, "user.virtfs.uid", &tmp_uid, sizeof(uid_t)) > 0) { > + if (fgetxattr_follow(fd, "user.virtfs.uid", &tmp_uid, sizeof(uid_t)) > 0) { > stbuf->st_uid = le32_to_cpu(tmp_uid); > } > - if (fgetxattr(fd, "user.virtfs.gid", &tmp_gid, sizeof(gid_t)) > 0) { > + if (fgetxattr_follow(fd, "user.virtfs.gid", &tmp_gid, sizeof(gid_t)) > 0) > + { > stbuf->st_gid = le32_to_cpu(tmp_gid); > } > - if (fgetxattr(fd, "user.virtfs.mode", &tmp_mode, sizeof(mode_t)) > 0) { > + if (fgetxattr_follow(fd, "user.virtfs.mode", &tmp_mode, sizeof(mode_t)) > 0) > + { > stbuf->st_mode = le32_to_cpu(tmp_mode); > } > - if (fgetxattr(fd, "user.virtfs.rdev", &tmp_dev, sizeof(dev_t)) > 0) { > + if (fgetxattr_follow(fd, "user.virtfs.rdev", &tmp_dev, sizeof(dev_t)) > 0) > + { > stbuf->st_rdev = le64_to_cpu(tmp_dev); > } > } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) { > diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util.c > index f709c27..8cf5554 100644 > --- a/hw/9pfs/9p-util.c > +++ b/hw/9pfs/9p-util.c > @@ -24,3 +24,42 @@ ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name, > g_free(proc_path); > return ret; > } > + > +ssize_t fgetxattr_follow(int fd, const char *name, > + void *value, size_t size) > +{ > + return fgetxattr(fd, name, value, size); > +} > + > +ssize_t flistxattrat_nofollow(int dirfd, const char *filename, > + char *list, size_t size) > +{ > + char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename); > + int ret; > + > + ret = llistxattr(proc_path, list, size); > + g_free(proc_path); > + return ret; > +} > + > +ssize_t fremovexattrat_nofollow(int dirfd, const char *filename, > + const char *name) > +{ > + char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename); > + int ret; > + > + ret = lremovexattr(proc_path, name); > + g_free(proc_path); > + return ret; > +} > + > +int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name, > + void *value, size_t size, int flags) > +{ > + char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename); > + int ret; > + > + ret = lsetxattr(proc_path, name, value, size, flags); > + g_free(proc_path); > + return ret; > +} > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h > index dc0d2e2..cb26343 100644 > --- a/hw/9pfs/9p-util.h > +++ b/hw/9pfs/9p-util.h > @@ -58,7 +58,13 @@ static inline int openat_file(int dirfd, const char *name, int flags, > > ssize_t fgetxattrat_nofollow(int dirfd, const char *path, const char *name, > void *value, size_t size); > +ssize_t fgetxattr_follow(int fd, const char *name, > + void *value, size_t size); > int fsetxattrat_nofollow(int dirfd, const char *path, const char *name, > void *value, size_t size, int flags); > +ssize_t flistxattrat_nofollow(int dirfd, const char *filename, > + char *list, size_t size); > +ssize_t fremovexattrat_nofollow(int dirfd, const char *filename, > + const char *name); > > #endif > diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c > index d05c1a1..c696d8f 100644 > --- a/hw/9pfs/9p-xattr.c > +++ b/hw/9pfs/9p-xattr.c > @@ -60,17 +60,6 @@ ssize_t pt_listxattr(FsContext *ctx, const char *path, > return name_size; > } > > -static ssize_t flistxattrat_nofollow(int dirfd, const char *filename, > - char *list, size_t size) > -{ > - char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename); > - int ret; > - > - ret = llistxattr(proc_path, list, size); > - g_free(proc_path); > - return ret; > -} > - > /* > * Get the list and pass to each layer to find out whether > * to send the data or not > @@ -196,17 +185,6 @@ ssize_t pt_getxattr(FsContext *ctx, const char *path, const char *name, > return local_getxattr_nofollow(ctx, path, name, value, size); > } > > -int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name, > - void *value, size_t size, int flags) > -{ > - char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename); > - int ret; > - > - ret = lsetxattr(proc_path, name, value, size, flags); > - g_free(proc_path); > - return ret; > -} > - > ssize_t local_setxattr_nofollow(FsContext *ctx, const char *path, > const char *name, void *value, size_t size, > int flags) > @@ -235,17 +213,6 @@ int pt_setxattr(FsContext *ctx, const char *path, const char *name, void *value, > return local_setxattr_nofollow(ctx, path, name, value, size, flags); > } > > -static ssize_t fremovexattrat_nofollow(int dirfd, const char *filename, > - const char *name) > -{ > - char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename); > - int ret; > - > - ret = lremovexattr(proc_path, name); > - g_free(proc_path); > - return ret; > -} > - > ssize_t local_removexattr_nofollow(FsContext *ctx, const char *path, > const char *name) > {
> I'm ok with this move, but if the functions need to have distinct > implementations, and they really do according to patch 10, then > I'd rather have distinct files and rely on conditional building in > the makefile. Maybe rename the current file to 9p-util-linux.c > and introduce a 9p-util-darwin.c later. Sounds good, I will make this change. >> Additionally, introduce a _follow version of fgetxattr and use it. >> On darwin, fgetxattr has a more general interface, so we'll need to factor >> it out anyway. >> > > No need for the _follow version in this case. I'm not entirely sure what you're proposing. On darwin `fgetxattr` takes two extra arguments, so I believe it needs to be factored out nevertheless. Are you just proposing doing that later in the patch series?
On Thu, 31 May 2018 12:14:30 -0400 Keno Fischer <keno@juliacomputing.com> wrote: > > I'm ok with this move, but if the functions need to have distinct > > implementations, and they really do according to patch 10, then > > I'd rather have distinct files and rely on conditional building in > > the makefile. Maybe rename the current file to 9p-util-linux.c > > and introduce a 9p-util-darwin.c later. > > Sounds good, I will make this change. > > >> Additionally, introduce a _follow version of fgetxattr and use it. > >> On darwin, fgetxattr has a more general interface, so we'll need to factor > >> it out anyway. > >> > > > > No need for the _follow version in this case. > > I'm not entirely sure what you're proposing. On darwin `fgetxattr` > takes two extra > arguments, Oops you're right... so we indeed need to handle this conflicting APIs, but fgetxattr_follow() is inapropriate, because fgetxattr() has nothing to follow since it already has an fd... The same goes with Darwin's version actually. The "nofollow" thingy only makes sense for those calls that have a dirfd and pathname argument. The OSX manpage for fgetxattr() seem to mention the XATTR_NOFOLLOW flag for getxattr() only. https://www.unix.com/man-page/osx/2/fgetxattr/ XATTR_NOFOLLOW do not follow symbolic links. getxattr() normally returns information from the target of path if it is a symbolic link. With this option, getxattr() will return extended attribute data from the symbolic link instead. > so I believe it needs to be factored out nevertheless. Are you just > proposing doing that later in the patch series? Maybe introduce a qemu_fgetxattr() API in 9p-utils.h ? In a separate patch, or maybe in patch 10. #ifdef CONFIG_DARWIN #define qemu_fgetxattr(...) fgetxattr(__VA_ARGS__, 0, 0) #else #define qemu_fgetxattr fgetxattr #endif
> Oops you're right... so we indeed need to handle this conflicting APIs, > but fgetxattr_follow() is inapropriate, because fgetxattr() has nothing > to follow since it already has an fd... The same goes with Darwin's > version actually. The "nofollow" thingy only makes sense for those calls > that have a dirfd and pathname argument. > > The OSX manpage for fgetxattr() seem to mention the XATTR_NOFOLLOW flag > for getxattr() only. > > https://www.unix.com/man-page/osx/2/fgetxattr/ > > XATTR_NOFOLLOW do not follow symbolic links. getxattr() normally returns > information from the target of path if it is a symbolic link. > With this option, getxattr() will return extended attribute > data from the symbolic link instead. Ah sorry, you're correct of course. Will fix. >> so I believe it needs to be factored out nevertheless. Are you just >> proposing doing that later in the patch series? > > Maybe introduce a qemu_fgetxattr() API in 9p-utils.h ? In a separate patch, or > maybe in patch 10. > > #ifdef CONFIG_DARWIN > #define qemu_fgetxattr(...) fgetxattr(__VA_ARGS__, 0, 0) > #else > #define qemu_fgetxattr fgetxattr > #endif > Sounds good. I'll do this in a separate patch, since the *at versions are all similar while this is a bit different.
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index 7592f8d..fd65d04 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -776,16 +776,19 @@ static int local_fstat(FsContext *fs_ctx, int fid_type, mode_t tmp_mode; dev_t tmp_dev; - if (fgetxattr(fd, "user.virtfs.uid", &tmp_uid, sizeof(uid_t)) > 0) { + if (fgetxattr_follow(fd, "user.virtfs.uid", &tmp_uid, sizeof(uid_t)) > 0) { stbuf->st_uid = le32_to_cpu(tmp_uid); } - if (fgetxattr(fd, "user.virtfs.gid", &tmp_gid, sizeof(gid_t)) > 0) { + if (fgetxattr_follow(fd, "user.virtfs.gid", &tmp_gid, sizeof(gid_t)) > 0) + { stbuf->st_gid = le32_to_cpu(tmp_gid); } - if (fgetxattr(fd, "user.virtfs.mode", &tmp_mode, sizeof(mode_t)) > 0) { + if (fgetxattr_follow(fd, "user.virtfs.mode", &tmp_mode, sizeof(mode_t)) > 0) + { stbuf->st_mode = le32_to_cpu(tmp_mode); } - if (fgetxattr(fd, "user.virtfs.rdev", &tmp_dev, sizeof(dev_t)) > 0) { + if (fgetxattr_follow(fd, "user.virtfs.rdev", &tmp_dev, sizeof(dev_t)) > 0) + { stbuf->st_rdev = le64_to_cpu(tmp_dev); } } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) { diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util.c index f709c27..8cf5554 100644 --- a/hw/9pfs/9p-util.c +++ b/hw/9pfs/9p-util.c @@ -24,3 +24,42 @@ ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name, g_free(proc_path); return ret; } + +ssize_t fgetxattr_follow(int fd, const char *name, + void *value, size_t size) +{ + return fgetxattr(fd, name, value, size); +} + +ssize_t flistxattrat_nofollow(int dirfd, const char *filename, + char *list, size_t size) +{ + char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename); + int ret; + + ret = llistxattr(proc_path, list, size); + g_free(proc_path); + return ret; +} + +ssize_t fremovexattrat_nofollow(int dirfd, const char *filename, + const char *name) +{ + char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename); + int ret; + + ret = lremovexattr(proc_path, name); + g_free(proc_path); + return ret; +} + +int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name, + void *value, size_t size, int flags) +{ + char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename); + int ret; + + ret = lsetxattr(proc_path, name, value, size, flags); + g_free(proc_path); + return ret; +} diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h index dc0d2e2..cb26343 100644 --- a/hw/9pfs/9p-util.h +++ b/hw/9pfs/9p-util.h @@ -58,7 +58,13 @@ static inline int openat_file(int dirfd, const char *name, int flags, ssize_t fgetxattrat_nofollow(int dirfd, const char *path, const char *name, void *value, size_t size); +ssize_t fgetxattr_follow(int fd, const char *name, + void *value, size_t size); int fsetxattrat_nofollow(int dirfd, const char *path, const char *name, void *value, size_t size, int flags); +ssize_t flistxattrat_nofollow(int dirfd, const char *filename, + char *list, size_t size); +ssize_t fremovexattrat_nofollow(int dirfd, const char *filename, + const char *name); #endif diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c index d05c1a1..c696d8f 100644 --- a/hw/9pfs/9p-xattr.c +++ b/hw/9pfs/9p-xattr.c @@ -60,17 +60,6 @@ ssize_t pt_listxattr(FsContext *ctx, const char *path, return name_size; } -static ssize_t flistxattrat_nofollow(int dirfd, const char *filename, - char *list, size_t size) -{ - char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename); - int ret; - - ret = llistxattr(proc_path, list, size); - g_free(proc_path); - return ret; -} - /* * Get the list and pass to each layer to find out whether * to send the data or not @@ -196,17 +185,6 @@ ssize_t pt_getxattr(FsContext *ctx, const char *path, const char *name, return local_getxattr_nofollow(ctx, path, name, value, size); } -int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name, - void *value, size_t size, int flags) -{ - char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename); - int ret; - - ret = lsetxattr(proc_path, name, value, size, flags); - g_free(proc_path); - return ret; -} - ssize_t local_setxattr_nofollow(FsContext *ctx, const char *path, const char *name, void *value, size_t size, int flags) @@ -235,17 +213,6 @@ int pt_setxattr(FsContext *ctx, const char *path, const char *name, void *value, return local_setxattr_nofollow(ctx, path, name, value, size, flags); } -static ssize_t fremovexattrat_nofollow(int dirfd, const char *filename, - const char *name) -{ - char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename); - int ret; - - ret = lremovexattr(proc_path, name); - g_free(proc_path); - return ret; -} - ssize_t local_removexattr_nofollow(FsContext *ctx, const char *path, const char *name) {