| Message ID | 20220220165056.72289-10-wwcohen@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 9p: Add support for darwin | expand |
On Sonntag, 20. Februar 2022 17:50:54 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 > - Move qemu_mknodat from 9p-util to osdep and os-posix > - Move pthread_fchdir_np declaration only to osdep > - 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 osdep.h and os-posix.c > - Rebase to apply cleanly on top of the 2022-02-10 > changes to 9pfs] > Signed-off-by: Will Cohen <wwcohen@gmail.com> > --- > hw/9pfs/9p-local.c | 4 ++-- > include/qemu/osdep.h | 12 ++++++++++++ > meson.build | 1 + > os-posix.c | 35 +++++++++++++++++++++++++++++++++++ > 4 files changed, 50 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 ce12f64853..c0f442d791 100644 > --- a/include/qemu/osdep.h > +++ b/include/qemu/osdep.h > @@ -818,6 +818,18 @@ static inline int > platform_does_not_support_system(const char *command) */ > struct dirent *qemu_dirent_dup(struct dirent *dent); > > +/* > + * 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); > + > #ifdef __cplusplus > } > #endif > diff --git a/meson.build b/meson.build > index ae5f7eec6e..6fdc0281ad 100644 > --- a/meson.build > +++ b/meson.build > @@ -1557,6 +1557,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')) diff --git a/os-posix.c b/os-posix.c > index ae6c9f2a5e..ccc3d1e9d3 100644 > --- a/os-posix.c > +++ b/os-posix.c > @@ -332,3 +332,38 @@ 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) > + */ > +#if defined CONFIG_DARWIN && 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() is not available on this > version of macOS"); Code style error: ERROR: line over 90 characters #226: FILE: os-posix.c:349: + error_report_once("pthread_fchdir_np() is not available on this version of macOS"); BTW, does anybody know what happened to our Patchew bot that used to automatically report such errors to the list? Best regards, Christian Schoenebeck > + 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; > +} > +#else > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev) > +{ > + return mknodat(dirfd, filename, mode, dev); > +} > +#endif
On Sonntag, 20. Februar 2022 17:50:54 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 > - Move qemu_mknodat from 9p-util to osdep and os-posix > - Move pthread_fchdir_np declaration only to osdep > - 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 osdep.h and os-posix.c > - Rebase to apply cleanly on top of the 2022-02-10 > changes to 9pfs] > Signed-off-by: Will Cohen <wwcohen@gmail.com> > --- > hw/9pfs/9p-local.c | 4 ++-- > include/qemu/osdep.h | 12 ++++++++++++ > meson.build | 1 + > os-posix.c | 35 +++++++++++++++++++++++++++++++++++ > 4 files changed, 50 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 ce12f64853..c0f442d791 100644 > --- a/include/qemu/osdep.h > +++ b/include/qemu/osdep.h > @@ -818,6 +818,18 @@ static inline int > platform_does_not_support_system(const char *command) */ > struct dirent *qemu_dirent_dup(struct dirent *dent); > > +/* > + * 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); > + > #ifdef __cplusplus > } > #endif Peter just informed me that osdep.h is not the right place to add something like this: https://lore.kernel.org/qemu-devel/CAFEAcA_=HAUNomKD2wurSVaAHa5mrk22A1oHKLWUDjk7v6Khmg@mail.gmail.com/ So this should be moved into a different header file. > diff --git a/meson.build b/meson.build > index ae5f7eec6e..6fdc0281ad 100644 > --- a/meson.build > +++ b/meson.build > @@ -1557,6 +1557,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')) diff --git a/os-posix.c b/os-posix.c > index ae6c9f2a5e..ccc3d1e9d3 100644 > --- a/os-posix.c > +++ b/os-posix.c > @@ -332,3 +332,38 @@ 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) > + */ > +#if defined CONFIG_DARWIN && 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() is not available on this > version of macOS"); + 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; > +} > +#else > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev) > +{ > + return mknodat(dirfd, filename, mode, dev); > +} > +#endif
On Tue, Feb 22, 2022 at 9:27 AM Christian Schoenebeck < qemu_oss@crudebyte.com> wrote: > On Sonntag, 20. Februar 2022 17:50:54 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 > > - Move qemu_mknodat from 9p-util to osdep and os-posix > > - Move pthread_fchdir_np declaration only to osdep > > - 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 osdep.h and os-posix.c > > - Rebase to apply cleanly on top of the 2022-02-10 > > changes to 9pfs] > > Signed-off-by: Will Cohen <wwcohen@gmail.com> > > --- > > hw/9pfs/9p-local.c | 4 ++-- > > include/qemu/osdep.h | 12 ++++++++++++ > > meson.build | 1 + > > os-posix.c | 35 +++++++++++++++++++++++++++++++++++ > > 4 files changed, 50 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 ce12f64853..c0f442d791 100644 > > --- a/include/qemu/osdep.h > > +++ b/include/qemu/osdep.h > > @@ -818,6 +818,18 @@ static inline int > > platform_does_not_support_system(const char *command) */ > > struct dirent *qemu_dirent_dup(struct dirent *dent); > > > > +/* > > + * 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); > > + > > #ifdef __cplusplus > > } > > #endif > > Peter just informed me that osdep.h is not the right place to add something > like this: > > https://lore.kernel.org/qemu-devel/CAFEAcA_=HAUNomKD2wurSVaAHa5mrk22A1oHKLWUDjk7v6Khmg@mail.gmail.com/ > > So this should be moved into a different header file. > Does that logic apply here too? In some ways I'd say that mknodat falls into the category of "things without which code would work on most platforms but fail to compile or misbehave on a minority of host OSes." If osdep.h is the wrong place, should it go back to 9p-util, where it lived before it moved to osdep? > > diff --git a/meson.build b/meson.build > > index ae5f7eec6e..6fdc0281ad 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -1557,6 +1557,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')) diff --git a/os-posix.c b/os-posix.c > > index ae6c9f2a5e..ccc3d1e9d3 100644 > > --- a/os-posix.c > > +++ b/os-posix.c > > @@ -332,3 +332,38 @@ 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) > > + */ > > +#if defined CONFIG_DARWIN && 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() is not available on this > > version of macOS"); + 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; > > +} > > +#else > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t > dev) > > +{ > > + return mknodat(dirfd, filename, mode, dev); > > +} > > +#endif > > >
On Freitag, 25. Februar 2022 15:00:40 CET Will Cohen wrote: > On Tue, Feb 22, 2022 at 9:27 AM Christian Schoenebeck < > > qemu_oss@crudebyte.com> wrote: > > On Sonntag, 20. Februar 2022 17:50:54 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 > > > > > > - Move qemu_mknodat from 9p-util to osdep and os-posix > > > - Move pthread_fchdir_np declaration only to osdep > > > - 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 osdep.h and os-posix.c > > > > > > - Rebase to apply cleanly on top of the 2022-02-10 > > > > > > changes to 9pfs] > > > > > > Signed-off-by: Will Cohen <wwcohen@gmail.com> > > > --- > > > > > > hw/9pfs/9p-local.c | 4 ++-- > > > include/qemu/osdep.h | 12 ++++++++++++ > > > meson.build | 1 + > > > os-posix.c | 35 +++++++++++++++++++++++++++++++++++ > > > 4 files changed, 50 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 ce12f64853..c0f442d791 100644 > > > --- a/include/qemu/osdep.h > > > +++ b/include/qemu/osdep.h > > > @@ -818,6 +818,18 @@ static inline int > > > platform_does_not_support_system(const char *command) */ > > > > > > struct dirent *qemu_dirent_dup(struct dirent *dent); > > > > > > +/* > > > + * 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); > > > > > + > > > > > > #ifdef __cplusplus > > > } > > > #endif > > > > Peter just informed me that osdep.h is not the right place to add > > something > > like this: > > > > https://lore.kernel.org/qemu-devel/CAFEAcA_=HAUNomKD2wurSVaAHa5mrk22A1oHKL > > WUDjk7v6Khmg@mail.gmail.com/ > > > > So this should be moved into a different header file. > > Does that logic apply here too? In some ways I'd say that mknodat falls > into the category of "things without which code would work on most > platforms but fail to compile or misbehave on a minority of host OSes." If > osdep.h is the wrong place, should it go back to 9p-util, where it lived > before it moved to osdep? The other function Peter wanted out from osdep.h would fall into that category as well. I understand it like "only put things into osdep.h as very last resort". CCing Peter on this. If he doesn't give you green light, or on doubt, then yes please move it from osdep -> 9p-util. Will, are you planning to send a v9 of this series soon? Next week I need to add changes that would collide with these here. So if you intend to send it soon, then I just wait so that we can finally queue these macOS patches here for QEMU 7.0, before I would go along with my changes. Best regards, Christian Schoenebeck > > > diff --git a/meson.build b/meson.build > > > index ae5f7eec6e..6fdc0281ad 100644 > > > --- a/meson.build > > > +++ b/meson.build > > > @@ -1557,6 +1557,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')) diff --git a/os-posix.c b/os-posix.c > > > index ae6c9f2a5e..ccc3d1e9d3 100644 > > > --- a/os-posix.c > > > +++ b/os-posix.c > > > @@ -332,3 +332,38 @@ 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) > > > + */ > > > +#if defined CONFIG_DARWIN && 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() is not available on this > > > version of macOS"); + 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; > > > +} > > > +#else > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t > > > > dev) > > > > > +{ > > > + return mknodat(dirfd, filename, mode, dev); > > > +} > > > +#endif
On Fri, Feb 25, 2022 at 11:31 AM Christian Schoenebeck < qemu_oss@crudebyte.com> wrote: > On Freitag, 25. Februar 2022 15:00:40 CET Will Cohen wrote: > > On Tue, Feb 22, 2022 at 9:27 AM Christian Schoenebeck < > > > > qemu_oss@crudebyte.com> wrote: > > > On Sonntag, 20. Februar 2022 17:50:54 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 > > > > > > > > - Move qemu_mknodat from 9p-util to osdep and os-posix > > > > - Move pthread_fchdir_np declaration only to osdep > > > > - 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 osdep.h and os-posix.c > > > > > > > > - Rebase to apply cleanly on top of the 2022-02-10 > > > > > > > > changes to 9pfs] > > > > > > > > Signed-off-by: Will Cohen <wwcohen@gmail.com> > > > > --- > > > > > > > > hw/9pfs/9p-local.c | 4 ++-- > > > > include/qemu/osdep.h | 12 ++++++++++++ > > > > meson.build | 1 + > > > > os-posix.c | 35 +++++++++++++++++++++++++++++++++++ > > > > 4 files changed, 50 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 ce12f64853..c0f442d791 100644 > > > > --- a/include/qemu/osdep.h > > > > +++ b/include/qemu/osdep.h > > > > @@ -818,6 +818,18 @@ static inline int > > > > platform_does_not_support_system(const char *command) */ > > > > > > > > struct dirent *qemu_dirent_dup(struct dirent *dent); > > > > > > > > +/* > > > > + * 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); > > > > > > > + > > > > > > > > #ifdef __cplusplus > > > > } > > > > #endif > > > > > > Peter just informed me that osdep.h is not the right place to add > > > something > > > like this: > > > > > > > https://lore.kernel.org/qemu-devel/CAFEAcA_=HAUNomKD2wurSVaAHa5mrk22A1oHKL > > > WUDjk7v6Khmg@mail.gmail.com/ > > > > > > So this should be moved into a different header file. > > > > Does that logic apply here too? In some ways I'd say that mknodat falls > > into the category of "things without which code would work on most > > platforms but fail to compile or misbehave on a minority of host OSes." > If > > osdep.h is the wrong place, should it go back to 9p-util, where it lived > > before it moved to osdep? > > The other function Peter wanted out from osdep.h would fall into that > category > as well. I understand it like "only put things into osdep.h as very last > resort". CCing Peter on this. If he doesn't give you green light, or on > doubt, > then yes please move it from osdep -> 9p-util. > > Will, are you planning to send a v9 of this series soon? Next week I need > to > add changes that would collide with these here. So if you intend to send > it > soon, then I just wait so that we can finally queue these macOS patches > here > for QEMU 7.0, before I would go along with my changes. > > Best regards, > Christian Schoenebeck > Prepping a v9 now based on all the other changes + the formatting issues. I'll send this weekend if that's okay! > > > > > diff --git a/meson.build b/meson.build > > > > index ae5f7eec6e..6fdc0281ad 100644 > > > > --- a/meson.build > > > > +++ b/meson.build > > > > @@ -1557,6 +1557,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')) diff --git a/os-posix.c b/os-posix.c > > > > index ae6c9f2a5e..ccc3d1e9d3 100644 > > > > --- a/os-posix.c > > > > +++ b/os-posix.c > > > > @@ -332,3 +332,38 @@ 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) > > > > + */ > > > > +#if defined CONFIG_DARWIN && 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() is not available on > this > > > > version of macOS"); + 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; > > > > +} > > > > +#else > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t > > > > > > dev) > > > > > > > +{ > > > > + return mknodat(dirfd, filename, mode, dev); > > > > +} > > > > +#endif > > >
On Freitag, 25. Februar 2022 17:38:51 CET Will Cohen wrote: > On Fri, Feb 25, 2022 at 11:31 AM Christian Schoenebeck < > > qemu_oss@crudebyte.com> wrote: > > On Freitag, 25. Februar 2022 15:00:40 CET Will Cohen wrote: > > > On Tue, Feb 22, 2022 at 9:27 AM Christian Schoenebeck [...] > > > osdep.h is the wrong place, should it go back to 9p-util, where it lived > > > before it moved to osdep? > > > > The other function Peter wanted out from osdep.h would fall into that > > category > > as well. I understand it like "only put things into osdep.h as very last > > resort". CCing Peter on this. If he doesn't give you green light, or on > > doubt, > > then yes please move it from osdep -> 9p-util. > > > > Will, are you planning to send a v9 of this series soon? Next week I need > > to > > add changes that would collide with these here. So if you intend to send > > it > > soon, then I just wait so that we can finally queue these macOS patches > > here > > for QEMU 7.0, before I would go along with my changes. > > > > Best regards, > > Christian Schoenebeck > > Prepping a v9 now based on all the other changes + the formatting issues. > I'll send this weekend if that's okay! Thumbs up! :) Best regards, Christian Schoenebeck
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 ce12f64853..c0f442d791 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -818,6 +818,18 @@ static inline int platform_does_not_support_system(const char *command) */ struct dirent *qemu_dirent_dup(struct dirent *dent); +/* + * 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); + #ifdef __cplusplus } #endif diff --git a/meson.build b/meson.build index ae5f7eec6e..6fdc0281ad 100644 --- a/meson.build +++ b/meson.build @@ -1557,6 +1557,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')) diff --git a/os-posix.c b/os-posix.c index ae6c9f2a5e..ccc3d1e9d3 100644 --- a/os-posix.c +++ b/os-posix.c @@ -332,3 +332,38 @@ 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) + */ +#if defined CONFIG_DARWIN && 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() is not available on this version of macOS"); + 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; +} +#else +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev) +{ + return mknodat(dirfd, filename, mode, dev); +} +#endif