diff mbox series

[v4,1/6] 9pfs: fix qemu_mknodat(S_IFREG) on macOS

Message ID a5338eada3c5130046785014c185ec4fa0ddeaa7.1651085921.git.qemu_oss@crudebyte.com (mailing list archive)
State New, archived
Headers show
Series 9pfs: macOS host fixes | expand

Commit Message

Christian Schoenebeck April 27, 2022, 6:54 p.m. UTC
mknod() on macOS does not support creating regular files, so
divert to openat_file() if S_IFREG is passed with mode argument.

Furthermore, 'man 2 mknodat' on Linux says: "Zero file type is
equivalent to type S_IFREG".

Link: https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Reviewed-by: Will Cohen <wwcohen@gmail.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-util-darwin.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Greg Kurz April 27, 2022, 8:16 p.m. UTC | #1
On Wed, 27 Apr 2022 20:54:04 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> mknod() on macOS does not support creating regular files, so
> divert to openat_file() if S_IFREG is passed with mode argument.
> 
> Furthermore, 'man 2 mknodat' on Linux says: "Zero file type is
> equivalent to type S_IFREG".
> 

Thinking again I have mixed feelings about this... qemu_mknodat()
should certainly match POSIX semantics, even non-portable, as
described in [1] but I'm not sure it should mimic linux-specific
behaviors.

[1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/mknod.html

> Link: https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> Reviewed-by: Will Cohen <wwcohen@gmail.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-util-darwin.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
> index bec0253474..e24d09763a 100644
> --- a/hw/9pfs/9p-util-darwin.c
> +++ b/hw/9pfs/9p-util-darwin.c
> @@ -77,6 +77,15 @@ int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
>  int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
>  {
>      int preserved_errno, err;
> +
> +    if (S_ISREG(mode) || !(mode & S_IFMT)) {

... so maybe I'd just check S_ISREG() here. Not a request, just food
for thought : sticking to POSIX semantics might help to make the code
more portable across all the new host supports that are showing up
these days.

> +        int fd = openat_file(dirfd, filename, O_CREAT, mode);
> +        if (fd == -1) {
> +            return fd;
> +        }
> +        close(fd);
> +        return 0;
> +    }
>      if (!pthread_fchdir_np) {
>          error_report_once("pthread_fchdir_np() not available on this version of macOS");
>          return -ENOTSUP;
Christian Schoenebeck April 28, 2022, 11:42 a.m. UTC | #2
On Mittwoch, 27. April 2022 22:16:12 CEST Greg Kurz wrote:
> On Wed, 27 Apr 2022 20:54:04 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > mknod() on macOS does not support creating regular files, so
> > divert to openat_file() if S_IFREG is passed with mode argument.
> > 
> > Furthermore, 'man 2 mknodat' on Linux says: "Zero file type is
> > equivalent to type S_IFREG".
> 
> Thinking again I have mixed feelings about this... qemu_mknodat()
> should certainly match POSIX semantics, even non-portable, as
> described in [1] but I'm not sure it should mimic linux-specific
> behaviors.
> 
> [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/mknod.html
> 
> > Link: https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > Reviewed-by: Will Cohen <wwcohen@gmail.com>
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> > ---
> > 
> >  hw/9pfs/9p-util-darwin.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
> > index bec0253474..e24d09763a 100644
> > --- a/hw/9pfs/9p-util-darwin.c
> > +++ b/hw/9pfs/9p-util-darwin.c
> > @@ -77,6 +77,15 @@ int fsetxattrat_nofollow(int dirfd, const char
> > *filename, const char *name,> 
> >  int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
> >  {
> >  
> >      int preserved_errno, err;
> > 
> > +
> > +    if (S_ISREG(mode) || !(mode & S_IFMT)) {
> 
> ... so maybe I'd just check S_ISREG() here. Not a request, just food
> for thought : sticking to POSIX semantics might help to make the code
> more portable across all the new host supports that are showing up
> these days.

Well, handling !(mode & S_IFMT) (i.e. "zero file type") is needed "somewhere" 
for 9p2000.L to behave as expected on macOS hosts. So you might argue this 
could rather be handled in 9p.c instead and there only if protocol is exactly 
9p2000.L.

OTOH I currently don't see an issue here providing that default behaviour.

Best regards,
Christian Schoenebeck

> > +        int fd = openat_file(dirfd, filename, O_CREAT, mode);
> > +        if (fd == -1) {
> > +            return fd;
> > +        }
> > +        close(fd);
> > +        return 0;
> > +    }
> > 
> >      if (!pthread_fchdir_np) {
> >      
> >          error_report_once("pthread_fchdir_np() not available on this
> >          version of macOS"); return -ENOTSUP;
diff mbox series

Patch

diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
index bec0253474..e24d09763a 100644
--- a/hw/9pfs/9p-util-darwin.c
+++ b/hw/9pfs/9p-util-darwin.c
@@ -77,6 +77,15 @@  int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
 int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
 {
     int preserved_errno, err;
+
+    if (S_ISREG(mode) || !(mode & S_IFMT)) {
+        int fd = openat_file(dirfd, filename, O_CREAT, mode);
+        if (fd == -1) {
+            return fd;
+        }
+        close(fd);
+        return 0;
+    }
     if (!pthread_fchdir_np) {
         error_report_once("pthread_fchdir_np() not available on this version of macOS");
         return -ENOTSUP;