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

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Montag, 28. Februar 2022 14:36:30 CET Thomas Huth wrote:
> On 28/02/2022 14.20, Christian Schoenebeck 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.
> 
> For lines less than 90 characters, it's just a warning, and I think it's ok
> in such cases to keep it longer than 80 characters, if the result of
> breaking it up would look more awkward otherwise.
> 
>   Thomas

This doesn't look awkward to me:

        error_report_once(
            "pthread_fchdir_np() is not available on this version of macOS"
        );

It silences the warning and grep is not affected either.

Best regards,
Christian Schoenebeck