On Donnerstag, 21. April 2022 14:26:37 CEST Greg Kurz wrote: > On Thu, 21 Apr 2022 12:55:24 +0200 > > Christian Schoenebeck <qemu_...@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_...@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_...@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