[v5,09/11] 9p: darwin: Implement compatibility for mknodat

Commit Message

Will Cohen Feb. 7, 2022, 10:40 p.m. UTC

From: Keno Fischer <[e-mail]>

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 <[e-mail]>
Signed-off-by: Michael Roitzsch <[e-mail]>
[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 <[e-mail]>
---
 hw/9pfs/9p-local.c   |  4 ++--
 include/qemu/osdep.h | 10 ++++++++++
 os-posix.c           | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 2 deletions(-)

Comments

Christian Schoenebeck Feb. 7, 2022, 10:56 p.m. UTC | #1

On Montag, 7. Februar 2022 23:40:22 CET Will Cohen wrote:
> From: Keno Fischer <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Michael Roitzsch <[e-mail]>
> [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 <[e-mail]>
> ---

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.

> +
> +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

Philippe Mathieu-Daudé Feb. 8, 2022, 10:55 a.m. UTC | #2

On 7/2/22 23:40, Will Cohen wrote:
> From: Keno Fischer <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Michael Roitzsch <[e-mail]>
> [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 <[e-mail]>
> ---
>   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/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
> +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev);

Misplaced. You want the declaration before the __cplusplus guard.

Will Cohen Feb. 8, 2022, 1:36 p.m. UTC | #3

On Mon, Feb 7, 2022 at 5:56 PM Christian Schoenebeck <[e-mail]>
wrote:

> On Montag, 7. Februar 2022 23:40:22 CET Will Cohen wrote:
> > From: Keno Fischer <[e-mail]>
> >
> > 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 <[e-mail]>
> > Signed-off-by: Michael Roitzsch <[e-mail]>
> > [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 <[e-mail]>
> > ---
>
> 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
>
>
>

Christian Schoenebeck Feb. 8, 2022, 3:02 p.m. UTC | #4

On Dienstag, 8. Februar 2022 14:36:42 CET Will Cohen wrote:
> On Mon, Feb 7, 2022 at 5:56 PM Christian Schoenebeck
> <[e-mail]>
> wrote:
> > On Montag, 7. Februar 2022 23:40:22 CET Will Cohen wrote:
> > > From: Keno Fischer <[e-mail]>
> > > 
> > > 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 <[e-mail]>
> > > Signed-off-by: Michael Roitzsch <[e-mail]>
> > > [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 <[e-mail]>
> > > ---
> > 
> > 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.

If you embed that include into ifdefs, sure!

#ifdef CONFIG_DARWIN
/* defines API_AVAILABLE(...) */
#include <os/availability.h>
#endif

One more thing though ...

> > > +
> > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> > 
> > dev)
> > 
> > > +{
> > > +    int preserved_errno, err;

pthread_fchdir_np() is weakly linked. So I guess here should be a check like:

	if (!pthread_fchdir_np) {
		return -ENOTSUPP;
	}

Before trying to call pthread_fchdir_np() below. As already discussed with the
Chromium [1] example, some do that a bit differently by using
__builtin_available():

	if (__builtin_available(macOS 10.12, *)) {
		return -ENOTSUPP;
	}

Which makes me wonder why they are not doing a simple NULL check?

[1] https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/launch_mac.cc#110

> > > +    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

Will Cohen Feb. 8, 2022, 3:57 p.m. UTC | #5

My inclination is to go with the __builtin_available(macOS 10.12, *) path,
if acceptable, since it partially mirrors the API_AVAILABLE macro idea. I
guess it's perhaps a tradeoff between predicting the future unknown
availability of functions versus just ensuring a minimum macOS version and
hoping for the best. With any luck, the distinction between the two
approaches will be moot, if we try to assume that a future macOS version
that removes this also provides mknodat.

On Tue, Feb 8, 2022 at 10:03 AM Christian Schoenebeck <
[e-mail]> wrote:

> On Dienstag, 8. Februar 2022 14:36:42 CET Will Cohen wrote:
> > On Mon, Feb 7, 2022 at 5:56 PM Christian Schoenebeck
> > <[e-mail]>
> > wrote:
> > > On Montag, 7. Februar 2022 23:40:22 CET Will Cohen wrote:
> > > > From: Keno Fischer <[e-mail]>
> > > >
> > > > 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 <[e-mail]>
> > > > Signed-off-by: Michael Roitzsch <[e-mail]>
> > > > [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 <[e-mail]>
> > > > ---
> > >
> > > 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.
>
> If you embed that include into ifdefs, sure!
>
> #ifdef CONFIG_DARWIN
> /* defines API_AVAILABLE(...) */
> #include <os/availability.h>
> #endif
>
> One more thing though ...
>
> > > > +
> > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> > >
> > > dev)
> > >
> > > > +{
> > > > +    int preserved_errno, err;
>
> pthread_fchdir_np() is weakly linked. So I guess here should be a check
> like:
>
>         if (!pthread_fchdir_np) {
>                 return -ENOTSUPP;
>         }
>
> Before trying to call pthread_fchdir_np() below. As already discussed with
> the
> Chromium [1] example, some do that a bit differently by using
> __builtin_available():
>
>         if (__builtin_available(macOS 10.12, *)) {
>                 return -ENOTSUPP;
>         }
>
> Which makes me wonder why they are not doing a simple NULL check?
>
> [1]
> https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/launch_mac.cc#110
>
> > > > +    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
>
>
>

Christian Schoenebeck Feb. 8, 2022, 4:11 p.m. UTC | #6

On Dienstag, 8. Februar 2022 16:57:55 CET Will Cohen wrote:
> My inclination is to go with the __builtin_available(macOS 10.12, *) path,
> if acceptable, since it partially mirrors the API_AVAILABLE macro idea. I

OTOH that's duplication of the ">= macOS 10.12" info, plus __builtin_available
is direct use of a clang-only extension, whereas API_AVAILABLE() works (or
more precisely: doesn't error out at least) with other compilers like GCC as
well. GCC is sometimes used for cross-compilation.

Moreover, I would also add an error message in this case, e.g.:

    if (!pthread_fchdir_np) {
        error_report_once("pthread_fchdir_np() is not available on this macOS version");
        return -ENOTSUPP;	
    }

I should elaborate why I think this is needed: you are already doing a Meson
check for the existence of pthread_fchdir_np(), but the system where QEMU is
compiled and the systems where the compiled binary will be running, might be
different ones (i.e. different macOS versions).

Best regards,
Christian Schoenebeck

> guess it's perhaps a tradeoff between predicting the future unknown
> availability of functions versus just ensuring a minimum macOS version and
> hoping for the best. With any luck, the distinction between the two
> approaches will be moot, if we try to assume that a future macOS version
> that removes this also provides mknodat.
> 
> On Tue, Feb 8, 2022 at 10:03 AM Christian Schoenebeck <
> 
> [e-mail]> wrote:
> > On Dienstag, 8. Februar 2022 14:36:42 CET Will Cohen wrote:
> > > On Mon, Feb 7, 2022 at 5:56 PM Christian Schoenebeck
> > > <[e-mail]>
> > > 
> > > wrote:
> > > > On Montag, 7. Februar 2022 23:40:22 CET Will Cohen wrote:
> > > > > From: Keno Fischer <[e-mail]>
> > > > > 
> > > > > 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 <[e-mail]>
> > > > > Signed-off-by: Michael Roitzsch <[e-mail]>
> > > > > [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 <[e-mail]>
> > > > > ---
> > > > 
> > > > 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.
> > 
> > If you embed that include into ifdefs, sure!
> > 
> > #ifdef CONFIG_DARWIN
> > /* defines API_AVAILABLE(...) */
> > #include <os/availability.h>
> > #endif
> > 
> > One more thing though ...
> > 
> > > > > +
> > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode,
> > > > > dev_t
> > > > 
> > > > dev)
> > > > 
> > > > > +{
> > > > > +    int preserved_errno, err;
> > 
> > pthread_fchdir_np() is weakly linked. So I guess here should be a check
> > 
> > like:
> >         if (!pthread_fchdir_np) {
> >         
> >                 return -ENOTSUPP;
> >         
> >         }
> > 
> > Before trying to call pthread_fchdir_np() below. As already discussed with
> > the
> > Chromium [1] example, some do that a bit differently by using
> > 
> > __builtin_available():
> >         if (__builtin_available(macOS 10.12, *)) {
> >         
> >                 return -ENOTSUPP;
> >         
> >         }
> > 
> > Which makes me wonder why they are not doing a simple NULL check?
> > 
> > [1]
> > https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/launch_
> > mac.cc#110> 
> > > > > +    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

Will Cohen Feb. 8, 2022, 4:19 p.m. UTC | #7

On Tue, Feb 8, 2022 at 11:11 AM Christian Schoenebeck <
[e-mail]> wrote:

> On Dienstag, 8. Februar 2022 16:57:55 CET Will Cohen wrote:
> > My inclination is to go with the __builtin_available(macOS 10.12, *)
> path,
> > if acceptable, since it partially mirrors the API_AVAILABLE macro idea. I
>
> OTOH that's duplication of the ">= macOS 10.12" info, plus
> __builtin_available
> is direct use of a clang-only extension, whereas API_AVAILABLE() works (or
> more precisely: doesn't error out at least) with other compilers like GCC
> as
> well. GCC is sometimes used for cross-compilation.
>
> Moreover, I would also add an error message in this case, e.g.:
>
>     if (!pthread_fchdir_np) {
>         error_report_once("pthread_fchdir_np() is not available on this
> macOS version");
>         return -ENOTSUPP;
>     }
>
> I should elaborate why I think this is needed: you are already doing a
> Meson
> check for the existence of pthread_fchdir_np(), but the system where QEMU
> is
> compiled and the systems where the compiled binary will be running, might
> be
> different ones (i.e. different macOS versions).
>
> Best regards,
> Christian Schoenebeck
>

Agreed, that way actually closes the edge case. Something along these lines
briefly crossed my mind during a previous version, but I quickly got passed
it by assuming that the compiling entity would always be the bottleneck,
which makes no sense in hindsight, so I very much appreciate that you
caught this.


> > guess it's perhaps a tradeoff between predicting the future unknown
> > availability of functions versus just ensuring a minimum macOS version
> and
> > hoping for the best. With any luck, the distinction between the two
> > approaches will be moot, if we try to assume that a future macOS version
> > that removes this also provides mknodat.
> >
> > On Tue, Feb 8, 2022 at 10:03 AM Christian Schoenebeck <
> >
> > [e-mail]> wrote:
> > > On Dienstag, 8. Februar 2022 14:36:42 CET Will Cohen wrote:
> > > > On Mon, Feb 7, 2022 at 5:56 PM Christian Schoenebeck
> > > > <[e-mail]>
> > > >
> > > > wrote:
> > > > > On Montag, 7. Februar 2022 23:40:22 CET Will Cohen wrote:
> > > > > > From: Keno Fischer <[e-mail]>
> > > > > >
> > > > > > 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 <[e-mail]>
> > > > > > Signed-off-by: Michael Roitzsch <[e-mail]>
> > > > > > [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 <[e-mail]>
> > > > > > ---
> > > > >
> > > > > 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.
> > >
> > > If you embed that include into ifdefs, sure!
> > >
> > > #ifdef CONFIG_DARWIN
> > > /* defines API_AVAILABLE(...) */
> > > #include <os/availability.h>
> > > #endif
> > >
> > > One more thing though ...
> > >
> > > > > > +
> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode,
> > > > > > dev_t
> > > > >
> > > > > dev)
> > > > >
> > > > > > +{
> > > > > > +    int preserved_errno, err;
> > >
> > > pthread_fchdir_np() is weakly linked. So I guess here should be a check
> > >
> > > like:
> > >         if (!pthread_fchdir_np) {
> > >
> > >                 return -ENOTSUPP;
> > >
> > >         }
> > >
> > > Before trying to call pthread_fchdir_np() below. As already discussed
> with
> > > the
> > > Chromium [1] example, some do that a bit differently by using
> > >
> > > __builtin_available():
> > >         if (__builtin_available(macOS 10.12, *)) {
> > >
> > >                 return -ENOTSUPP;
> > >
> > >         }
> > >
> > > Which makes me wonder why they are not doing a simple NULL check?
> > >
> > > [1]
> > >
> https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/launch_
> > > mac.cc#110>
> > > > > > +    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
>

Will Cohen Feb. 8, 2022, 6:04 p.m. UTC | #8

On Tue, Feb 8, 2022 at 11:19 AM Will Cohen <[e-mail]> wrote:

> On Tue, Feb 8, 2022 at 11:11 AM Christian Schoenebeck <
> [e-mail]> wrote:
>
>> On Dienstag, 8. Februar 2022 16:57:55 CET Will Cohen wrote:
>> > My inclination is to go with the __builtin_available(macOS 10.12, *)
>> path,
>> > if acceptable, since it partially mirrors the API_AVAILABLE macro idea.
>> I
>>
>> OTOH that's duplication of the ">= macOS 10.12" info, plus
>> __builtin_available
>> is direct use of a clang-only extension, whereas API_AVAILABLE() works (or
>> more precisely: doesn't error out at least) with other compilers like GCC
>> as
>> well. GCC is sometimes used for cross-compilation.
>>
>> Moreover, I would also add an error message in this case, e.g.:
>>
>>     if (!pthread_fchdir_np) {
>>         error_report_once("pthread_fchdir_np() is not available on this
>> macOS version");
>>         return -ENOTSUPP;
>>     }
>>
>> I should elaborate why I think this is needed: you are already doing a
>> Meson
>> check for the existence of pthread_fchdir_np(), but the system where QEMU
>> is
>> compiled and the systems where the compiled binary will be running, might
>> be
>> different ones (i.e. different macOS versions).
>>
>> Best regards,
>> Christian Schoenebeck
>>
>
> Agreed, that way actually closes the edge case. Something along these
> lines briefly crossed my mind during a previous version, but I quickly got
> passed it by assuming that the compiling entity would always be the
> bottleneck, which makes no sense in hindsight, so I very much appreciate
> that you caught this.
>

Ah, rebuilding leads to a compiler error:

../os-posix.c:348:10: warning: address of function 'pthread_fchdir_np' will
always evaluate to 'true' [-Wpointer-bool-conversion]
    if (!pthread_fchdir_np) {
        ~^~~~~~~~~~~~~~~~~

I don't have a machine that's pre-10.12 so I can't see what the result is
there, but this might be why the __builtin_available approach got taken.


>
>
>> > guess it's perhaps a tradeoff between predicting the future unknown
>> > availability of functions versus just ensuring a minimum macOS version
>> and
>> > hoping for the best. With any luck, the distinction between the two
>> > approaches will be moot, if we try to assume that a future macOS version
>> > that removes this also provides mknodat.
>> >
>> > On Tue, Feb 8, 2022 at 10:03 AM Christian Schoenebeck <
>> >
>> > [e-mail]> wrote:
>> > > On Dienstag, 8. Februar 2022 14:36:42 CET Will Cohen wrote:
>> > > > On Mon, Feb 7, 2022 at 5:56 PM Christian Schoenebeck
>> > > > <[e-mail]>
>> > > >
>> > > > wrote:
>> > > > > On Montag, 7. Februar 2022 23:40:22 CET Will Cohen wrote:
>> > > > > > From: Keno Fischer <[e-mail]>
>> > > > > >
>> > > > > > 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 <[e-mail]>
>> > > > > > Signed-off-by: Michael Roitzsch <[e-mail]>
>> > > > > > [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 <[e-mail]>
>> > > > > > ---
>> > > > >
>> > > > > 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.
>> > >
>> > > If you embed that include into ifdefs, sure!
>> > >
>> > > #ifdef CONFIG_DARWIN
>> > > /* defines API_AVAILABLE(...) */
>> > > #include <os/availability.h>
>> > > #endif
>> > >
>> > > One more thing though ...
>> > >
>> > > > > > +
>> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode,
>> > > > > > dev_t
>> > > > >
>> > > > > dev)
>> > > > >
>> > > > > > +{
>> > > > > > +    int preserved_errno, err;
>> > >
>> > > pthread_fchdir_np() is weakly linked. So I guess here should be a
>> check
>> > >
>> > > like:
>> > >         if (!pthread_fchdir_np) {
>> > >
>> > >                 return -ENOTSUPP;
>> > >
>> > >         }
>> > >
>> > > Before trying to call pthread_fchdir_np() below. As already discussed
>> with
>> > > the
>> > > Chromium [1] example, some do that a bit differently by using
>> > >
>> > > __builtin_available():
>> > >         if (__builtin_available(macOS 10.12, *)) {
>> > >
>> > >                 return -ENOTSUPP;
>> > >
>> > >         }
>> > >
>> > > Which makes me wonder why they are not doing a simple NULL check?
>> > >
>> > > [1]
>> > >
>> https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/launch_
>> > > mac.cc#110>
>> > > > > > +    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
>>
>

Christian Schoenebeck Feb. 8, 2022, 6:28 p.m. UTC | #9

On Dienstag, 8. Februar 2022 19:04:31 CET Will Cohen wrote:
> On Tue, Feb 8, 2022 at 11:19 AM Will Cohen <[e-mail]> wrote:
> > On Tue, Feb 8, 2022 at 11:11 AM Christian Schoenebeck <
> > 
> > [e-mail]> wrote:
> >> On Dienstag, 8. Februar 2022 16:57:55 CET Will Cohen wrote:
> >> > My inclination is to go with the __builtin_available(macOS 10.12, *)
> >> 
> >> path,
> >> 
> >> > if acceptable, since it partially mirrors the API_AVAILABLE macro idea.
> >> 
> >> I
> >> 
> >> OTOH that's duplication of the ">= macOS 10.12" info, plus
> >> __builtin_available
> >> is direct use of a clang-only extension, whereas API_AVAILABLE() works
> >> (or
> >> more precisely: doesn't error out at least) with other compilers like GCC
> >> as
> >> well. GCC is sometimes used for cross-compilation.
> >> 
> >> Moreover, I would also add an error message in this case, e.g.:
> >>     if (!pthread_fchdir_np) {
> >>     
> >>         error_report_once("pthread_fchdir_np() is not available on this
> >> 
> >> macOS version");
> >> 
> >>         return -ENOTSUPP;
> >>     
> >>     }
> >> 
> >> I should elaborate why I think this is needed: you are already doing a
> >> Meson
> >> check for the existence of pthread_fchdir_np(), but the system where QEMU
> >> is
> >> compiled and the systems where the compiled binary will be running, might
> >> be
> >> different ones (i.e. different macOS versions).
> >> 
> >> Best regards,
> >> Christian Schoenebeck
> > 
> > Agreed, that way actually closes the edge case. Something along these
> > lines briefly crossed my mind during a previous version, but I quickly got
> > passed it by assuming that the compiling entity would always be the
> > bottleneck, which makes no sense in hindsight, so I very much appreciate
> > that you caught this.
> 
> Ah, rebuilding leads to a compiler error:
> 
> ../os-posix.c:348:10: warning: address of function 'pthread_fchdir_np' will
> always evaluate to 'true' [-Wpointer-bool-conversion]
>     if (!pthread_fchdir_np) {
>         ~^~~~~~~~~~~~~~~~~
> 
> I don't have a machine that's pre-10.12 so I can't see what the result is
> there, but this might be why the __builtin_available approach got taken.

I guess that's because you are compiling QEMU with minimum deployment target 
being macOS >= 10.12 already. In this case the compiler won't make 
pthread_fchdir_np a weak link, it only does emit a weak link if you are 
targeting macOS versions prior than the defined availablity attribute,
hence the address would never be NULL here and hence the compiler warning.

So I guess it is okay if you just omit checking presence of pthread_fchdir_np 
at runtime and just assume it exists.

Added Akihiko on CC, just in case he would have something to add on this macOS 
issue here. :)

Best regards,
Christian Schoenebeck

> >> > guess it's perhaps a tradeoff between predicting the future unknown
> >> > availability of functions versus just ensuring a minimum macOS version
> >> 
> >> and
> >> 
> >> > hoping for the best. With any luck, the distinction between the two
> >> > approaches will be moot, if we try to assume that a future macOS
> >> > version
> >> > that removes this also provides mknodat.
> >> > 
> >> > On Tue, Feb 8, 2022 at 10:03 AM Christian Schoenebeck <
> >> > 
> >> > [e-mail]> wrote:
> >> > > On Dienstag, 8. Februar 2022 14:36:42 CET Will Cohen wrote:
> >> > > > On Mon, Feb 7, 2022 at 5:56 PM Christian Schoenebeck
> >> > > > <[e-mail]>
> >> > > > 
> >> > > > wrote:
> >> > > > > On Montag, 7. Februar 2022 23:40:22 CET Will Cohen wrote:
> >> > > > > > From: Keno Fischer <[e-mail]>
> >> > > > > > 
> >> > > > > > 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 <[e-mail]>
> >> > > > > > Signed-off-by: Michael Roitzsch <[e-mail]>
> >> > > > > > [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 <[e-mail]>
> >> > > > > > ---
> >> > > > > 
> >> > > > > 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.
> >> > > 
> >> > > If you embed that include into ifdefs, sure!
> >> > > 
> >> > > #ifdef CONFIG_DARWIN
> >> > > /* defines API_AVAILABLE(...) */
> >> > > #include <os/availability.h>
> >> > > #endif
> >> > > 
> >> > > One more thing though ...
> >> > > 
> >> > > > > > +
> >> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode,
> >> > > > > > dev_t
> >> > > > > 
> >> > > > > dev)
> >> > > > > 
> >> > > > > > +{
> >> > > > > > +    int preserved_errno, err;
> >> > > 
> >> > > pthread_fchdir_np() is weakly linked. So I guess here should be a
> >> 
> >> check
> >> 
> >> > > like:
> >> > >         if (!pthread_fchdir_np) {
> >> > >         
> >> > >                 return -ENOTSUPP;
> >> > >         
> >> > >         }
> >> > > 
> >> > > Before trying to call pthread_fchdir_np() below. As already discussed
> >> 
> >> with
> >> 
> >> > > the
> >> > > Chromium [1] example, some do that a bit differently by using
> >> > > 
> >> > > __builtin_available():
> >> > >         if (__builtin_available(macOS 10.12, *)) {
> >> > >         
> >> > >                 return -ENOTSUPP;
> >> > >         
> >> > >         }
> >> > > 
> >> > > Which makes me wonder why they are not doing a simple NULL check?
> >> > > 
> >> > > [1]
> >> 
> >> https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/launch
> >> _
> >> 
> >> > > mac.cc#110>
> >> > > 
> >> > > > > > +    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


Best regards,
Christian Schoenebeck

Christian Schoenebeck Feb. 8, 2022, 7:48 p.m. UTC | #10

On Dienstag, 8. Februar 2022 19:28:21 CET Christian Schoenebeck wrote:
> On Dienstag, 8. Februar 2022 19:04:31 CET Will Cohen wrote:
> > On Tue, Feb 8, 2022 at 11:19 AM Will Cohen <[e-mail]> wrote:
> > > On Tue, Feb 8, 2022 at 11:11 AM Christian Schoenebeck <
> > > 
> > > [e-mail]> wrote:
> > >> On Dienstag, 8. Februar 2022 16:57:55 CET Will Cohen wrote:
> > >> > My inclination is to go with the __builtin_available(macOS 10.12, *)
> > >> 
> > >> path,
> > >> 
> > >> > if acceptable, since it partially mirrors the API_AVAILABLE macro
> > >> > idea.
> > >> 
> > >> I
> > >> 
> > >> OTOH that's duplication of the ">= macOS 10.12" info, plus
> > >> __builtin_available
> > >> is direct use of a clang-only extension, whereas API_AVAILABLE() works
> > >> (or
> > >> more precisely: doesn't error out at least) with other compilers like
> > >> GCC
> > >> as
> > >> well. GCC is sometimes used for cross-compilation.
> > >> 
> > >> Moreover, I would also add an error message in this case, e.g.:
> > >>     if (!pthread_fchdir_np) {
> > >>     
> > >>         error_report_once("pthread_fchdir_np() is not available on this
> > >> 
> > >> macOS version");
> > >> 
> > >>         return -ENOTSUPP;
> > >>     
> > >>     }
> > >> 
> > >> I should elaborate why I think this is needed: you are already doing a
> > >> Meson
> > >> check for the existence of pthread_fchdir_np(), but the system where
> > >> QEMU
> > >> is
> > >> compiled and the systems where the compiled binary will be running,
> > >> might
> > >> be
> > >> different ones (i.e. different macOS versions).
> > >> 
> > >> Best regards,
> > >> Christian Schoenebeck
> > > 
> > > Agreed, that way actually closes the edge case. Something along these
> > > lines briefly crossed my mind during a previous version, but I quickly
> > > got
> > > passed it by assuming that the compiling entity would always be the
> > > bottleneck, which makes no sense in hindsight, so I very much appreciate
> > > that you caught this.
> > 
> > Ah, rebuilding leads to a compiler error:
> > 
> > ../os-posix.c:348:10: warning: address of function 'pthread_fchdir_np'
> > will
> > always evaluate to 'true' [-Wpointer-bool-conversion]
> > 
> >     if (!pthread_fchdir_np) {
> >     
> >         ~^~~~~~~~~~~~~~~~~
> > 
> > I don't have a machine that's pre-10.12 so I can't see what the result is
> > there, but this might be why the __builtin_available approach got taken.
> 
> I guess that's because you are compiling QEMU with minimum deployment target
> being macOS >= 10.12 already. In this case the compiler won't make
> pthread_fchdir_np a weak link, it only does emit a weak link if you are
> targeting macOS versions prior than the defined availablity attribute,
> hence the address would never be NULL here and hence the compiler warning.
> 
> So I guess it is okay if you just omit checking presence of
> pthread_fchdir_np at runtime and just assume it exists.
> 
> Added Akihiko on CC, just in case he would have something to add on this
> macOS issue here. :)

On a second thought: this case a bit special. Are we worried that 
pthread_fchdir_np() is "not yet" available on macOS, or "no longer" available. 
Probably both, right?

So maybe it would make sense to replace the API_AVAILABLE() attribute directly 
with a __attribute__((weak)) attribute. Then the runtime check with the 
proposed error message would also trigger if a bleeding edge macOS version no 
longer has pthread_fchdir_np().

Also keep in mind: there are always also the MAC_OS_X_VERSION_MIN_REQUIRED and 
MAC_OS_X_VERSION_MAX_ALLOWED macros to query the deployment target at compile 
time to wrap deployment target dependent code accordingly.

On doubt you could just make some tests there by simply "inventing" a non-
existent function.

Best regards,
Christian Schoenebeck

> > >> > guess it's perhaps a tradeoff between predicting the future unknown
> > >> > availability of functions versus just ensuring a minimum macOS
> > >> > version
> > >> 
> > >> and
> > >> 
> > >> > hoping for the best. With any luck, the distinction between the two
> > >> > approaches will be moot, if we try to assume that a future macOS
> > >> > version
> > >> > that removes this also provides mknodat.
> > >> > 
> > >> > On Tue, Feb 8, 2022 at 10:03 AM Christian Schoenebeck <
> > >> > 
> > >> > [e-mail]> wrote:
> > >> > > On Dienstag, 8. Februar 2022 14:36:42 CET Will Cohen wrote:
> > >> > > > On Mon, Feb 7, 2022 at 5:56 PM Christian Schoenebeck
> > >> > > > <[e-mail]>
> > >> > > > 
> > >> > > > wrote:
> > >> > > > > On Montag, 7. Februar 2022 23:40:22 CET Will Cohen wrote:
> > >> > > > > > From: Keno Fischer <[e-mail]>
> > >> > > > > > 
> > >> > > > > > 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 <[e-mail]>
> > >> > > > > > Signed-off-by: Michael Roitzsch <[e-mail]>
> > >> > > > > > [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 <[e-mail]>
> > >> > > > > > ---
> > >> > > > > 
> > >> > > > > 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.
> > >> > > 
> > >> > > If you embed that include into ifdefs, sure!
> > >> > > 
> > >> > > #ifdef CONFIG_DARWIN
> > >> > > /* defines API_AVAILABLE(...) */
> > >> > > #include <os/availability.h>
> > >> > > #endif
> > >> > > 
> > >> > > One more thing though ...
> > >> > > 
> > >> > > > > > +
> > >> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t
> > >> > > > > > mode,
> > >> > > > > > dev_t
> > >> > > > > 
> > >> > > > > dev)
> > >> > > > > 
> > >> > > > > > +{
> > >> > > > > > +    int preserved_errno, err;
> > >> > > 
> > >> > > pthread_fchdir_np() is weakly linked. So I guess here should be a
> > >> 
> > >> check
> > >> 
> > >> > > like:
> > >> > >         if (!pthread_fchdir_np) {
> > >> > >         
> > >> > >                 return -ENOTSUPP;
> > >> > >         
> > >> > >         }
> > >> > > 
> > >> > > Before trying to call pthread_fchdir_np() below. As already
> > >> > > discussed
> > >> 
> > >> with
> > >> 
> > >> > > the
> > >> > > Chromium [1] example, some do that a bit differently by using
> > >> > > 
> > >> > > __builtin_available():
> > >> > >         if (__builtin_available(macOS 10.12, *)) {
> > >> > >         
> > >> > >                 return -ENOTSUPP;
> > >> > >         
> > >> > >         }
> > >> > > 
> > >> > > Which makes me wonder why they are not doing a simple NULL check?
> > >> > > 
> > >> > > [1]
> > >> 
> > >> https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/laun
> > >> ch
> > >> _
> > >> 
> > >> > > mac.cc#110>
> > >> > > 
> > >> > > > > > +    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
> 
> Best regards,
> Christian Schoenebeck


Best regards,
Christian Schoenebeck

Will Cohen Feb. 8, 2022, 10:57 p.m. UTC | #11

On Tue, Feb 8, 2022 at 2:49 PM Christian Schoenebeck <[e-mail]>
wrote:

> On Dienstag, 8. Februar 2022 19:28:21 CET Christian Schoenebeck wrote:
> > On Dienstag, 8. Februar 2022 19:04:31 CET Will Cohen wrote:
> > > On Tue, Feb 8, 2022 at 11:19 AM Will Cohen <[e-mail]> wrote:
> > > > On Tue, Feb 8, 2022 at 11:11 AM Christian Schoenebeck <
> > > >
> > > > [e-mail]> wrote:
> > > >> On Dienstag, 8. Februar 2022 16:57:55 CET Will Cohen wrote:
> > > >> > My inclination is to go with the __builtin_available(macOS 10.12,
> *)
> > > >>
> > > >> path,
> > > >>
> > > >> > if acceptable, since it partially mirrors the API_AVAILABLE macro
> > > >> > idea.
> > > >>
> > > >> I
> > > >>
> > > >> OTOH that's duplication of the ">= macOS 10.12" info, plus
> > > >> __builtin_available
> > > >> is direct use of a clang-only extension, whereas API_AVAILABLE()
> works
> > > >> (or
> > > >> more precisely: doesn't error out at least) with other compilers
> like
> > > >> GCC
> > > >> as
> > > >> well. GCC is sometimes used for cross-compilation.
> > > >>
> > > >> Moreover, I would also add an error message in this case, e.g.:
> > > >>     if (!pthread_fchdir_np) {
> > > >>
> > > >>         error_report_once("pthread_fchdir_np() is not available on
> this
> > > >>
> > > >> macOS version");
> > > >>
> > > >>         return -ENOTSUPP;
> > > >>
> > > >>     }
> > > >>
> > > >> I should elaborate why I think this is needed: you are already
> doing a
> > > >> Meson
> > > >> check for the existence of pthread_fchdir_np(), but the system where
> > > >> QEMU
> > > >> is
> > > >> compiled and the systems where the compiled binary will be running,
> > > >> might
> > > >> be
> > > >> different ones (i.e. different macOS versions).
> > > >>
> > > >> Best regards,
> > > >> Christian Schoenebeck
> > > >
> > > > Agreed, that way actually closes the edge case. Something along these
> > > > lines briefly crossed my mind during a previous version, but I
> quickly
> > > > got
> > > > passed it by assuming that the compiling entity would always be the
> > > > bottleneck, which makes no sense in hindsight, so I very much
> appreciate
> > > > that you caught this.
> > >
> > > Ah, rebuilding leads to a compiler error:
> > >
> > > ../os-posix.c:348:10: warning: address of function 'pthread_fchdir_np'
> > > will
> > > always evaluate to 'true' [-Wpointer-bool-conversion]
> > >
> > >     if (!pthread_fchdir_np) {
> > >
> > >         ~^~~~~~~~~~~~~~~~~
> > >
> > > I don't have a machine that's pre-10.12 so I can't see what the result
> is
> > > there, but this might be why the __builtin_available approach got
> taken.
> >
> > I guess that's because you are compiling QEMU with minimum deployment
> target
> > being macOS >= 10.12 already. In this case the compiler won't make
> > pthread_fchdir_np a weak link, it only does emit a weak link if you are
> > targeting macOS versions prior than the defined availablity attribute,
> > hence the address would never be NULL here and hence the compiler
> warning.
> >
> > So I guess it is okay if you just omit checking presence of
> > pthread_fchdir_np at runtime and just assume it exists.
> >
> > Added Akihiko on CC, just in case he would have something to add on this
> > macOS issue here. :)
>
> On a second thought: this case a bit special. Are we worried that
> pthread_fchdir_np() is "not yet" available on macOS, or "no longer"
> available.
> Probably both, right?
>
> So maybe it would make sense to replace the API_AVAILABLE() attribute
> directly
> with a __attribute__((weak)) attribute. Then the runtime check with the
> proposed error message would also trigger if a bleeding edge macOS version
> no
> longer has pthread_fchdir_np().
>
> Also keep in mind: there are always also the MAC_OS_X_VERSION_MIN_REQUIRED
> and
> MAC_OS_X_VERSION_MAX_ALLOWED macros to query the deployment target at
> compile
> time to wrap deployment target dependent code accordingly.
>
> On doubt you could just make some tests there by simply "inventing" a non-
> existent function.
>
> Best regards,
> Christian Schoenebeck
>

I like the idea of switching it to __attribute__((weak)). I should note
that I'm not sure that I can actually fully test this out since I'm getting
stuck with the linker noting my undefined fake function during the build,
but the idea does make logical sense to me for the future fail case and the
happy case builds again when implemented with actual pthread_fchdir_np:

[1075/2909] Linking target qemu-nbd
FAILED: qemu-nbd
cc -m64 -mcx16  -o qemu-nbd qemu-nbd.p/qemu-nbd.c.o -Wl,-dead_strip_dylibs
-Wl,-headerpad_max_install_names -Wl,-undefined,error -Wl,-force_load
libblockdev.fa -Wl,-force_load libblock.fa -Wl,-force_load libcrypto.fa
-Wl,-force_load libauthz.fa -Wl,-force_load libqom.fa -Wl,-force_load
libio.fa -fstack-protector-strong
-Wl,-rpath,/usr/local/Cellar/gnutls/3.7.3/lib
-Wl,-rpath,/usr/local/Cellar/pixman/0.40.0/lib libqemuutil.a libblockdev.fa
libblock.fa libcrypto.fa libauthz.fa libqom.fa libio.fa @block.syms
/usr/local/Cellar/gnutls/3.7.3/lib/libgnutls.dylib -lutil
-L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib -lgio-2.0
-lgobject-2.0 -lglib-2.0 -lintl -L/usr/local/Cellar/glib/2.70.3/lib
-L/usr/local/opt/gettext/lib -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl -lm
-L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
-lgmodule-2.0 -lglib-2.0 -lintl
/usr/local/Cellar/pixman/0.40.0/lib/libpixman-1.dylib -lz -lxml2 -framework
CoreFoundation -framework IOKit -lcurl -L/usr/local/Cellar/glib/2.70.3/lib
-L/usr/local/opt/gettext/lib -lgmodule-2.0 -lglib-2.0 -lintl -lbz2
/usr/local/Cellar/libssh/0.9.6/lib/libssh.dylib -lpam
Undefined symbols for architecture x86_64:
  "_pthread_fchdir_npfoo", referenced from:
      _qemu_mknodat in libblockdev.fa(os-posix.c.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see
invocation)
ninja: build stopped: subcommand failed.
make[1]: *** [run-ninja] Error 1
make: *** [all] Error 2

With that caveat re testing in mind, unless there's another recommended
path forward, I think it makes sense to stick with __attribute__((weak))
and prepare v6 which incorporates this and all the other feedback from this
round.


>
> > > >> > guess it's perhaps a tradeoff between predicting the future
> unknown
> > > >> > availability of functions versus just ensuring a minimum macOS
> > > >> > version
> > > >>
> > > >> and
> > > >>
> > > >> > hoping for the best. With any luck, the distinction between the
> two
> > > >> > approaches will be moot, if we try to assume that a future macOS
> > > >> > version
> > > >> > that removes this also provides mknodat.
> > > >> >
> > > >> > On Tue, Feb 8, 2022 at 10:03 AM Christian Schoenebeck <
> > > >> >
> > > >> > [e-mail]> wrote:
> > > >> > > On Dienstag, 8. Februar 2022 14:36:42 CET Will Cohen wrote:
> > > >> > > > On Mon, Feb 7, 2022 at 5:56 PM Christian Schoenebeck
> > > >> > > > <[e-mail]>
> > > >> > > >
> > > >> > > > wrote:
> > > >> > > > > On Montag, 7. Februar 2022 23:40:22 CET Will Cohen wrote:
> > > >> > > > > > From: Keno Fischer <[e-mail]>
> > > >> > > > > >
> > > >> > > > > > 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 <[e-mail]>
> > > >> > > > > > Signed-off-by: Michael Roitzsch <
> [e-mail]>
> > > >> > > > > > [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 <[e-mail]>
> > > >> > > > > > ---
> > > >> > > > >
> > > >> > > > > 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.
> > > >> > >
> > > >> > > If you embed that include into ifdefs, sure!
> > > >> > >
> > > >> > > #ifdef CONFIG_DARWIN
> > > >> > > /* defines API_AVAILABLE(...) */
> > > >> > > #include <os/availability.h>
> > > >> > > #endif
> > > >> > >
> > > >> > > One more thing though ...
> > > >> > >
> > > >> > > > > > +
> > > >> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t
> > > >> > > > > > mode,
> > > >> > > > > > dev_t
> > > >> > > > >
> > > >> > > > > dev)
> > > >> > > > >
> > > >> > > > > > +{
> > > >> > > > > > +    int preserved_errno, err;
> > > >> > >
> > > >> > > pthread_fchdir_np() is weakly linked. So I guess here should be
> a
> > > >>
> > > >> check
> > > >>
> > > >> > > like:
> > > >> > >         if (!pthread_fchdir_np) {
> > > >> > >
> > > >> > >                 return -ENOTSUPP;
> > > >> > >
> > > >> > >         }
> > > >> > >
> > > >> > > Before trying to call pthread_fchdir_np() below. As already
> > > >> > > discussed
> > > >>
> > > >> with
> > > >>
> > > >> > > the
> > > >> > > Chromium [1] example, some do that a bit differently by using
> > > >> > >
> > > >> > > __builtin_available():
> > > >> > >         if (__builtin_available(macOS 10.12, *)) {
> > > >> > >
> > > >> > >                 return -ENOTSUPP;
> > > >> > >
> > > >> > >         }
> > > >> > >
> > > >> > > Which makes me wonder why they are not doing a simple NULL
> check?
> > > >> > >
> > > >> > > [1]
> > > >>
> > > >>
> https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/laun
> > > >> ch
> > > >> _
> > > >>
> > > >> > > mac.cc#110>
> > > >> > >
> > > >> > > > > > +    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
> >
> > Best regards,
> > Christian Schoenebeck
>
>
> Best regards,
> Christian Schoenebeck
>
>
>

Akihiko Odaki Feb. 9, 2022, 1:33 p.m. UTC | #12

On Wed, Feb 9, 2022 at 7:57 AM Will Cohen <[e-mail]> wrote:
>
> On Tue, Feb 8, 2022 at 2:49 PM Christian Schoenebeck <[e-mail]> wrote:
>>
>> On Dienstag, 8. Februar 2022 19:28:21 CET Christian Schoenebeck wrote:
>> > On Dienstag, 8. Februar 2022 19:04:31 CET Will Cohen wrote:
>> > > On Tue, Feb 8, 2022 at 11:19 AM Will Cohen <[e-mail]> wrote:
>> > > > On Tue, Feb 8, 2022 at 11:11 AM Christian Schoenebeck <
>> > > >
>> > > > [e-mail]> wrote:
>> > > >> On Dienstag, 8. Februar 2022 16:57:55 CET Will Cohen wrote:
>> > > >> > My inclination is to go with the __builtin_available(macOS 10.12, *)
>> > > >>
>> > > >> path,
>> > > >>
>> > > >> > if acceptable, since it partially mirrors the API_AVAILABLE macro
>> > > >> > idea.
>> > > >>
>> > > >> I
>> > > >>
>> > > >> OTOH that's duplication of the ">= macOS 10.12" info, plus
>> > > >> __builtin_available
>> > > >> is direct use of a clang-only extension, whereas API_AVAILABLE() works
>> > > >> (or
>> > > >> more precisely: doesn't error out at least) with other compilers like
>> > > >> GCC
>> > > >> as
>> > > >> well. GCC is sometimes used for cross-compilation.
>> > > >>
>> > > >> Moreover, I would also add an error message in this case, e.g.:
>> > > >>     if (!pthread_fchdir_np) {
>> > > >>
>> > > >>         error_report_once("pthread_fchdir_np() is not available on this
>> > > >>
>> > > >> macOS version");
>> > > >>
>> > > >>         return -ENOTSUPP;
>> > > >>
>> > > >>     }
>> > > >>
>> > > >> I should elaborate why I think this is needed: you are already doing a
>> > > >> Meson
>> > > >> check for the existence of pthread_fchdir_np(), but the system where
>> > > >> QEMU
>> > > >> is
>> > > >> compiled and the systems where the compiled binary will be running,
>> > > >> might
>> > > >> be
>> > > >> different ones (i.e. different macOS versions).
>> > > >>
>> > > >> Best regards,
>> > > >> Christian Schoenebeck
>> > > >
>> > > > Agreed, that way actually closes the edge case. Something along these
>> > > > lines briefly crossed my mind during a previous version, but I quickly
>> > > > got
>> > > > passed it by assuming that the compiling entity would always be the
>> > > > bottleneck, which makes no sense in hindsight, so I very much appreciate
>> > > > that you caught this.
>> > >
>> > > Ah, rebuilding leads to a compiler error:
>> > >
>> > > ../os-posix.c:348:10: warning: address of function 'pthread_fchdir_np'
>> > > will
>> > > always evaluate to 'true' [-Wpointer-bool-conversion]
>> > >
>> > >     if (!pthread_fchdir_np) {
>> > >
>> > >         ~^~~~~~~~~~~~~~~~~
>> > >
>> > > I don't have a machine that's pre-10.12 so I can't see what the result is
>> > > there, but this might be why the __builtin_available approach got taken.
>> >
>> > I guess that's because you are compiling QEMU with minimum deployment target
>> > being macOS >= 10.12 already. In this case the compiler won't make
>> > pthread_fchdir_np a weak link, it only does emit a weak link if you are
>> > targeting macOS versions prior than the defined availablity attribute,
>> > hence the address would never be NULL here and hence the compiler warning.
>> >
>> > So I guess it is okay if you just omit checking presence of
>> > pthread_fchdir_np at runtime and just assume it exists.
>> >
>> > Added Akihiko on CC, just in case he would have something to add on this
>> > macOS issue here. :)

Hi, Thanks for CCing me.

>>
>> On a second thought: this case a bit special. Are we worried that
>> pthread_fchdir_np() is "not yet" available on macOS, or "no longer" available.
>> Probably both, right?
>>
>> So maybe it would make sense to replace the API_AVAILABLE() attribute directly
>> with a __attribute__((weak)) attribute. Then the runtime check with the
>> proposed error message would also trigger if a bleeding edge macOS version no
>> longer has pthread_fchdir_np().
>>
>> Also keep in mind: there are always also the MAC_OS_X_VERSION_MIN_REQUIRED and
>> MAC_OS_X_VERSION_MAX_ALLOWED macros to query the deployment target at compile
>> time to wrap deployment target dependent code accordingly.
>>
>> On doubt you could just make some tests there by simply "inventing" a non-
>> existent function.
>>
>> Best regards,
>> Christian Schoenebeck
>
>
> I like the idea of switching it to __attribute__((weak)). I should note that I'm not sure that I can actually fully test this out since I'm getting stuck with the linker noting my undefined fake function during the build, but the idea does make logical sense to me for the future fail case and the happy case builds again when implemented with actual pthread_fchdir_np:
>
> [1075/2909] Linking target qemu-nbd
> FAILED: qemu-nbd
> cc -m64 -mcx16  -o qemu-nbd qemu-nbd.p/qemu-nbd.c.o -Wl,-dead_strip_dylibs -Wl,-headerpad_max_install_names -Wl,-undefined,error -Wl,-force_load libblockdev.fa -Wl,-force_load libblock.fa -Wl,-force_load libcrypto.fa -Wl,-force_load libauthz.fa -Wl,-force_load libqom.fa -Wl,-force_load libio.fa -fstack-protector-strong -Wl,-rpath,/usr/local/Cellar/gnutls/3.7.3/lib -Wl,-rpath,/usr/local/Cellar/pixman/0.40.0/lib libqemuutil.a libblockdev.fa libblock.fa libcrypto.fa libauthz.fa libqom.fa libio.fa @block.syms /usr/local/Cellar/gnutls/3.7.3/lib/libgnutls.dylib -lutil -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl -lm -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib -lgmodule-2.0 -lglib-2.0 -lintl /usr/local/Cellar/pixman/0.40.0/lib/libpixman-1.dylib -lz -lxml2 -framework CoreFoundation -framework IOKit -lcurl -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib -lgmodule-2.0 -lglib-2.0 -lintl -lbz2 /usr/local/Cellar/libssh/0.9.6/lib/libssh.dylib -lpam
> Undefined symbols for architecture x86_64:
>   "_pthread_fchdir_npfoo", referenced from:
>       _qemu_mknodat in libblockdev.fa(os-posix.c.o)
> ld: symbol(s) not found for architecture x86_64
> clang: error: linker command failed with exit code 1 (use -v to see invocation)
> ninja: build stopped: subcommand failed.
> make[1]: *** [run-ninja] Error 1
> make: *** [all] Error 2
>
> With that caveat re testing in mind, unless there's another recommended path forward, I think it makes sense to stick with __attribute__((weak)) and prepare v6 which incorporates this and all the other feedback from this round.

__attribute__((weak_import)), which explicitly marks the function as
external, is more appropriate here. It is feature-equivalent with the
availability attribute when the minimum deployment target is lower
than the version which introduced the function.

Regards,
Akihiko Odaki

>
>>
>>
>> > > >> > guess it's perhaps a tradeoff between predicting the future unknown
>> > > >> > availability of functions versus just ensuring a minimum macOS
>> > > >> > version
>> > > >>
>> > > >> and
>> > > >>
>> > > >> > hoping for the best. With any luck, the distinction between the two
>> > > >> > approaches will be moot, if we try to assume that a future macOS
>> > > >> > version
>> > > >> > that removes this also provides mknodat.
>> > > >> >
>> > > >> > On Tue, Feb 8, 2022 at 10:03 AM Christian Schoenebeck <
>> > > >> >
>> > > >> > [e-mail]> wrote:
>> > > >> > > On Dienstag, 8. Februar 2022 14:36:42 CET Will Cohen wrote:
>> > > >> > > > On Mon, Feb 7, 2022 at 5:56 PM Christian Schoenebeck
>> > > >> > > > <[e-mail]>
>> > > >> > > >
>> > > >> > > > wrote:
>> > > >> > > > > On Montag, 7. Februar 2022 23:40:22 CET Will Cohen wrote:
>> > > >> > > > > > From: Keno Fischer <[e-mail]>
>> > > >> > > > > >
>> > > >> > > > > > 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 <[e-mail]>
>> > > >> > > > > > Signed-off-by: Michael Roitzsch <[e-mail]>
>> > > >> > > > > > [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 <[e-mail]>
>> > > >> > > > > > ---
>> > > >> > > > >
>> > > >> > > > > 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.
>> > > >> > >
>> > > >> > > If you embed that include into ifdefs, sure!
>> > > >> > >
>> > > >> > > #ifdef CONFIG_DARWIN
>> > > >> > > /* defines API_AVAILABLE(...) */
>> > > >> > > #include <os/availability.h>
>> > > >> > > #endif
>> > > >> > >
>> > > >> > > One more thing though ...
>> > > >> > >
>> > > >> > > > > > +
>> > > >> > > > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t
>> > > >> > > > > > mode,
>> > > >> > > > > > dev_t
>> > > >> > > > >
>> > > >> > > > > dev)
>> > > >> > > > >
>> > > >> > > > > > +{
>> > > >> > > > > > +    int preserved_errno, err;
>> > > >> > >
>> > > >> > > pthread_fchdir_np() is weakly linked. So I guess here should be a
>> > > >>
>> > > >> check
>> > > >>
>> > > >> > > like:
>> > > >> > >         if (!pthread_fchdir_np) {
>> > > >> > >
>> > > >> > >                 return -ENOTSUPP;
>> > > >> > >
>> > > >> > >         }
>> > > >> > >
>> > > >> > > Before trying to call pthread_fchdir_np() below. As already
>> > > >> > > discussed
>> > > >>
>> > > >> with
>> > > >>
>> > > >> > > the
>> > > >> > > Chromium [1] example, some do that a bit differently by using
>> > > >> > >
>> > > >> > > __builtin_available():
>> > > >> > >         if (__builtin_available(macOS 10.12, *)) {
>> > > >> > >
>> > > >> > >                 return -ENOTSUPP;
>> > > >> > >
>> > > >> > >         }
>> > > >> > >
>> > > >> > > Which makes me wonder why they are not doing a simple NULL check?
>> > > >> > >
>> > > >> > > [1]
>> > > >>
>> > > >> https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/laun
>> > > >> ch
>> > > >> _
>> > > >>
>> > > >> > > mac.cc#110>
>> > > >> > >
>> > > >> > > > > > +    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
>> >
>> > Best regards,
>> > Christian Schoenebeck
>>
>>
>> Best regards,
>> Christian Schoenebeck
>>
>>

Christian Schoenebeck Feb. 9, 2022, 1:50 p.m. UTC | #13

On Dienstag, 8. Februar 2022 23:57:32 CET Will Cohen wrote:
> > On a second thought: this case a bit special. Are we worried that
> > pthread_fchdir_np() is "not yet" available on macOS, or "no longer"
> > available.
> > Probably both, right?
> > 
> > So maybe it would make sense to replace the API_AVAILABLE() attribute
> > directly
> > with a __attribute__((weak)) attribute. Then the runtime check with the
> > proposed error message would also trigger if a bleeding edge macOS version
> > no
> > longer has pthread_fchdir_np().
> > 
> > Also keep in mind: there are always also the MAC_OS_X_VERSION_MIN_REQUIRED
> > and
> > MAC_OS_X_VERSION_MAX_ALLOWED macros to query the deployment target at
> > compile
> > time to wrap deployment target dependent code accordingly.
> > 
> > On doubt you could just make some tests there by simply "inventing" a non-
> > existent function.
> > 
> > Best regards,
> > Christian Schoenebeck
> 
> I like the idea of switching it to __attribute__((weak)). I should note
> that I'm not sure that I can actually fully test this out since I'm getting
> stuck with the linker noting my undefined fake function during the build,
> but the idea does make logical sense to me for the future fail case and the
> happy case builds again when implemented with actual pthread_fchdir_np:
> 
> [1075/2909] Linking target qemu-nbd
> FAILED: qemu-nbd
> cc -m64 -mcx16  -o qemu-nbd qemu-nbd.p/qemu-nbd.c.o -Wl,-dead_strip_dylibs
> -Wl,-headerpad_max_install_names -Wl,-undefined,error -Wl,-force_load
> libblockdev.fa -Wl,-force_load libblock.fa -Wl,-force_load libcrypto.fa
> -Wl,-force_load libauthz.fa -Wl,-force_load libqom.fa -Wl,-force_load
> libio.fa -fstack-protector-strong
> -Wl,-rpath,/usr/local/Cellar/gnutls/3.7.3/lib
> -Wl,-rpath,/usr/local/Cellar/pixman/0.40.0/lib libqemuutil.a libblockdev.fa
> libblock.fa libcrypto.fa libauthz.fa libqom.fa libio.fa @block.syms
> /usr/local/Cellar/gnutls/3.7.3/lib/libgnutls.dylib -lutil
> -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib -lgio-2.0
> -lgobject-2.0 -lglib-2.0 -lintl -L/usr/local/Cellar/glib/2.70.3/lib
> -L/usr/local/opt/gettext/lib -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl -lm
> -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> -lgmodule-2.0 -lglib-2.0 -lintl
> /usr/local/Cellar/pixman/0.40.0/lib/libpixman-1.dylib -lz -lxml2 -framework
> CoreFoundation -framework IOKit -lcurl -L/usr/local/Cellar/glib/2.70.3/lib
> -L/usr/local/opt/gettext/lib -lgmodule-2.0 -lglib-2.0 -lintl -lbz2
> /usr/local/Cellar/libssh/0.9.6/lib/libssh.dylib -lpam
> Undefined symbols for architecture x86_64:
>   "_pthread_fchdir_npfoo", referenced from:
>       _qemu_mknodat in libblockdev.fa(os-posix.c.o)
> ld: symbol(s) not found for architecture x86_64
> clang: error: linker command failed with exit code 1 (use -v to see

Even when "weak" linking, the respective symbol must still at least exist at 
compile time. To make this more clear, the following test works for me:

$ cat a.c
#include <stdio.h>

void aaa(void) {
    printf("aaa\n");
}
$ cat ab.c
#include <stdio.h>

void aaa(void) {
    printf("aaa\n");
}

void bbb(void) {
    printf("bbb\n");
}
$ cat main.c
#include <stdio.h>

void aaa(void);
void bbb(void) __attribute__((weak));

int main() {
    aaa();
    if (bbb)
        bbb();
    else
        printf("bbb() not supported\n");
    return 0;
}
$ clang -dynamiclib ab.c -o foo.1.dylib
$ clang main.c -o weaktest foo.1.dylib
$ ./weaktest
aaa
bbb
$ clang -dynamiclib a.c -o foo.1.dylib
$ ./weaktest
aaa
bbb() not supported
$

> With that caveat re testing in mind, unless there's another recommended
> path forward, I think it makes sense to stick with __attribute__((weak))
> and prepare v6 which incorporates this and all the other feedback from this
> round.

Just make this clear with an appropriate comment why it is weakly linked: to 
guard that function pthread_fchdir_np might simply disappear in future macOS 
versions, as it is simply a private API.

Best regards,
Christian Schoenebeck

Christian Schoenebeck Feb. 9, 2022, 2:08 p.m. UTC | #14

On Mittwoch, 9. Februar 2022 14:33:25 CET Akihiko Odaki wrote:
> > I like the idea of switching it to __attribute__((weak)). I should note
> > that I'm not sure that I can actually fully test this out since I'm
> > getting stuck with the linker noting my undefined fake function during
> > the build, but the idea does make logical sense to me for the future fail
> > case and the happy case builds again when implemented with actual
> > pthread_fchdir_np:
> > 
> > [1075/2909] Linking target qemu-nbd
> > FAILED: qemu-nbd
> > cc -m64 -mcx16  -o qemu-nbd qemu-nbd.p/qemu-nbd.c.o -Wl,-dead_strip_dylibs
> > -Wl,-headerpad_max_install_names -Wl,-undefined,error -Wl,-force_load
> > libblockdev.fa -Wl,-force_load libblock.fa -Wl,-force_load libcrypto.fa
> > -Wl,-force_load libauthz.fa -Wl,-force_load libqom.fa -Wl,-force_load
> > libio.fa -fstack-protector-strong
> > -Wl,-rpath,/usr/local/Cellar/gnutls/3.7.3/lib
> > -Wl,-rpath,/usr/local/Cellar/pixman/0.40.0/lib libqemuutil.a
> > libblockdev.fa libblock.fa libcrypto.fa libauthz.fa libqom.fa libio.fa
> > @block.syms /usr/local/Cellar/gnutls/3.7.3/lib/libgnutls.dylib -lutil
> > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> > -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl
> > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> > -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl -lm
> > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> > -lgmodule-2.0 -lglib-2.0 -lintl
> > /usr/local/Cellar/pixman/0.40.0/lib/libpixman-1.dylib -lz -lxml2
> > -framework CoreFoundation -framework IOKit -lcurl
> > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> > -lgmodule-2.0 -lglib-2.0 -lintl -lbz2
> > /usr/local/Cellar/libssh/0.9.6/lib/libssh.dylib -lpam> 
> > Undefined symbols for architecture x86_64:
> >   "_pthread_fchdir_npfoo", referenced from:
> >       _qemu_mknodat in libblockdev.fa(os-posix.c.o)
> > 
> > ld: symbol(s) not found for architecture x86_64
> > clang: error: linker command failed with exit code 1 (use -v to see
> > invocation) ninja: build stopped: subcommand failed.
> > make[1]: *** [run-ninja] Error 1
> > make: *** [all] Error 2
> > 
> > With that caveat re testing in mind, unless there's another recommended
> > path forward, I think it makes sense to stick with __attribute__((weak))
> > and prepare v6 which incorporates this and all the other feedback from
> > this round.
> __attribute__((weak_import)), which explicitly marks the function as
> external, is more appropriate here. It is feature-equivalent with the
> availability attribute when the minimum deployment target is lower
> than the version which introduced the function.

Thanks for chiming in on this macOS issue Akihiko!

Are you sure that "weak_import" is still the preferred way? From behaviour PoV 
I do not see any difference at all. I can't even tell what the intended 
difference was, and QEMU currently only seems to use "weak" in the entire code 
base.

Googling around, "weak_import" seems to be required on ancient OS X versions 
only and that it's now deprecated in favour of the more common "weak", no?

Best regards,
Christian Schoenebeck

Will Cohen Feb. 9, 2022, 6:20 p.m. UTC | #15

On Wed, Feb 9, 2022 at 9:08 AM Christian Schoenebeck <[e-mail]>
wrote:

> On Mittwoch, 9. Februar 2022 14:33:25 CET Akihiko Odaki wrote:
> > > I like the idea of switching it to __attribute__((weak)). I should note
> > > that I'm not sure that I can actually fully test this out since I'm
> > > getting stuck with the linker noting my undefined fake function during
> > > the build, but the idea does make logical sense to me for the future
> fail
> > > case and the happy case builds again when implemented with actual
> > > pthread_fchdir_np:
> > >
> > > [1075/2909] Linking target qemu-nbd
> > > FAILED: qemu-nbd
> > > cc -m64 -mcx16  -o qemu-nbd qemu-nbd.p/qemu-nbd.c.o
> -Wl,-dead_strip_dylibs
> > > -Wl,-headerpad_max_install_names -Wl,-undefined,error -Wl,-force_load
> > > libblockdev.fa -Wl,-force_load libblock.fa -Wl,-force_load libcrypto.fa
> > > -Wl,-force_load libauthz.fa -Wl,-force_load libqom.fa -Wl,-force_load
> > > libio.fa -fstack-protector-strong
> > > -Wl,-rpath,/usr/local/Cellar/gnutls/3.7.3/lib
> > > -Wl,-rpath,/usr/local/Cellar/pixman/0.40.0/lib libqemuutil.a
> > > libblockdev.fa libblock.fa libcrypto.fa libauthz.fa libqom.fa libio.fa
> > > @block.syms /usr/local/Cellar/gnutls/3.7.3/lib/libgnutls.dylib -lutil
> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> > > -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl
> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> > > -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl -lm
> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> > > -lgmodule-2.0 -lglib-2.0 -lintl
> > > /usr/local/Cellar/pixman/0.40.0/lib/libpixman-1.dylib -lz -lxml2
> > > -framework CoreFoundation -framework IOKit -lcurl
> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> > > -lgmodule-2.0 -lglib-2.0 -lintl -lbz2
> > > /usr/local/Cellar/libssh/0.9.6/lib/libssh.dylib -lpam>
> > > Undefined symbols for architecture x86_64:
> > >   "_pthread_fchdir_npfoo", referenced from:
> > >       _qemu_mknodat in libblockdev.fa(os-posix.c.o)
> > >
> > > ld: symbol(s) not found for architecture x86_64
> > > clang: error: linker command failed with exit code 1 (use -v to see
> > > invocation) ninja: build stopped: subcommand failed.
> > > make[1]: *** [run-ninja] Error 1
> > > make: *** [all] Error 2
> > >
> > > With that caveat re testing in mind, unless there's another recommended
> > > path forward, I think it makes sense to stick with
> __attribute__((weak))
> > > and prepare v6 which incorporates this and all the other feedback from
> > > this round.
> > __attribute__((weak_import)), which explicitly marks the function as
> > external, is more appropriate here. It is feature-equivalent with the
> > availability attribute when the minimum deployment target is lower
> > than the version which introduced the function.
>
> Thanks for chiming in on this macOS issue Akihiko!
>
> Are you sure that "weak_import" is still the preferred way? From behaviour
> PoV
> I do not see any difference at all. I can't even tell what the intended
> difference was, and QEMU currently only seems to use "weak" in the entire
> code
> base.
>
> Googling around, "weak_import" seems to be required on ancient OS X
> versions
> only and that it's now deprecated in favour of the more common "weak", no?
>
> Best regards,
> Christian Schoenebeck
>

Either way seems reasonable to me. For reference, what I'm seeing on Google
and what Christian may be referring to is a circa-2016 conversation on GCC
bugzilla, where a tentative conclusion seems to be that the distinction
between the two is small and weak is probably preferred now:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69179

Akihiko Odaki Feb. 9, 2022, 11:10 p.m. UTC | #16

On Thu, Feb 10, 2022 at 3:20 AM Will Cohen <[e-mail]> wrote:
>
> On Wed, Feb 9, 2022 at 9:08 AM Christian Schoenebeck <[e-mail]> wrote:
>>
>> On Mittwoch, 9. Februar 2022 14:33:25 CET Akihiko Odaki wrote:
>> > > I like the idea of switching it to __attribute__((weak)). I should note
>> > > that I'm not sure that I can actually fully test this out since I'm
>> > > getting stuck with the linker noting my undefined fake function during
>> > > the build, but the idea does make logical sense to me for the future fail
>> > > case and the happy case builds again when implemented with actual
>> > > pthread_fchdir_np:
>> > >
>> > > [1075/2909] Linking target qemu-nbd
>> > > FAILED: qemu-nbd
>> > > cc -m64 -mcx16  -o qemu-nbd qemu-nbd.p/qemu-nbd.c.o -Wl,-dead_strip_dylibs
>> > > -Wl,-headerpad_max_install_names -Wl,-undefined,error -Wl,-force_load
>> > > libblockdev.fa -Wl,-force_load libblock.fa -Wl,-force_load libcrypto.fa
>> > > -Wl,-force_load libauthz.fa -Wl,-force_load libqom.fa -Wl,-force_load
>> > > libio.fa -fstack-protector-strong
>> > > -Wl,-rpath,/usr/local/Cellar/gnutls/3.7.3/lib
>> > > -Wl,-rpath,/usr/local/Cellar/pixman/0.40.0/lib libqemuutil.a
>> > > libblockdev.fa libblock.fa libcrypto.fa libauthz.fa libqom.fa libio.fa
>> > > @block.syms /usr/local/Cellar/gnutls/3.7.3/lib/libgnutls.dylib -lutil
>> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
>> > > -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl
>> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
>> > > -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl -lm
>> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
>> > > -lgmodule-2.0 -lglib-2.0 -lintl
>> > > /usr/local/Cellar/pixman/0.40.0/lib/libpixman-1.dylib -lz -lxml2
>> > > -framework CoreFoundation -framework IOKit -lcurl
>> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
>> > > -lgmodule-2.0 -lglib-2.0 -lintl -lbz2
>> > > /usr/local/Cellar/libssh/0.9.6/lib/libssh.dylib -lpam>
>> > > Undefined symbols for architecture x86_64:
>> > >   "_pthread_fchdir_npfoo", referenced from:
>> > >       _qemu_mknodat in libblockdev.fa(os-posix.c.o)
>> > >
>> > > ld: symbol(s) not found for architecture x86_64
>> > > clang: error: linker command failed with exit code 1 (use -v to see
>> > > invocation) ninja: build stopped: subcommand failed.
>> > > make[1]: *** [run-ninja] Error 1
>> > > make: *** [all] Error 2
>> > >
>> > > With that caveat re testing in mind, unless there's another recommended
>> > > path forward, I think it makes sense to stick with __attribute__((weak))
>> > > and prepare v6 which incorporates this and all the other feedback from
>> > > this round.
>> > __attribute__((weak_import)), which explicitly marks the function as
>> > external, is more appropriate here. It is feature-equivalent with the
>> > availability attribute when the minimum deployment target is lower
>> > than the version which introduced the function.
>>
>> Thanks for chiming in on this macOS issue Akihiko!
>>
>> Are you sure that "weak_import" is still the preferred way? From behaviour PoV
>> I do not see any difference at all. I can't even tell what the intended
>> difference was, and QEMU currently only seems to use "weak" in the entire code
>> base.
>>
>> Googling around, "weak_import" seems to be required on ancient OS X versions
>> only and that it's now deprecated in favour of the more common "weak", no?
>>
>> Best regards,
>> Christian Schoenebeck
>
>
> Either way seems reasonable to me. For reference, what I'm seeing on Google and what Christian may be referring to is a circa-2016 conversation on GCC bugzilla, where a tentative conclusion seems to be that the distinction between the two is small and weak is probably preferred now: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69179
>

GCC doesn't maintain features specific to Apple well so we should look
at clang. Compiling QEMU for macOS with GCC would result in errors
anyway because QEMU uses clang extensions like availability checks and
blocks for Apple's ABIs/APIs. clang still distinguishes
__attribute__((weak)) and __attribute__((weak_import)).

The present uses of __attribute__((weak)) in QEMU are correct and
shouldn't be replaced with __attribute__((weak_import)) even when
targeting macOS since they have default implementations and are
statically resolved.

Regards,
Akihiko Odaki

Will Cohen Feb. 10, 2022, 3:46 p.m. UTC | #17

On Wed, Feb 9, 2022 at 6:10 PM Akihiko Odaki <[e-mail]>
wrote:

> On Thu, Feb 10, 2022 at 3:20 AM Will Cohen <[e-mail]> wrote:
> >
> > On Wed, Feb 9, 2022 at 9:08 AM Christian Schoenebeck <
> [e-mail]> wrote:
> >>
> >> On Mittwoch, 9. Februar 2022 14:33:25 CET Akihiko Odaki wrote:
> >> > > I like the idea of switching it to __attribute__((weak)). I should
> note
> >> > > that I'm not sure that I can actually fully test this out since I'm
> >> > > getting stuck with the linker noting my undefined fake function
> during
> >> > > the build, but the idea does make logical sense to me for the
> future fail
> >> > > case and the happy case builds again when implemented with actual
> >> > > pthread_fchdir_np:
> >> > >
> >> > > [1075/2909] Linking target qemu-nbd
> >> > > FAILED: qemu-nbd
> >> > > cc -m64 -mcx16  -o qemu-nbd qemu-nbd.p/qemu-nbd.c.o
> -Wl,-dead_strip_dylibs
> >> > > -Wl,-headerpad_max_install_names -Wl,-undefined,error
> -Wl,-force_load
> >> > > libblockdev.fa -Wl,-force_load libblock.fa -Wl,-force_load
> libcrypto.fa
> >> > > -Wl,-force_load libauthz.fa -Wl,-force_load libqom.fa
> -Wl,-force_load
> >> > > libio.fa -fstack-protector-strong
> >> > > -Wl,-rpath,/usr/local/Cellar/gnutls/3.7.3/lib
> >> > > -Wl,-rpath,/usr/local/Cellar/pixman/0.40.0/lib libqemuutil.a
> >> > > libblockdev.fa libblock.fa libcrypto.fa libauthz.fa libqom.fa
> libio.fa
> >> > > @block.syms /usr/local/Cellar/gnutls/3.7.3/lib/libgnutls.dylib
> -lutil
> >> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> >> > > -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl
> >> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> >> > > -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl -lm
> >> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> >> > > -lgmodule-2.0 -lglib-2.0 -lintl
> >> > > /usr/local/Cellar/pixman/0.40.0/lib/libpixman-1.dylib -lz -lxml2
> >> > > -framework CoreFoundation -framework IOKit -lcurl
> >> > > -L/usr/local/Cellar/glib/2.70.3/lib -L/usr/local/opt/gettext/lib
> >> > > -lgmodule-2.0 -lglib-2.0 -lintl -lbz2
> >> > > /usr/local/Cellar/libssh/0.9.6/lib/libssh.dylib -lpam>
> >> > > Undefined symbols for architecture x86_64:
> >> > >   "_pthread_fchdir_npfoo", referenced from:
> >> > >       _qemu_mknodat in libblockdev.fa(os-posix.c.o)
> >> > >
> >> > > ld: symbol(s) not found for architecture x86_64
> >> > > clang: error: linker command failed with exit code 1 (use -v to see
> >> > > invocation) ninja: build stopped: subcommand failed.
> >> > > make[1]: *** [run-ninja] Error 1
> >> > > make: *** [all] Error 2
> >> > >
> >> > > With that caveat re testing in mind, unless there's another
> recommended
> >> > > path forward, I think it makes sense to stick with
> __attribute__((weak))
> >> > > and prepare v6 which incorporates this and all the other feedback
> from
> >> > > this round.
> >> > __attribute__((weak_import)), which explicitly marks the function as
> >> > external, is more appropriate here. It is feature-equivalent with the
> >> > availability attribute when the minimum deployment target is lower
> >> > than the version which introduced the function.
> >>
> >> Thanks for chiming in on this macOS issue Akihiko!
> >>
> >> Are you sure that "weak_import" is still the preferred way? From
> behaviour PoV
> >> I do not see any difference at all. I can't even tell what the intended
> >> difference was, and QEMU currently only seems to use "weak" in the
> entire code
> >> base.
> >>
> >> Googling around, "weak_import" seems to be required on ancient OS X
> versions
> >> only and that it's now deprecated in favour of the more common "weak",
> no?
> >>
> >> Best regards,
> >> Christian Schoenebeck
> >
> >
> > Either way seems reasonable to me. For reference, what I'm seeing on
> Google and what Christian may be referring to is a circa-2016 conversation
> on GCC bugzilla, where a tentative conclusion seems to be that the
> distinction between the two is small and weak is probably preferred now:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69179
> >
>
> GCC doesn't maintain features specific to Apple well so we should look
> at clang. Compiling QEMU for macOS with GCC would result in errors
> anyway because QEMU uses clang extensions like availability checks and
> blocks for Apple's ABIs/APIs. clang still distinguishes
> __attribute__((weak)) and __attribute__((weak_import)).
>
> The present uses of __attribute__((weak)) in QEMU are correct and
> shouldn't be replaced with __attribute__((weak_import)) even when
> targeting macOS since they have default implementations and are
> statically resolved.
>
> Regards,
> Akihiko Odaki
>

Noted. v6 adjusts to use weak_import. Many thanks!

Patch

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
+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));
+
+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