| Message ID | 284de6154d7ad57ac6539a7318aa2364261da105.1650370027.git.qemu_oss@crudebyte.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 9pfs: macOS host fixes | expand |
On Tue, 19 Apr 2022 13:43:30 +0200 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > When mapped POSIX ACL is used, we are ignoring errors when trying > to remove a POSIX ACL xattr that does not exist. On Linux hosts we > would get ENODATA in such cases, on macOS hosts however we get > ENOATTR instead, so ignore ENOATTR errors as well. > > This patch fixes e.g. a command on Linux guest like: > > cp --preserve=mode old new > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > --- > hw/9pfs/9p-posix-acl.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/9pfs/9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c > index eadae270dd..2bf155f941 100644 > --- a/hw/9pfs/9p-posix-acl.c > +++ b/hw/9pfs/9p-posix-acl.c > @@ -65,7 +65,13 @@ static int mp_pacl_removexattr(FsContext *ctx, > int ret; > > ret = local_removexattr_nofollow(ctx, path, MAP_ACL_ACCESS); > - if (ret == -1 && errno == ENODATA) { > + if (ret == -1 && > + (errno == ENODATA > +#ifdef ENOATTR > + || errno == ENOATTR > +#endif > + ) We already have this in <qemu/xattr.h> which is included by 9p-posix-acl.c : /* * Modern distributions (e.g. Fedora 15), have no libattr.so, place attr.h * in /usr/include/sys, and don't have ENOATTR. */ #ifdef CONFIG_LIBATTR # include <attr/xattr.h> #else # if !defined(ENOATTR) # define ENOATTR ENODATA # endif # include <sys/xattr.h> #endif I guess this patch could just s/ENODATA/ENOATTR/ to avoid the extra ifdefery. > + ) { > /* > * We don't get ENODATA error when trying to remove a > * posix acl that is not present. So don't throw the error
On Donnerstag, 21. April 2022 10:26:11 CEST Greg Kurz wrote: > On Tue, 19 Apr 2022 13:43:30 +0200 > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > When mapped POSIX ACL is used, we are ignoring errors when trying > > to remove a POSIX ACL xattr that does not exist. On Linux hosts we > > would get ENODATA in such cases, on macOS hosts however we get > > ENOATTR instead, so ignore ENOATTR errors as well. > > > > This patch fixes e.g. a command on Linux guest like: > > cp --preserve=mode old new > > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > > --- > > > > hw/9pfs/9p-posix-acl.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/hw/9pfs/9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c > > index eadae270dd..2bf155f941 100644 > > --- a/hw/9pfs/9p-posix-acl.c > > +++ b/hw/9pfs/9p-posix-acl.c > > @@ -65,7 +65,13 @@ static int mp_pacl_removexattr(FsContext *ctx, > > > > int ret; > > > > ret = local_removexattr_nofollow(ctx, path, MAP_ACL_ACCESS); > > > > - if (ret == -1 && errno == ENODATA) { > > + if (ret == -1 && > > + (errno == ENODATA > > +#ifdef ENOATTR > > + || errno == ENOATTR > > +#endif > > + ) > > We already have this in <qemu/xattr.h> which is included by > 9p-posix-acl.c : > > /* > * Modern distributions (e.g. Fedora 15), have no libattr.so, place attr.h > * in /usr/include/sys, and don't have ENOATTR. > */ > > > #ifdef CONFIG_LIBATTR > # include <attr/xattr.h> > #else > # if !defined(ENOATTR) > # define ENOATTR ENODATA > # endif > # include <sys/xattr.h> > #endif > > I guess this patch could just s/ENODATA/ENOATTR/ to avoid the > extra ifdefery. Not viable, because macOS does have both ENODATA==96 and ENOATTR==93. On Linux the two macros were historically defined to the same numeric values, that's why it worked there. Maybe I should define a separate macro like: #if ... # define P9_ENOATTR ENOATTR #else # define P9_ENOATTR ENODATA #end ? Actually good that you pointed me at this, because I just realized there is a 2nd place in 9p-posix-acl.c which would require this as well. For some reason the 2nd place just did not trigger while I was testing it on macOS. Best regards, Christian Schoenebeck
On Thu, 21 Apr 2022 12:55:24 +0200 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > On Donnerstag, 21. April 2022 10:26:11 CEST Greg Kurz wrote: > > On Tue, 19 Apr 2022 13:43:30 +0200 > > > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > > When mapped POSIX ACL is used, we are ignoring errors when trying > > > to remove a POSIX ACL xattr that does not exist. On Linux hosts we > > > would get ENODATA in such cases, on macOS hosts however we get > > > ENOATTR instead, so ignore ENOATTR errors as well. > > > > > > This patch fixes e.g. a command on Linux guest like: > > > cp --preserve=mode old new > > > > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > > > --- > > > > > > hw/9pfs/9p-posix-acl.c | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/9pfs/9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c > > > index eadae270dd..2bf155f941 100644 > > > --- a/hw/9pfs/9p-posix-acl.c > > > +++ b/hw/9pfs/9p-posix-acl.c > > > @@ -65,7 +65,13 @@ static int mp_pacl_removexattr(FsContext *ctx, > > > > > > int ret; > > > > > > ret = local_removexattr_nofollow(ctx, path, MAP_ACL_ACCESS); > > > > > > - if (ret == -1 && errno == ENODATA) { > > > + if (ret == -1 && > > > + (errno == ENODATA > > > +#ifdef ENOATTR > > > + || errno == ENOATTR > > > +#endif > > > + ) > > > > We already have this in <qemu/xattr.h> which is included by > > 9p-posix-acl.c : > > > > /* > > * Modern distributions (e.g. Fedora 15), have no libattr.so, place attr.h > > * in /usr/include/sys, and don't have ENOATTR. > > */ > > > > > > #ifdef CONFIG_LIBATTR > > # include <attr/xattr.h> > > #else > > # if !defined(ENOATTR) > > # define ENOATTR ENODATA > > # endif > > # include <sys/xattr.h> > > #endif > > > > I guess this patch could just s/ENODATA/ENOATTR/ to avoid the > > extra ifdefery. > > Not viable, because macOS does have both ENODATA==96 and ENOATTR==93. On Linux > the two macros were historically defined to the same numeric values, that's > why it worked there. > I was meaning your current fix could simply do: - if (ret == -1 && errno == ENODATA) { + if (ret == -1 && errno == ENOATTR) { since ENOATTR works in all cases, but this is rather a hack. Another solution would be to ensure that local_removexattr_nofollow() only reports linux errnos. This could be handled cleanly in the fremovexattrat_nofollow() implementation in 9p-util-darwin.c. Since the 9p code base mostly assumes the host is linux, this should probably be generalized to other places where we check errno. > Maybe I should define a separate macro like: > > #if ... > # define P9_ENOATTR ENOATTR > #else > # define P9_ENOATTR ENODATA > #end > > ? > > Actually good that you pointed me at this, because I just realized there is a > 2nd place in 9p-posix-acl.c which would require this as well. For some reason > the 2nd place just did not trigger while I was testing it on macOS. > > Best regards, > Christian Schoenebeck > >
On Donnerstag, 21. April 2022 14:26:37 CEST Greg Kurz wrote: > On Thu, 21 Apr 2022 12:55:24 +0200 > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > On Donnerstag, 21. April 2022 10:26:11 CEST Greg Kurz wrote: > > > On Tue, 19 Apr 2022 13:43:30 +0200 > > > > > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > > > When mapped POSIX ACL is used, we are ignoring errors when trying > > > > to remove a POSIX ACL xattr that does not exist. On Linux hosts we > > > > would get ENODATA in such cases, on macOS hosts however we get > > > > ENOATTR instead, so ignore ENOATTR errors as well. > > > > > > > > This patch fixes e.g. a command on Linux guest like: > > > > cp --preserve=mode old new > > > > > > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > > > > --- > > > > > > > > hw/9pfs/9p-posix-acl.c | 8 +++++++- > > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/hw/9pfs/9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c > > > > index eadae270dd..2bf155f941 100644 > > > > --- a/hw/9pfs/9p-posix-acl.c > > > > +++ b/hw/9pfs/9p-posix-acl.c > > > > @@ -65,7 +65,13 @@ static int mp_pacl_removexattr(FsContext *ctx, > > > > > > > > int ret; > > > > > > > > ret = local_removexattr_nofollow(ctx, path, MAP_ACL_ACCESS); > > > > > > > > - if (ret == -1 && errno == ENODATA) { > > > > + if (ret == -1 && > > > > + (errno == ENODATA > > > > +#ifdef ENOATTR > > > > + || errno == ENOATTR > > > > +#endif > > > > + ) > > > > > > We already have this in <qemu/xattr.h> which is included by > > > 9p-posix-acl.c : > > > > > > /* > > > > > > * Modern distributions (e.g. Fedora 15), have no libattr.so, place > > > attr.h > > > * in /usr/include/sys, and don't have ENOATTR. > > > */ > > > > > > #ifdef CONFIG_LIBATTR > > > # include <attr/xattr.h> > > > #else > > > # if !defined(ENOATTR) > > > # define ENOATTR ENODATA > > > # endif > > > # include <sys/xattr.h> > > > #endif > > > > > > I guess this patch could just s/ENODATA/ENOATTR/ to avoid the > > > extra ifdefery. > > > > Not viable, because macOS does have both ENODATA==96 and ENOATTR==93. On > > Linux the two macros were historically defined to the same numeric > > values, that's why it worked there. > > I was meaning your current fix could simply do: > > - if (ret == -1 && errno == ENODATA) { > + if (ret == -1 && errno == ENOATTR) { > > since ENOATTR works in all cases, but this is rather a hack. > > Another solution would be to ensure that local_removexattr_nofollow() only > reports linux errnos. This could be handled cleanly in the > fremovexattrat_nofollow() implementation in 9p-util-darwin.c. > > Since the 9p code base mostly assumes the host is linux, this should > probably be generalized to other places where we check errno. Got it. I tend to go with the former (checking errno == ENOATTR and defining ENOATTR if non existent). I find that a bit cleaner than the latter which would have the potential to mask another error (ENODATA). > > Maybe I should define a separate macro like: > > > > #if ... > > # define P9_ENOATTR ENOATTR > > #else > > # define P9_ENOATTR ENODATA > > #end > > > > ? > > > > Actually good that you pointed me at this, because I just realized there > > is a 2nd place in 9p-posix-acl.c which would require this as well. For > > some reason the 2nd place just did not trigger while I was testing it on > > macOS. > > > > Best regards, > > Christian Schoenebeck
diff --git a/hw/9pfs/9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c index eadae270dd..2bf155f941 100644 --- a/hw/9pfs/9p-posix-acl.c +++ b/hw/9pfs/9p-posix-acl.c @@ -65,7 +65,13 @@ static int mp_pacl_removexattr(FsContext *ctx, int ret; ret = local_removexattr_nofollow(ctx, path, MAP_ACL_ACCESS); - if (ret == -1 && errno == ENODATA) { + if (ret == -1 && + (errno == ENODATA +#ifdef ENOATTR + || errno == ENOATTR +#endif + ) + ) { /* * We don't get ENODATA error when trying to remove a * posix acl that is not present. So don't throw the error
When mapped POSIX ACL is used, we are ignoring errors when trying to remove a POSIX ACL xattr that does not exist. On Linux hosts we would get ENODATA in such cases, on macOS hosts however we get ENOATTR instead, so ignore ENOATTR errors as well. This patch fixes e.g. a command on Linux guest like: cp --preserve=mode old new Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> --- hw/9pfs/9p-posix-acl.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)