Re: [PATCH v9 09/11] 9p: darwin: Implement compatibility for mknodat

[Greg Kurz]

On Mon, 28 Feb 2022 08:37:10 -0500
Will Cohen <wwcohen@gmail.com> wrote:

> On Mon, Feb 28, 2022 at 8:20 AM Christian Schoenebeck <
> qemu_oss@crudebyte.com> wrote:
> 
> > On Sonntag, 27. Februar 2022 23:35:20 CET Will Cohen wrote:
> > > From: Keno Fischer <keno@juliacomputing.com>
> > >
> > > Darwin does not support mknodat. However, to avoid race conditions
> > > with later setting the permissions, we must avoid using mknod on
> > > the full path instead. We could try to fchdir, but that would cause
> > > problems if multiple threads try to call mknodat at the same time.
> > > However, luckily there is a solution: Darwin includes a function
> > > that sets the cwd for the current thread only.
> > > This should suffice to use mknod safely.
> > >
> > > This function (pthread_fchdir_np) is protected by a check in
> > > meson in a patch later in this series.
> > >
> > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > > [Will Cohen: - Adjust coding style
> > >              - Replace clang references with gcc
> > >              - Note radar filed with Apple for missing syscall
> > >              - Replace direct syscall with pthread_fchdir_np and
> > >                adjust patch notes accordingly
> > >              - Declare pthread_fchdir_np with
> > >              - __attribute__((weak_import)) to allow checking for
> > >                its presence before usage
> > >              - Move declarations above cplusplus guard
> > >              - Add CONFIG_PTHREAD_FCHDIR_NP to meson and check for
> > >                presence in 9p-util
> > >              - Rebase to apply cleanly on top of the 2022-02-10
> > >                changes to 9pfs
> > >              - Fix line over 90 characters formatting error]
> > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > ---
> > >  hw/9pfs/9p-local.c       |  4 ++--
> > >  hw/9pfs/9p-util-darwin.c | 33 +++++++++++++++++++++++++++++++++
> > >  hw/9pfs/9p-util-linux.c  |  6 ++++++
> > >  hw/9pfs/9p-util.h        | 11 +++++++++++
> > >  meson.build              |  1 +
> > >  5 files changed, 53 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > > index a0d08e5216..d42ce6d8b8 100644
> > > --- a/hw/9pfs/9p-local.c
> > > +++ b/hw/9pfs/9p-local.c
> > > @@ -682,7 +682,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath
> > > *dir_path,
> > >
> > >      if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
> > >          fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> > > -        err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
> > > +        err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
> > >          if (err == -1) {
> > >              goto out;
> > >          }
> > > @@ -697,7 +697,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath
> > > *dir_path, }
> > >      } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
> > >                 fs_ctx->export_flags & V9FS_SM_NONE) {
> > > -        err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
> > > +        err = qemu_mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
> > >          if (err == -1) {
> > >              goto out;
> > >          }
> > > diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
> > > index cdb4c9e24c..bec0253474 100644
> > > --- a/hw/9pfs/9p-util-darwin.c
> > > +++ b/hw/9pfs/9p-util-darwin.c
> > > @@ -7,6 +7,8 @@
> > >
> > >  #include "qemu/osdep.h"
> > >  #include "qemu/xattr.h"
> > > +#include "qapi/error.h"
> > > +#include "qemu/error-report.h"
> > >  #include "9p-util.h"
> > >
> > >  ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char
> > > *name, @@ -62,3 +64,34 @@ int fsetxattrat_nofollow(int dirfd, const char
> > > *filename, const char *name, close_preserve_errno(fd);
> > >      return ret;
> > >  }
> > > +
> > > +/*
> > > + * As long as mknodat is not available on macOS, this workaround
> > > + * using pthread_fchdir_np is needed.
> > > + *
> > > + * Radar filed with Apple for implementing mknodat:
> > > + * rdar://FB9862426 (https://openradar.appspot.com/FB9862426)
> > > + */
> > > +#if defined CONFIG_PTHREAD_FCHDIR_NP
> > > +
> > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> > dev)
> > > +{
> > > +    int preserved_errno, err;
> > > +    if (!pthread_fchdir_np) {
> > > +        error_report_once("pthread_fchdir_np() not available on this
> > version of macOS");
> >
> > You took the code style error message a bit too literal; this line is still
> > too long:
> >
> > WARNING: line over 80 characters
> > #199: FILE: hw/9pfs/9p-util-darwin.c:81:
> > +        error_report_once("pthread_fchdir_np() not available on this
> > version of macOS");
> >
> > total: 0 errors, 1 warnings, 91 lines checked
> >
> > However this is too trivial to send a v10 just for this. If there are no
> > other
> > issues in this v9 then I'll simply fix this on my end. My plan is to queue
> > this series tomorrow.
> >
> > Best regards,
> > Christian Schoenebeck
> 
> 
> Sorry for the over-literalness. I was just trying to avoid a need for
> further changes by being too dramatic on my end for a small fix! Any
> stylistic changes needed here are, of course, totally acceptable!
> 

As Thomas is saying this is just a warning and it is certainly
easier for a developer to grep the error message if it is kept
on a single line.

> >
> >
> > > +        return -ENOTSUP;
> > > +    }
> > > +    if (pthread_fchdir_np(dirfd) < 0) {
> > > +        return -1;
> > > +    }
> > > +    err = mknod(filename, mode, dev);
> > > +    preserved_errno = errno;
> > > +    /* Stop using the thread-local cwd */
> > > +    pthread_fchdir_np(-1);
> > > +    if (err < 0) {
> > > +        errno = preserved_errno;
> > > +    }
> > > +    return err;
> > > +}
> > > +
> > > +#endif
> > > diff --git a/hw/9pfs/9p-util-linux.c b/hw/9pfs/9p-util-linux.c
> > > index 398614a5d0..db451b0784 100644
> > > --- a/hw/9pfs/9p-util-linux.c
> > > +++ b/hw/9pfs/9p-util-linux.c
> > > @@ -61,4 +61,10 @@ int fsetxattrat_nofollow(int dirfd, const char
> > *filename,
> > > const char *name, ret = lsetxattr(proc_path, name, value, size, flags);
> > >      g_free(proc_path);
> > >      return ret;
> > > +
> > > +}
> > > +
> > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> > dev)
> > > +{
> > > +    return mknodat(dirfd, filename, mode, dev);
> > >  }
> > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
> > > index 9abff79884..1f74d37558 100644
> > > --- a/hw/9pfs/9p-util.h
> > > +++ b/hw/9pfs/9p-util.h
> > > @@ -112,5 +112,16 @@ static inline off_t qemu_dirent_off(struct dirent
> > > *dent) #endif
> > >  }
> > >
> > > +/*
> > > + * As long as mknodat is not available on macOS, this workaround
> > > + * using pthread_fchdir_np is needed. qemu_mknodat is defined in
> > > + * os-posix.c. pthread_fchdir_np is weakly linked here as a guard
> > > + * in case it disappears in future macOS versions, because it is
> > > + * is a private API.
> > > + */
> > > +#if defined CONFIG_DARWIN && defined CONFIG_PTHREAD_FCHDIR_NP
> > > +int pthread_fchdir_np(int fd) __attribute__((weak_import));
> > > +#endif
> > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> > dev);
> > >
> > >  #endif
> > > diff --git a/meson.build b/meson.build
> > > index 8df40bfac4..3f8dca2c7a 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -1609,6 +1609,7 @@ config_host_data.set('CONFIG_POSIX_FALLOCATE',
> > > cc.has_function('posix_fallocate'
> > > config_host_data.set('CONFIG_POSIX_MEMALIGN',
> > > cc.has_function('posix_memalign')) config_host_data.set('CONFIG_PPOLL',
> > > cc.has_function('ppoll'))
> > >  config_host_data.set('CONFIG_PREADV', cc.has_function('preadv', prefix:
> > > '#include <sys/uio.h>'))
> > +config_host_data.set('CONFIG_PTHREAD_FCHDIR_NP',
> > > cc.has_function('pthread_fchdir_np'))
> > > config_host_data.set('CONFIG_SEM_TIMEDWAIT',
> > > cc.has_function('sem_timedwait', dependencies: threads))
> > > config_host_data.set('CONFIG_SENDFILE', cc.has_function('sendfile'))
> > > config_host_data.set('CONFIG_SETNS', cc.has_function('setns') and
> > > cc.has_function('unshare'))
> >
> >
> >