On Mon, Feb 7, 2022 at 5:56 PM Christian Schoenebeck <qemu_...@crudebyte.com> wrote:
> On Montag, 7. Februar 2022 23:40:22 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 tihs 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 > > - Move qemu_mknodat from 9p-util to osdep and os-posix] > > Signed-off-by: Will Cohen <wwco...@gmail.com> > > --- > > Like already mentioned by me moments ago on previous v4 (just echoing) ... > > > hw/9pfs/9p-local.c | 4 ++-- > > include/qemu/osdep.h | 10 ++++++++++ > > os-posix.c | 34 ++++++++++++++++++++++++++++++++++ > > 3 files changed, 46 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/include/qemu/osdep.h b/include/qemu/osdep.h > > index d1660d67fa..f3a8367ece 100644 > > --- a/include/qemu/osdep.h > > +++ b/include/qemu/osdep.h > > @@ -810,3 +810,13 @@ static inline int > > platform_does_not_support_system(const char *command) #endif > > > > #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 > > + */ > > +#ifdef CONFIG_DARWIN > > +int pthread_fchdir_np(int fd); > > +#endif > > I would make that: > > #ifdef CONFIG_DARWIN > int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12)); > #endif > > here and ... > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t > dev); > > diff --git a/os-posix.c b/os-posix.c > > index ae6c9f2a5e..95c1607065 100644 > > --- a/os-posix.c > > +++ b/os-posix.c > > @@ -24,6 +24,7 @@ > > */ > > > > #include "qemu/osdep.h" > > +#include <os/availability.h> > > #include <sys/wait.h> > > #include <pwd.h> > > #include <grp.h> > > @@ -332,3 +333,36 @@ int os_mlock(void) > > return -ENOSYS; > > #endif > > } > > + > > +/* > > + * 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) > > + */ > > +#ifdef CONFIG_DARWIN > > + > > +int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12)); > > ... drop the duplicate declaration of pthread_fchdir_np() here. > Trying this out, it reminds me that this use of API_AVAILABLE in os-posix.c relies on the added #include <os/availability.h>. Leaving the include out leads to: .../include/qemu/osdep.h:820:31: error: expected function body after function declarator int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12)); ^ 1 error generated. ninja: build stopped: subcommand failed. make[1]: *** [run-ninja] Error 1 make: *** [all] Error 2 The admonition against modifying osdep.h's includes too much led me to steer away from putting it all in there. If there's no issue with adding +#include <os/availability.h> to osdep.h, then this change makes sense to me. > > + > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t > dev) > > +{ > > + int preserved_errno, err; > > + 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; > > +} > > +#else > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t > dev) > > +{ > > + return mknodat(dirfd, filename, mode, dev); > > +} > > +#endif > > >