Re: [Qemu-devel] [PATCH 03/13] 9p: Move a couple xattr functions to 9p-util

[Keno Fischer]

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

Re: [Qemu-devel] [PATCH 03/13] 9p: Move a couple xattr functions to 9p-util

[Greg Kurz]

On Thu, 31 May 2018 12:14:30 -0400
Keno Fischer  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

Re: [Qemu-devel] [PATCH 03/13] 9p: Move a couple xattr functions to 9p-util

[Keno Fischer]

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

Re: [Qemu-devel] [PATCH 03/13] 9p: Move a couple xattr functions to 9p-util

[Greg Kurz]

On Sat, 26 May 2018 01:23:05 -0400
k...@juliacomputing.com wrote:

> From: Keno Fischer 
> 
> 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 
> ---
>  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", _uid, sizeof(uid_t)) > 0) {
> +if (fgetxattr_follow(fd, "user.virtfs.uid", _uid, sizeof(uid_t)) 
> > 0) {
>  stbuf->st_uid = le32_to_cpu(tmp_uid);
>  }
> -if (fgetxattr(fd, "user.virtfs.gid", _gid, sizeof(gid_t)) > 0) {
> +if (fgetxattr_follow(fd, "user.virtfs.gid", _gid, sizeof(gid_t)) 
> > 0)
> +{
>  stbuf->st_gid = le32_to_cpu(tmp_gid);
>  }
> -if (fgetxattr(fd, "user.virtfs.mode", _mode, sizeof(mode_t)) > 
> 0) {
> +if (fgetxattr_follow(fd, "user.virtfs.mode", _mode, 
> sizeof(mode_t)) > 0)
> +{
>  stbuf->st_mode = le32_to_cpu(tmp_mode);
>  }
> -if (fgetxattr(fd, "user.virtfs.rdev", _dev, sizeof(dev_t)) > 0) {
> +if (fgetxattr_follow(fd, "user.virtfs.rdev", _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 

[Qemu-devel] [PATCH 03/13] 9p: Move a couple xattr functions to 9p-util

[keno]

From: Keno Fischer 

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.

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.

Signed-off-by: Keno Fischer 
---
 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", _uid, sizeof(uid_t)) > 0) {
+if (fgetxattr_follow(fd, "user.virtfs.uid", _uid, sizeof(uid_t)) > 
0) {
 stbuf->st_uid = le32_to_cpu(tmp_uid);
 }
-if (fgetxattr(fd, "user.virtfs.gid", _gid, sizeof(gid_t)) > 0) {
+if (fgetxattr_follow(fd, "user.virtfs.gid", _gid, sizeof(gid_t)) > 
0)
+{
 stbuf->st_gid = le32_to_cpu(tmp_gid);
 }
-if (fgetxattr(fd, "user.virtfs.mode", _mode, sizeof(mode_t)) > 0) {
+if (fgetxattr_follow(fd, "user.virtfs.mode", _mode, 
sizeof(mode_t)) > 0)
+{
 stbuf->st_mode = le32_to_cpu(tmp_mode);
 }
-if (fgetxattr(fd, "user.virtfs.rdev", _dev, sizeof(dev_t)) > 0) {
+if (fgetxattr_follow(fd, "user.virtfs.rdev", _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,