On Mon, Feb 28, 2022 at 8:20 AM Christian Schoenebeck <
qemu_...@crudebyte.com> wrote:
> On Sonntag, 27. Februar 2022 23:35:20 CET Will Cohen wrote:
> > From: Keno Fischer <k...@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 <k...@juliacomputing.com>
> > Signed-off-by: Michael Roitzsch <reactorcont...@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 <wwco...@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!
>
>
> > + 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'))
>
>
>
