diff mbox series

[09/16] hw/9pfs: Disable unsupported flags and features for Windows

Message ID 20221024045759.448014-10-bin.meng@windriver.com
State New
Headers show
Series hw/9pfs: Add 9pfs support for Windows | expand

Commit Message

Bin Meng Oct. 24, 2022, 4:57 a.m. UTC 
From: Guohuai Shi <guohuai.shi@windriver.com>

Some flags and features are not supported on Windows, like mknod,
readlink, file mode, etc. Update the codes for Windows.

Signed-off-by: Guohuai Shi <guohuai.shi@windriver.com>
Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 hw/9pfs/9p-util.h |  6 +++-
 hw/9pfs/9p.c      | 90 ++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 86 insertions(+), 10 deletions(-)

Comments

Christian Schoenebeck Nov. 1, 2022, 3:04 p.m. UTC | #1 
On Monday, October 24, 2022 6:57:52 AM CET Bin Meng wrote:
> From: Guohuai Shi <guohuai.shi@windriver.com>
> 
> Some flags and features are not supported on Windows, like mknod,
> readlink, file mode, etc. Update the codes for Windows.
> 
> Signed-off-by: Guohuai Shi <guohuai.shi@windriver.com>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
>  hw/9pfs/9p-util.h |  6 +++-
>  hw/9pfs/9p.c      | 90 ++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 86 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
> index 82b2d0c3e4..3d154e9103 100644
> --- a/hw/9pfs/9p-util.h
> +++ b/hw/9pfs/9p-util.h
> @@ -53,8 +53,10 @@ static inline uint64_t makedev_dotl(uint32_t dev_major, uint32_t dev_minor)
>   */
>  static inline uint64_t host_dev_to_dotl_dev(dev_t dev)
>  {
> -#ifdef CONFIG_LINUX
> +#if defined(CONFIG_LINUX)
>      return dev;
> +#elif defined(CONFIG_WIN32)
> +    return 0;

Really?

>  #else
>      return makedev_dotl(major(dev), minor(dev));
>  #endif
> @@ -260,7 +262,9 @@ static inline struct dirent *qemu_dirent_dup(struct dirent *dent)
>  #if defined CONFIG_DARWIN && defined CONFIG_PTHREAD_FCHDIR_NP
>  int pthread_fchdir_np(int fd) __attribute__((weak_import));
>  #endif
> +#ifndef CONFIG_WIN32
>  int qemu_mknodat(P9_FILE_ID dirfd, const char *filename, mode_t mode,
>                   dev_t dev);
> +#endif
>  
>  #endif
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 6c4af86240..771aab34ac 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -39,6 +39,11 @@
>  #include "qemu/xxhash.h"
>  #include <math.h>
>  
> +#ifdef CONFIG_WIN32
> +#define UTIME_NOW   ((1l << 30) - 1l)
> +#define UTIME_OMIT  ((1l << 30) - 2l)
> +#endif
> +
>  int open_fd_hw;
>  int total_open_fd;
>  static int open_fd_rc;
> @@ -132,13 +137,17 @@ static int dotl_to_open_flags(int flags)
>      DotlOpenflagMap dotl_oflag_map[] = {
>          { P9_DOTL_CREATE, O_CREAT },
>          { P9_DOTL_EXCL, O_EXCL },
> +#ifndef CONFIG_WIN32
>          { P9_DOTL_NOCTTY , O_NOCTTY },
> +#endif
>          { P9_DOTL_TRUNC, O_TRUNC },
>          { P9_DOTL_APPEND, O_APPEND },
> +#ifndef CONFIG_WIN32
>          { P9_DOTL_NONBLOCK, O_NONBLOCK } ,
>          { P9_DOTL_DSYNC, O_DSYNC },
>          { P9_DOTL_FASYNC, FASYNC },
> -#ifndef CONFIG_DARWIN
> +#endif
> +#ifdef CONFIG_LINUX

Better

   #if !defined(CONFIG_DARWIN) && !defined(CONFIG_WIN32)

Otherwise it might automatically opt-out other future platforms
unintentionally.

>          { P9_DOTL_NOATIME, O_NOATIME },
>          /*
>           *  On Darwin, we could map to F_NOCACHE, which is
> @@ -151,8 +160,10 @@ static int dotl_to_open_flags(int flags)
>  #endif
>          { P9_DOTL_LARGEFILE, O_LARGEFILE },
>          { P9_DOTL_DIRECTORY, O_DIRECTORY },
> +#ifndef CONFIG_WIN32
>          { P9_DOTL_NOFOLLOW, O_NOFOLLOW },
>          { P9_DOTL_SYNC, O_SYNC },
> +#endif
>      };
>  
>      for (i = 0; i < ARRAY_SIZE(dotl_oflag_map); i++) {
> @@ -179,8 +190,11 @@ static int get_dotl_openflags(V9fsState *s, int oflags)
>       * Filter the client open flags
>       */
>      flags = dotl_to_open_flags(oflags);
> -    flags &= ~(O_NOCTTY | O_ASYNC | O_CREAT);
> -#ifndef CONFIG_DARWIN
> +    flags &= ~(O_CREAT);
> +#ifndef CONFIG_WIN32
> +    flags &= ~(O_NOCTTY | O_ASYNC);
> +#endif
> +#ifdef CONFIG_LINUX

Same as above: better explicitly opt-out than the other way around.

>      /*
>       * Ignore direct disk access hint until the server supports it.
>       */
> @@ -986,9 +1000,11 @@ static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
>      if (S_ISDIR(stbuf->st_mode)) {
>          qidp->type |= P9_QID_TYPE_DIR;
>      }
> +#ifndef CONFIG_WIN32
>      if (S_ISLNK(stbuf->st_mode)) {
>          qidp->type |= P9_QID_TYPE_SYMLINK;
>      }
> +#endif
>  
>      return 0;
>  }
> @@ -1097,6 +1113,7 @@ static mode_t v9mode_to_mode(uint32_t mode, V9fsString *extension)
>          ret |= S_IFDIR;
>      }
>  
> +#ifndef CONFIG_WIN32
>      if (mode & P9_STAT_MODE_SYMLINK) {
>          ret |= S_IFLNK;
>      }
> @@ -1106,6 +1123,7 @@ static mode_t v9mode_to_mode(uint32_t mode, V9fsString *extension)
>      if (mode & P9_STAT_MODE_NAMED_PIPE) {
>          ret |= S_IFIFO;
>      }
> +#endif
>      if (mode & P9_STAT_MODE_DEVICE) {
>          if (extension->size && extension->data[0] == 'c') {
>              ret |= S_IFCHR;
> @@ -1118,6 +1136,7 @@ static mode_t v9mode_to_mode(uint32_t mode, V9fsString *extension)
>          ret |= S_IFREG;
>      }
>  
> +#ifndef CONFIG_WIN32
>      if (mode & P9_STAT_MODE_SETUID) {
>          ret |= S_ISUID;
>      }
> @@ -1127,6 +1146,7 @@ static mode_t v9mode_to_mode(uint32_t mode, V9fsString *extension)
>      if (mode & P9_STAT_MODE_SETVTX) {
>          ret |= S_ISVTX;
>      }
> +#endif
>  
>      return ret;
>  }
> @@ -1182,6 +1202,7 @@ static uint32_t stat_to_v9mode(const struct stat *stbuf)
>          mode |= P9_STAT_MODE_DIR;
>      }
>  
> +#ifndef CONFIG_WIN32
>      if (S_ISLNK(stbuf->st_mode)) {
>          mode |= P9_STAT_MODE_SYMLINK;
>      }
> @@ -1193,11 +1214,13 @@ static uint32_t stat_to_v9mode(const struct stat *stbuf)
>      if (S_ISFIFO(stbuf->st_mode)) {
>          mode |= P9_STAT_MODE_NAMED_PIPE;
>      }
> +#endif
>  
>      if (S_ISBLK(stbuf->st_mode) || S_ISCHR(stbuf->st_mode)) {
>          mode |= P9_STAT_MODE_DEVICE;
>      }
>  
> +#ifndef CONFIG_WIN32
>      if (stbuf->st_mode & S_ISUID) {
>          mode |= P9_STAT_MODE_SETUID;
>      }
> @@ -1209,6 +1232,7 @@ static uint32_t stat_to_v9mode(const struct stat *stbuf)
>      if (stbuf->st_mode & S_ISVTX) {
>          mode |= P9_STAT_MODE_SETVTX;
>      }
> +#endif
>  
>      return mode;
>  }
> @@ -1247,9 +1271,17 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, V9fsPath *path,
>              return err;
>          }
>      } else if (v9stat->mode & P9_STAT_MODE_DEVICE) {
> +        unsigned maj, min;
> +
> +#ifndef CONFIG_WIN32
> +        maj = major(stbuf->st_rdev);
> +        min = minor(stbuf->st_rdev);
> +#else
> +        maj = min = 0;
> +#endif

Really?

>          v9fs_string_sprintf(&v9stat->extension, "%c %u %u",
>                  S_ISCHR(stbuf->st_mode) ? 'c' : 'b',
> -                major(stbuf->st_rdev), minor(stbuf->st_rdev));
> +                maj, min);
>      } else if (S_ISDIR(stbuf->st_mode) || S_ISREG(stbuf->st_mode)) {
>          v9fs_string_sprintf(&v9stat->extension, "%s %lu",
>                  "HARDLINKCOUNT", (unsigned long)stbuf->st_nlink);
> @@ -1317,7 +1349,14 @@ static int32_t blksize_to_iounit(const V9fsPDU *pdu, int32_t blksize)
>  
>  static int32_t stat_to_iounit(const V9fsPDU *pdu, const struct stat *stbuf)
>  {
> -    return blksize_to_iounit(pdu, stbuf->st_blksize);
> +    int32_t blksize;
> +
> +#ifndef CONFIG_WIN32
> +    blksize = stbuf->st_blksize);
> +#else
> +    blksize = 0;
> +#endif

Really?

> +    return blksize_to_iounit(pdu, blksize);
>  }
>  
>  static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
> @@ -1332,7 +1371,11 @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
>      v9lstat->st_rdev = host_dev_to_dotl_dev(stbuf->st_rdev);
>      v9lstat->st_size = stbuf->st_size;
>      v9lstat->st_blksize = stat_to_iounit(pdu, stbuf);
> +#ifndef CONFIG_WIN32
>      v9lstat->st_blocks = stbuf->st_blocks;
> +#else
> +    v9lstat->st_blocks = 0;
> +#endif

Really?

>      v9lstat->st_atime_sec = stbuf->st_atime;
>      v9lstat->st_mtime_sec = stbuf->st_mtime;
>      v9lstat->st_ctime_sec = stbuf->st_ctime;
> @@ -1340,7 +1383,8 @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
>      v9lstat->st_atime_nsec = stbuf->st_atimespec.tv_nsec;
>      v9lstat->st_mtime_nsec = stbuf->st_mtimespec.tv_nsec;
>      v9lstat->st_ctime_nsec = stbuf->st_ctimespec.tv_nsec;
> -#else
> +#endif
> +#ifdef CONFIG_LINUX
>      v9lstat->st_atime_nsec = stbuf->st_atim.tv_nsec;
>      v9lstat->st_mtime_nsec = stbuf->st_mtim.tv_nsec;
>      v9lstat->st_ctime_nsec = stbuf->st_ctim.tv_nsec;
> @@ -2471,6 +2515,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
>      struct dirent *dent;
>      struct stat *st;
>      struct V9fsDirEnt *entries = NULL;
> +    unsigned char d_type = 0;
>  
>      /*
>       * inode remapping requires the device id, which in turn might be
> @@ -2540,10 +2585,13 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
>          v9fs_string_init(&name);
>          v9fs_string_sprintf(&name, "%s", dent->d_name);
>  
> +#ifndef CONFIG_WIN32
> +        d_type = dent->d_type;
> +#endif
>          /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
>          len = pdu_marshal(pdu, 11 + count, "Qqbs",
>                            &qid, off,
> -                          dent->d_type, &name);
> +                          d_type, &name);

Are you saying that d_type is not initialized with zero already?

>          v9fs_string_free(&name);
>  
> @@ -2873,8 +2921,12 @@ static void coroutine_fn v9fs_create(void *opaque)
>          }
>  
>          nmode |= perm & 0777;
> +#ifndef CONFIG_WIN32
>          err = v9fs_co_mknod(pdu, fidp, &name, fidp->uid, -1,
>                              makedev(major, minor), nmode, &stbuf);
> +#else
> +        err = -ENOTSUP;
> +#endif
>          if (err < 0) {
>              goto out;
>          }
> @@ -2899,8 +2951,12 @@ static void coroutine_fn v9fs_create(void *opaque)
>          v9fs_path_copy(&fidp->path, &path);
>          v9fs_path_unlock(s);
>      } else if (perm & P9_STAT_MODE_SOCKET) {
> +#ifndef CONFIG_WIN32
>          err = v9fs_co_mknod(pdu, fidp, &name, fidp->uid, -1,
>                              0, S_IFSOCK | (perm & 0777), &stbuf);
> +#else
> +        err = -ENOTSUP;
> +#endif

As with previous patches, I would consider making the user aware to use
mapped(-xattr) with something like error_report_once().

>          if (err < 0) {
>              goto out;
>          }
> @@ -3634,7 +3690,7 @@ out_nofid:
>  
>  static void coroutine_fn v9fs_mknod(void *opaque)
>  {
> -
> +#ifndef CONFIG_WIN32
>      int mode;
>      gid_t gid;
>      int32_t fid;
> @@ -3691,6 +3747,10 @@ out:
>  out_nofid:
>      pdu_complete(pdu, err);
>      v9fs_string_free(&name);
> +#else
> +    V9fsPDU *pdu = opaque;
> +    pdu_complete(pdu, -1);
> +#endif
>  }
>  
>  /*
> @@ -3963,7 +4023,7 @@ out_nofid:
>  #if defined(CONFIG_LINUX)
>  /* Currently, only Linux has XATTR_SIZE_MAX */
>  #define P9_XATTR_SIZE_MAX XATTR_SIZE_MAX
> -#elif defined(CONFIG_DARWIN)
> +#elif defined(CONFIG_DARWIN) || defined(CONFIG_WIN32)
>  /*
>   * Darwin doesn't seem to define a maximum xattr size in its user
>   * space header, so manually configure it across platforms as 64k.
> @@ -3980,6 +4040,7 @@ out_nofid:
>  
>  static void coroutine_fn v9fs_xattrcreate(void *opaque)
>  {
> +#ifndef CONFIG_WIN32
>      int flags, rflags = 0;
>      int32_t fid;
>      uint64_t size;
> @@ -4041,10 +4102,15 @@ out_put_fid:
>  out_nofid:
>      pdu_complete(pdu, err);
>      v9fs_string_free(&name);
> +#else
> +    V9fsPDU *pdu = opaque;
> +    pdu_complete(pdu, -1);
> +#endif
>  }
>  
>  static void coroutine_fn v9fs_readlink(void *opaque)
>  {
> +#ifndef CONFIG_WIN32
>      V9fsPDU *pdu = opaque;
>      size_t offset = 7;
>      V9fsString target;
> @@ -4080,6 +4146,10 @@ out:
>      put_fid(pdu, fidp);
>  out_nofid:
>      pdu_complete(pdu, err);
> +#else
> +    V9fsPDU *pdu = opaque;
> +    pdu_complete(pdu, -1);
> +#endif

Unnecessary double declaration of pdu.

>  }
>  
>  static CoroutineEntry *pdu_co_handlers[] = {
> @@ -4341,6 +4411,7 @@ void v9fs_reset(V9fsState *s)
>  
>  static void __attribute__((__constructor__)) v9fs_set_fd_limit(void)
>  {
> +#ifndef CONFIG_WIN32
>      struct rlimit rlim;
>      if (getrlimit(RLIMIT_NOFILE, &rlim) < 0) {
>          error_report("Failed to get the resource limit");
> @@ -4348,4 +4419,5 @@ static void __attribute__((__constructor__)) v9fs_set_fd_limit(void)
>      }
>      open_fd_hw = rlim.rlim_cur - MIN(400, rlim.rlim_cur / 3);
>      open_fd_rc = rlim.rlim_cur / 2;
> +#endif

Really?

>  }
>
Shi, Guohuai Nov. 1, 2022, 3:34 p.m. UTC | #2 
> -----Original Message-----
> From: Christian Schoenebeck <qemu_oss@crudebyte.com>
> Sent: Tuesday, November 1, 2022 23:04
> To: qemu-devel@nongnu.org
> Cc: Shi, Guohuai <Guohuai.Shi@windriver.com>; Greg Kurz <groug@kaod.org>;
> Meng, Bin <Bin.Meng@windriver.com>
> Subject: Re: [PATCH 09/16] hw/9pfs: Disable unsupported flags and features
> for Windows
> 
> [Please note: This e-mail is from an EXTERNAL e-mail address]
> 
> On Monday, October 24, 2022 6:57:52 AM CET Bin Meng wrote:
> > From: Guohuai Shi <guohuai.shi@windriver.com>
> >
> > Some flags and features are not supported on Windows, like mknod,
> > readlink, file mode, etc. Update the codes for Windows.
> >
> > Signed-off-by: Guohuai Shi <guohuai.shi@windriver.com>
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > ---
> >
> >  hw/9pfs/9p-util.h |  6 +++-
> >  hw/9pfs/9p.c      | 90 ++++++++++++++++++++++++++++++++++++++++++-----
> >  2 files changed, 86 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h index
> > 82b2d0c3e4..3d154e9103 100644
> > --- a/hw/9pfs/9p-util.h
> > +++ b/hw/9pfs/9p-util.h
> > @@ -53,8 +53,10 @@ static inline uint64_t makedev_dotl(uint32_t dev_major,
> uint32_t dev_minor)
> >   */
> >  static inline uint64_t host_dev_to_dotl_dev(dev_t dev)  { -#ifdef
> > CONFIG_LINUX
> > +#if defined(CONFIG_LINUX)
> >      return dev;
> > +#elif defined(CONFIG_WIN32)
> > +    return 0;
> 
> Really?

Check MS this document: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fstat-fstat32-fstat64-fstati64-fstat32i64-fstat64i32?view=msvc-170
st_rdev: If a device, fd; otherwise 0.
st_dev: If a device, fd; otherwise 0.

So for any file open, it should be 0.

> 
> >  #else
> >      return makedev_dotl(major(dev), minor(dev));  #endif @@ -260,7
> > +262,9 @@ static inline struct dirent *qemu_dirent_dup(struct dirent
> > *dent)  #if defined CONFIG_DARWIN && defined CONFIG_PTHREAD_FCHDIR_NP
> > int pthread_fchdir_np(int fd) __attribute__((weak_import));  #endif
> > +#ifndef CONFIG_WIN32
> >  int qemu_mknodat(P9_FILE_ID dirfd, const char *filename, mode_t mode,
> >                   dev_t dev);
> > +#endif
> >
> >  #endif
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 6c4af86240..771aab34ac
> > 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -39,6 +39,11 @@
> >  #include "qemu/xxhash.h"
> >  #include <math.h>
> >
> > +#ifdef CONFIG_WIN32
> > +#define UTIME_NOW   ((1l << 30) - 1l)
> > +#define UTIME_OMIT  ((1l << 30) - 2l) #endif
> > +
> >  int open_fd_hw;
> >  int total_open_fd;
> >  static int open_fd_rc;
> > @@ -132,13 +137,17 @@ static int dotl_to_open_flags(int flags)
> >      DotlOpenflagMap dotl_oflag_map[] = {
> >          { P9_DOTL_CREATE, O_CREAT },
> >          { P9_DOTL_EXCL, O_EXCL },
> > +#ifndef CONFIG_WIN32
> >          { P9_DOTL_NOCTTY , O_NOCTTY },
> > +#endif
> >          { P9_DOTL_TRUNC, O_TRUNC },
> >          { P9_DOTL_APPEND, O_APPEND },
> > +#ifndef CONFIG_WIN32
> >          { P9_DOTL_NONBLOCK, O_NONBLOCK } ,
> >          { P9_DOTL_DSYNC, O_DSYNC },
> >          { P9_DOTL_FASYNC, FASYNC },
> > -#ifndef CONFIG_DARWIN
> > +#endif
> > +#ifdef CONFIG_LINUX
> 
> Better
> 
>    #if !defined(CONFIG_DARWIN) && !defined(CONFIG_WIN32)
> 

It is OK.

> Otherwise it might automatically opt-out other future platforms
> unintentionally.
> 
> >          { P9_DOTL_NOATIME, O_NOATIME },
> >          /*
> >           *  On Darwin, we could map to F_NOCACHE, which is @@ -151,8
> > +160,10 @@ static int dotl_to_open_flags(int flags)  #endif
> >          { P9_DOTL_LARGEFILE, O_LARGEFILE },
> >          { P9_DOTL_DIRECTORY, O_DIRECTORY },
> > +#ifndef CONFIG_WIN32
> >          { P9_DOTL_NOFOLLOW, O_NOFOLLOW },
> >          { P9_DOTL_SYNC, O_SYNC },
> > +#endif
> >      };
> >
> >      for (i = 0; i < ARRAY_SIZE(dotl_oflag_map); i++) { @@ -179,8
> > +190,11 @@ static int get_dotl_openflags(V9fsState *s, int oflags)
> >       * Filter the client open flags
> >       */
> >      flags = dotl_to_open_flags(oflags);
> > -    flags &= ~(O_NOCTTY | O_ASYNC | O_CREAT);
> > -#ifndef CONFIG_DARWIN
> > +    flags &= ~(O_CREAT);
> > +#ifndef CONFIG_WIN32
> > +    flags &= ~(O_NOCTTY | O_ASYNC);
> > +#endif
> > +#ifdef CONFIG_LINUX
> 
> Same as above: better explicitly opt-out than the other way around.
> 

It is OK.

> >      /*
> >       * Ignore direct disk access hint until the server supports it.
> >       */
> > @@ -986,9 +1000,11 @@ static int stat_to_qid(V9fsPDU *pdu, const struct
> stat *stbuf, V9fsQID *qidp)
> >      if (S_ISDIR(stbuf->st_mode)) {
> >          qidp->type |= P9_QID_TYPE_DIR;
> >      }
> > +#ifndef CONFIG_WIN32
> >      if (S_ISLNK(stbuf->st_mode)) {
> >          qidp->type |= P9_QID_TYPE_SYMLINK;
> >      }
> > +#endif
> >
> >      return 0;
> >  }
> > @@ -1097,6 +1113,7 @@ static mode_t v9mode_to_mode(uint32_t mode,
> V9fsString *extension)
> >          ret |= S_IFDIR;
> >      }
> >
> > +#ifndef CONFIG_WIN32
> >      if (mode & P9_STAT_MODE_SYMLINK) {
> >          ret |= S_IFLNK;
> >      }
> > @@ -1106,6 +1123,7 @@ static mode_t v9mode_to_mode(uint32_t mode,
> V9fsString *extension)
> >      if (mode & P9_STAT_MODE_NAMED_PIPE) {
> >          ret |= S_IFIFO;
> >      }
> > +#endif
> >      if (mode & P9_STAT_MODE_DEVICE) {
> >          if (extension->size && extension->data[0] == 'c') {
> >              ret |= S_IFCHR;
> > @@ -1118,6 +1136,7 @@ static mode_t v9mode_to_mode(uint32_t mode,
> V9fsString *extension)
> >          ret |= S_IFREG;
> >      }
> >
> > +#ifndef CONFIG_WIN32
> >      if (mode & P9_STAT_MODE_SETUID) {
> >          ret |= S_ISUID;
> >      }
> > @@ -1127,6 +1146,7 @@ static mode_t v9mode_to_mode(uint32_t mode,
> V9fsString *extension)
> >      if (mode & P9_STAT_MODE_SETVTX) {
> >          ret |= S_ISVTX;
> >      }
> > +#endif
> >
> >      return ret;
> >  }
> > @@ -1182,6 +1202,7 @@ static uint32_t stat_to_v9mode(const struct stat
> *stbuf)
> >          mode |= P9_STAT_MODE_DIR;
> >      }
> >
> > +#ifndef CONFIG_WIN32
> >      if (S_ISLNK(stbuf->st_mode)) {
> >          mode |= P9_STAT_MODE_SYMLINK;
> >      }
> > @@ -1193,11 +1214,13 @@ static uint32_t stat_to_v9mode(const struct stat
> *stbuf)
> >      if (S_ISFIFO(stbuf->st_mode)) {
> >          mode |= P9_STAT_MODE_NAMED_PIPE;
> >      }
> > +#endif
> >
> >      if (S_ISBLK(stbuf->st_mode) || S_ISCHR(stbuf->st_mode)) {
> >          mode |= P9_STAT_MODE_DEVICE;
> >      }
> >
> > +#ifndef CONFIG_WIN32
> >      if (stbuf->st_mode & S_ISUID) {
> >          mode |= P9_STAT_MODE_SETUID;
> >      }
> > @@ -1209,6 +1232,7 @@ static uint32_t stat_to_v9mode(const struct stat
> *stbuf)
> >      if (stbuf->st_mode & S_ISVTX) {
> >          mode |= P9_STAT_MODE_SETVTX;
> >      }
> > +#endif
> >
> >      return mode;
> >  }
> > @@ -1247,9 +1271,17 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu,
> V9fsPath *path,
> >              return err;
> >          }
> >      } else if (v9stat->mode & P9_STAT_MODE_DEVICE) {
> > +        unsigned maj, min;
> > +
> > +#ifndef CONFIG_WIN32
> > +        maj = major(stbuf->st_rdev);
> > +        min = minor(stbuf->st_rdev);
> > +#else
> > +        maj = min = 0;
> > +#endif
> 
> Really?

See above link.

> 
> >          v9fs_string_sprintf(&v9stat->extension, "%c %u %u",
> >                  S_ISCHR(stbuf->st_mode) ? 'c' : 'b',
> > -                major(stbuf->st_rdev), minor(stbuf->st_rdev));
> > +                maj, min);
> >      } else if (S_ISDIR(stbuf->st_mode) || S_ISREG(stbuf->st_mode)) {
> >          v9fs_string_sprintf(&v9stat->extension, "%s %lu",
> >                  "HARDLINKCOUNT", (unsigned long)stbuf->st_nlink); @@
> > -1317,7 +1349,14 @@ static int32_t blksize_to_iounit(const V9fsPDU
> > *pdu, int32_t blksize)
> >
> >  static int32_t stat_to_iounit(const V9fsPDU *pdu, const struct stat
> > *stbuf)  {
> > -    return blksize_to_iounit(pdu, stbuf->st_blksize);
> > +    int32_t blksize;
> > +
> > +#ifndef CONFIG_WIN32
> > +    blksize = stbuf->st_blksize);
> > +#else
> > +    blksize = 0;
> > +#endif
> 
> Really?

Windows struct stat does not have such field. See above link.

> 
> > +    return blksize_to_iounit(pdu, blksize);
> >  }
> >
> >  static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat
> > *stbuf, @@ -1332,7 +1371,11 @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu,
> const struct stat *stbuf,
> >      v9lstat->st_rdev = host_dev_to_dotl_dev(stbuf->st_rdev);
> >      v9lstat->st_size = stbuf->st_size;
> >      v9lstat->st_blksize = stat_to_iounit(pdu, stbuf);
> > +#ifndef CONFIG_WIN32
> >      v9lstat->st_blocks = stbuf->st_blocks;
> > +#else
> > +    v9lstat->st_blocks = 0;
> > +#endif
> 
> Really?

Windows struct stat does not have such field. See above link.

> 
> >      v9lstat->st_atime_sec = stbuf->st_atime;
> >      v9lstat->st_mtime_sec = stbuf->st_mtime;
> >      v9lstat->st_ctime_sec = stbuf->st_ctime; @@ -1340,7 +1383,8 @@
> > static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
> >      v9lstat->st_atime_nsec = stbuf->st_atimespec.tv_nsec;
> >      v9lstat->st_mtime_nsec = stbuf->st_mtimespec.tv_nsec;
> >      v9lstat->st_ctime_nsec = stbuf->st_ctimespec.tv_nsec; -#else
> > +#endif
> > +#ifdef CONFIG_LINUX
> >      v9lstat->st_atime_nsec = stbuf->st_atim.tv_nsec;
> >      v9lstat->st_mtime_nsec = stbuf->st_mtim.tv_nsec;
> >      v9lstat->st_ctime_nsec = stbuf->st_ctim.tv_nsec; @@ -2471,6
> > +2515,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu,
> V9fsFidState *fidp,
> >      struct dirent *dent;
> >      struct stat *st;
> >      struct V9fsDirEnt *entries = NULL;
> > +    unsigned char d_type = 0;
> >
> >      /*
> >       * inode remapping requires the device id, which in turn might be
> > @@ -2540,10 +2585,13 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU
> *pdu, V9fsFidState *fidp,
> >          v9fs_string_init(&name);
> >          v9fs_string_sprintf(&name, "%s", dent->d_name);
> >
> > +#ifndef CONFIG_WIN32
> > +        d_type = dent->d_type;
> > +#endif
> >          /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
> >          len = pdu_marshal(pdu, 11 + count, "Qqbs",
> >                            &qid, off,
> > -                          dent->d_type, &name);
> > +                          d_type, &name);
> 
> Are you saying that d_type is not initialized with zero already?

struct dirent is defined by MinGW, it does not have d_type member:

https://github.com/Alexpux/mingw-w64/blob/master/mingw-w64-headers/crt/dirent.h


> 
> >          v9fs_string_free(&name);
> >
> > @@ -2873,8 +2921,12 @@ static void coroutine_fn v9fs_create(void *opaque)
> >          }
> >
> >          nmode |= perm & 0777;
> > +#ifndef CONFIG_WIN32
> >          err = v9fs_co_mknod(pdu, fidp, &name, fidp->uid, -1,
> >                              makedev(major, minor), nmode, &stbuf);
> > +#else
> > +        err = -ENOTSUP;
> > +#endif
> >          if (err < 0) {
> >              goto out;
> >          }
> > @@ -2899,8 +2951,12 @@ static void coroutine_fn v9fs_create(void *opaque)
> >          v9fs_path_copy(&fidp->path, &path);
> >          v9fs_path_unlock(s);
> >      } else if (perm & P9_STAT_MODE_SOCKET) {
> > +#ifndef CONFIG_WIN32
> >          err = v9fs_co_mknod(pdu, fidp, &name, fidp->uid, -1,
> >                              0, S_IFSOCK | (perm & 0777), &stbuf);
> > +#else
> > +        err = -ENOTSUP;
> > +#endif
> 
> As with previous patches, I would consider making the user aware to use
> mapped(-xattr) with something like error_report_once().

OK, got it.

> 
> >          if (err < 0) {
> >              goto out;
> >          }
> > @@ -3634,7 +3690,7 @@ out_nofid:
> >
> >  static void coroutine_fn v9fs_mknod(void *opaque)  {
> > -
> > +#ifndef CONFIG_WIN32
> >      int mode;
> >      gid_t gid;
> >      int32_t fid;
> > @@ -3691,6 +3747,10 @@ out:
> >  out_nofid:
> >      pdu_complete(pdu, err);
> >      v9fs_string_free(&name);
> > +#else
> > +    V9fsPDU *pdu = opaque;
> > +    pdu_complete(pdu, -1);
> > +#endif
> >  }
> >
> >  /*
> > @@ -3963,7 +4023,7 @@ out_nofid:
> >  #if defined(CONFIG_LINUX)
> >  /* Currently, only Linux has XATTR_SIZE_MAX */  #define
> > P9_XATTR_SIZE_MAX XATTR_SIZE_MAX -#elif defined(CONFIG_DARWIN)
> > +#elif defined(CONFIG_DARWIN) || defined(CONFIG_WIN32)
> >  /*
> >   * Darwin doesn't seem to define a maximum xattr size in its user
> >   * space header, so manually configure it across platforms as 64k.
> > @@ -3980,6 +4040,7 @@ out_nofid:
> >
> >  static void coroutine_fn v9fs_xattrcreate(void *opaque)  {
> > +#ifndef CONFIG_WIN32
> >      int flags, rflags = 0;
> >      int32_t fid;
> >      uint64_t size;
> > @@ -4041,10 +4102,15 @@ out_put_fid:
> >  out_nofid:
> >      pdu_complete(pdu, err);
> >      v9fs_string_free(&name);
> > +#else
> > +    V9fsPDU *pdu = opaque;
> > +    pdu_complete(pdu, -1);
> > +#endif
> >  }
> >
> >  static void coroutine_fn v9fs_readlink(void *opaque)  {
> > +#ifndef CONFIG_WIN32
> >      V9fsPDU *pdu = opaque;
> >      size_t offset = 7;
> >      V9fsString target;
> > @@ -4080,6 +4146,10 @@ out:
> >      put_fid(pdu, fidp);
> >  out_nofid:
> >      pdu_complete(pdu, err);
> > +#else
> > +    V9fsPDU *pdu = opaque;
> > +    pdu_complete(pdu, -1);
> > +#endif
> 
> Unnecessary double declaration of pdu.
> 

OK, got it.

> >  }
> >
> >  static CoroutineEntry *pdu_co_handlers[] = { @@ -4341,6 +4411,7 @@
> > void v9fs_reset(V9fsState *s)
> >
> >  static void __attribute__((__constructor__)) v9fs_set_fd_limit(void)
> > {
> > +#ifndef CONFIG_WIN32
> >      struct rlimit rlim;
> >      if (getrlimit(RLIMIT_NOFILE, &rlim) < 0) {
> >          error_report("Failed to get the resource limit"); @@ -4348,4
> > +4419,5 @@ static void __attribute__((__constructor__))
> v9fs_set_fd_limit(void)
> >      }
> >      open_fd_hw = rlim.rlim_cur - MIN(400, rlim.rlim_cur / 3);
> >      open_fd_rc = rlim.rlim_cur / 2;
> > +#endif
> 
> Really?

Windows does not provide getrlimit()

> 
> >  }
> >
> 
>
Christian Schoenebeck Nov. 1, 2022, 6:59 p.m. UTC | #3 
On Tuesday, November 1, 2022 4:34:54 PM CET Shi, Guohuai wrote:
> 
> > -----Original Message-----
> > From: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > Sent: Tuesday, November 1, 2022 23:04
> > To: qemu-devel@nongnu.org
> > Cc: Shi, Guohuai <Guohuai.Shi@windriver.com>; Greg Kurz <groug@kaod.org>;
> > Meng, Bin <Bin.Meng@windriver.com>
> > Subject: Re: [PATCH 09/16] hw/9pfs: Disable unsupported flags and features
> > for Windows
> > 
> > [Please note: This e-mail is from an EXTERNAL e-mail address]
> > 
> > On Monday, October 24, 2022 6:57:52 AM CET Bin Meng wrote:
> > > From: Guohuai Shi <guohuai.shi@windriver.com>
> > >
> > > Some flags and features are not supported on Windows, like mknod,
> > > readlink, file mode, etc. Update the codes for Windows.
> > >
> > > Signed-off-by: Guohuai Shi <guohuai.shi@windriver.com>
> > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > ---
> > >
> > >  hw/9pfs/9p-util.h |  6 +++-
> > >  hw/9pfs/9p.c      | 90 ++++++++++++++++++++++++++++++++++++++++++-----
> > >  2 files changed, 86 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h index
> > > 82b2d0c3e4..3d154e9103 100644
> > > --- a/hw/9pfs/9p-util.h
> > > +++ b/hw/9pfs/9p-util.h
> > > @@ -53,8 +53,10 @@ static inline uint64_t makedev_dotl(uint32_t dev_major,
> > uint32_t dev_minor)
> > >   */
> > >  static inline uint64_t host_dev_to_dotl_dev(dev_t dev)  { -#ifdef
> > > CONFIG_LINUX
> > > +#if defined(CONFIG_LINUX)
> > >      return dev;
> > > +#elif defined(CONFIG_WIN32)
> > > +    return 0;
> > 
> > Really?
> 
> Check MS this document: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fstat-fstat32-fstat64-fstati64-fstat32i64-fstat64i32?view=msvc-170
> st_rdev: If a device, fd; otherwise 0.
> st_dev: If a device, fd; otherwise 0.
> 
> So for any file open, it should be 0.

Yeah, but that function translates a corresponding device ID for *Linux* guest
side. And the intention is to avoid e.g. file ID collisions on guest side.
Because for a Linux guest, the two-tuple (device number, inode number) makes a
system-wide unique file ID.

If you just return zero here, that might be OK if only one 9p directory is
exported to guest, but say you have "C:\foo\" exported and "D:\bar\" exported
and mounted via 9p to guest, then guest would assume every file with the same
inode number on those two to be the same files. But they are not. They are on
two different drives actually.

> 
> > 
> > >  #else
> > >      return makedev_dotl(major(dev), minor(dev));  #endif @@ -260,7
> > > +262,9 @@ static inline struct dirent *qemu_dirent_dup(struct dirent
> > > *dent)  #if defined CONFIG_DARWIN && defined CONFIG_PTHREAD_FCHDIR_NP
> > > int pthread_fchdir_np(int fd) __attribute__((weak_import));  #endif
> > > +#ifndef CONFIG_WIN32
> > >  int qemu_mknodat(P9_FILE_ID dirfd, const char *filename, mode_t mode,
> > >                   dev_t dev);
> > > +#endif
> > >
> > >  #endif
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 6c4af86240..771aab34ac
> > > 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> > > @@ -39,6 +39,11 @@
> > >  #include "qemu/xxhash.h"
> > >  #include <math.h>
> > >
> > > +#ifdef CONFIG_WIN32
> > > +#define UTIME_NOW   ((1l << 30) - 1l)
> > > +#define UTIME_OMIT  ((1l << 30) - 2l) #endif
> > > +
> > >  int open_fd_hw;
> > >  int total_open_fd;
> > >  static int open_fd_rc;
> > > @@ -132,13 +137,17 @@ static int dotl_to_open_flags(int flags)
> > >      DotlOpenflagMap dotl_oflag_map[] = {
> > >          { P9_DOTL_CREATE, O_CREAT },
> > >          { P9_DOTL_EXCL, O_EXCL },
> > > +#ifndef CONFIG_WIN32
> > >          { P9_DOTL_NOCTTY , O_NOCTTY },
> > > +#endif
> > >          { P9_DOTL_TRUNC, O_TRUNC },
> > >          { P9_DOTL_APPEND, O_APPEND },
> > > +#ifndef CONFIG_WIN32
> > >          { P9_DOTL_NONBLOCK, O_NONBLOCK } ,
> > >          { P9_DOTL_DSYNC, O_DSYNC },
> > >          { P9_DOTL_FASYNC, FASYNC },
> > > -#ifndef CONFIG_DARWIN
> > > +#endif
> > > +#ifdef CONFIG_LINUX
> > 
> > Better
> > 
> >    #if !defined(CONFIG_DARWIN) && !defined(CONFIG_WIN32)
> > 
> 
> It is OK.

You got my point, hopefully:

> > Otherwise it might automatically opt-out other future platforms
> > unintentionally.
> > 
> > >          { P9_DOTL_NOATIME, O_NOATIME },
> > >          /*
> > >           *  On Darwin, we could map to F_NOCACHE, which is @@ -151,8
> > > +160,10 @@ static int dotl_to_open_flags(int flags)  #endif
> > >          { P9_DOTL_LARGEFILE, O_LARGEFILE },
> > >          { P9_DOTL_DIRECTORY, O_DIRECTORY },
> > > +#ifndef CONFIG_WIN32
> > >          { P9_DOTL_NOFOLLOW, O_NOFOLLOW },
> > >          { P9_DOTL_SYNC, O_SYNC },
> > > +#endif
> > >      };
> > >
> > >      for (i = 0; i < ARRAY_SIZE(dotl_oflag_map); i++) { @@ -179,8
> > > +190,11 @@ static int get_dotl_openflags(V9fsState *s, int oflags)
> > >       * Filter the client open flags
> > >       */
> > >      flags = dotl_to_open_flags(oflags);
> > > -    flags &= ~(O_NOCTTY | O_ASYNC | O_CREAT);
> > > -#ifndef CONFIG_DARWIN
> > > +    flags &= ~(O_CREAT);
> > > +#ifndef CONFIG_WIN32
> > > +    flags &= ~(O_NOCTTY | O_ASYNC);
> > > +#endif
> > > +#ifdef CONFIG_LINUX
> > 
> > Same as above: better explicitly opt-out than the other way around.
> > 
> 
> It is OK.
> 
> > >      /*
> > >       * Ignore direct disk access hint until the server supports it.
> > >       */
> > > @@ -986,9 +1000,11 @@ static int stat_to_qid(V9fsPDU *pdu, const struct
> > stat *stbuf, V9fsQID *qidp)
> > >      if (S_ISDIR(stbuf->st_mode)) {
> > >          qidp->type |= P9_QID_TYPE_DIR;
> > >      }
> > > +#ifndef CONFIG_WIN32
> > >      if (S_ISLNK(stbuf->st_mode)) {
> > >          qidp->type |= P9_QID_TYPE_SYMLINK;
> > >      }
> > > +#endif
> > >
> > >      return 0;
> > >  }
> > > @@ -1097,6 +1113,7 @@ static mode_t v9mode_to_mode(uint32_t mode,
> > V9fsString *extension)
> > >          ret |= S_IFDIR;
> > >      }
> > >
> > > +#ifndef CONFIG_WIN32
> > >      if (mode & P9_STAT_MODE_SYMLINK) {
> > >          ret |= S_IFLNK;
> > >      }
> > > @@ -1106,6 +1123,7 @@ static mode_t v9mode_to_mode(uint32_t mode,
> > V9fsString *extension)
> > >      if (mode & P9_STAT_MODE_NAMED_PIPE) {
> > >          ret |= S_IFIFO;
> > >      }
> > > +#endif
> > >      if (mode & P9_STAT_MODE_DEVICE) {
> > >          if (extension->size && extension->data[0] == 'c') {
> > >              ret |= S_IFCHR;
> > > @@ -1118,6 +1136,7 @@ static mode_t v9mode_to_mode(uint32_t mode,
> > V9fsString *extension)
> > >          ret |= S_IFREG;
> > >      }
> > >
> > > +#ifndef CONFIG_WIN32
> > >      if (mode & P9_STAT_MODE_SETUID) {
> > >          ret |= S_ISUID;
> > >      }
> > > @@ -1127,6 +1146,7 @@ static mode_t v9mode_to_mode(uint32_t mode,
> > V9fsString *extension)
> > >      if (mode & P9_STAT_MODE_SETVTX) {
> > >          ret |= S_ISVTX;
> > >      }
> > > +#endif
> > >
> > >      return ret;
> > >  }
> > > @@ -1182,6 +1202,7 @@ static uint32_t stat_to_v9mode(const struct stat
> > *stbuf)
> > >          mode |= P9_STAT_MODE_DIR;
> > >      }
> > >
> > > +#ifndef CONFIG_WIN32
> > >      if (S_ISLNK(stbuf->st_mode)) {
> > >          mode |= P9_STAT_MODE_SYMLINK;
> > >      }
> > > @@ -1193,11 +1214,13 @@ static uint32_t stat_to_v9mode(const struct stat
> > *stbuf)
> > >      if (S_ISFIFO(stbuf->st_mode)) {
> > >          mode |= P9_STAT_MODE_NAMED_PIPE;
> > >      }
> > > +#endif
> > >
> > >      if (S_ISBLK(stbuf->st_mode) || S_ISCHR(stbuf->st_mode)) {
> > >          mode |= P9_STAT_MODE_DEVICE;
> > >      }
> > >
> > > +#ifndef CONFIG_WIN32
> > >      if (stbuf->st_mode & S_ISUID) {
> > >          mode |= P9_STAT_MODE_SETUID;
> > >      }
> > > @@ -1209,6 +1232,7 @@ static uint32_t stat_to_v9mode(const struct stat
> > *stbuf)
> > >      if (stbuf->st_mode & S_ISVTX) {
> > >          mode |= P9_STAT_MODE_SETVTX;
> > >      }
> > > +#endif
> > >
> > >      return mode;
> > >  }
> > > @@ -1247,9 +1271,17 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu,
> > V9fsPath *path,
> > >              return err;
> > >          }
> > >      } else if (v9stat->mode & P9_STAT_MODE_DEVICE) {
> > > +        unsigned maj, min;
> > > +
> > > +#ifndef CONFIG_WIN32
> > > +        maj = major(stbuf->st_rdev);
> > > +        min = minor(stbuf->st_rdev);
> > > +#else
> > > +        maj = min = 0;
> > > +#endif
> > 
> > Really?
> 
> See above link.
> 
> > 
> > >          v9fs_string_sprintf(&v9stat->extension, "%c %u %u",
> > >                  S_ISCHR(stbuf->st_mode) ? 'c' : 'b',
> > > -                major(stbuf->st_rdev), minor(stbuf->st_rdev));
> > > +                maj, min);
> > >      } else if (S_ISDIR(stbuf->st_mode) || S_ISREG(stbuf->st_mode)) {
> > >          v9fs_string_sprintf(&v9stat->extension, "%s %lu",
> > >                  "HARDLINKCOUNT", (unsigned long)stbuf->st_nlink); @@
> > > -1317,7 +1349,14 @@ static int32_t blksize_to_iounit(const V9fsPDU
> > > *pdu, int32_t blksize)
> > >
> > >  static int32_t stat_to_iounit(const V9fsPDU *pdu, const struct stat
> > > *stbuf)  {
> > > -    return blksize_to_iounit(pdu, stbuf->st_blksize);
> > > +    int32_t blksize;
> > > +
> > > +#ifndef CONFIG_WIN32
> > > +    blksize = stbuf->st_blksize);
> > > +#else
> > > +    blksize = 0;
> > > +#endif
> > 
> > Really?
> 
> Windows struct stat does not have such field. See above link.

Yeah, but you cannot simply return zero here, because that information is
interpreted on guest side as the optimum chunk size for I/O. So some apps
might misbehave e.g. by trying allocate buffers with zero size, throwing
division by zero exceptions, or trying to read() / write() with zero chunk
size.

I'm pretty sure there is some kind of API to get the block size of the
underlying drive on Windows. And if not, then something like 4k or 8k is still
better than zero.

> > 
> > > +    return blksize_to_iounit(pdu, blksize);
> > >  }
> > >
> > >  static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat
> > > *stbuf, @@ -1332,7 +1371,11 @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu,
> > const struct stat *stbuf,
> > >      v9lstat->st_rdev = host_dev_to_dotl_dev(stbuf->st_rdev);
> > >      v9lstat->st_size = stbuf->st_size;
> > >      v9lstat->st_blksize = stat_to_iounit(pdu, stbuf);
> > > +#ifndef CONFIG_WIN32
> > >      v9lstat->st_blocks = stbuf->st_blocks;
> > > +#else
> > > +    v9lstat->st_blocks = 0;
> > > +#endif
> > 
> > Really?
> 
> Windows struct stat does not have such field. See above link.

Then it probably has to be calculated by file size / block size.

> > 
> > >      v9lstat->st_atime_sec = stbuf->st_atime;
> > >      v9lstat->st_mtime_sec = stbuf->st_mtime;
> > >      v9lstat->st_ctime_sec = stbuf->st_ctime; @@ -1340,7 +1383,8 @@
> > > static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
> > >      v9lstat->st_atime_nsec = stbuf->st_atimespec.tv_nsec;
> > >      v9lstat->st_mtime_nsec = stbuf->st_mtimespec.tv_nsec;
> > >      v9lstat->st_ctime_nsec = stbuf->st_ctimespec.tv_nsec; -#else
> > > +#endif
> > > +#ifdef CONFIG_LINUX
> > >      v9lstat->st_atime_nsec = stbuf->st_atim.tv_nsec;
> > >      v9lstat->st_mtime_nsec = stbuf->st_mtim.tv_nsec;
> > >      v9lstat->st_ctime_nsec = stbuf->st_ctim.tv_nsec; @@ -2471,6
> > > +2515,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu,
> > V9fsFidState *fidp,
> > >      struct dirent *dent;
> > >      struct stat *st;
> > >      struct V9fsDirEnt *entries = NULL;
> > > +    unsigned char d_type = 0;
> > >
> > >      /*
> > >       * inode remapping requires the device id, which in turn might be
> > > @@ -2540,10 +2585,13 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU
> > *pdu, V9fsFidState *fidp,
> > >          v9fs_string_init(&name);
> > >          v9fs_string_sprintf(&name, "%s", dent->d_name);
> > >
> > > +#ifndef CONFIG_WIN32
> > > +        d_type = dent->d_type;
> > > +#endif
> > >          /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
> > >          len = pdu_marshal(pdu, 11 + count, "Qqbs",
> > >                            &qid, off,
> > > -                          dent->d_type, &name);
> > > +                          d_type, &name);
> > 
> > Are you saying that d_type is not initialized with zero already?
> 
> struct dirent is defined by MinGW, it does not have d_type member:
> 
> https://github.com/Alexpux/mingw-w64/blob/master/mingw-w64-headers/crt/dirent.h

My bad, I misread your code. That's fine.

> 
> > 
> > >          v9fs_string_free(&name);
> > >
> > > @@ -2873,8 +2921,12 @@ static void coroutine_fn v9fs_create(void *opaque)
> > >          }
> > >
> > >          nmode |= perm & 0777;
> > > +#ifndef CONFIG_WIN32
> > >          err = v9fs_co_mknod(pdu, fidp, &name, fidp->uid, -1,
> > >                              makedev(major, minor), nmode, &stbuf);
> > > +#else
> > > +        err = -ENOTSUP;
> > > +#endif
> > >          if (err < 0) {
> > >              goto out;
> > >          }
> > > @@ -2899,8 +2951,12 @@ static void coroutine_fn v9fs_create(void *opaque)
> > >          v9fs_path_copy(&fidp->path, &path);
> > >          v9fs_path_unlock(s);
> > >      } else if (perm & P9_STAT_MODE_SOCKET) {
> > > +#ifndef CONFIG_WIN32
> > >          err = v9fs_co_mknod(pdu, fidp, &name, fidp->uid, -1,
> > >                              0, S_IFSOCK | (perm & 0777), &stbuf);
> > > +#else
> > > +        err = -ENOTSUP;
> > > +#endif
> > 
> > As with previous patches, I would consider making the user aware to use
> > mapped(-xattr) with something like error_report_once().
> 
> OK, got it.
> 
> > 
> > >          if (err < 0) {
> > >              goto out;
> > >          }
> > > @@ -3634,7 +3690,7 @@ out_nofid:
> > >
> > >  static void coroutine_fn v9fs_mknod(void *opaque)  {
> > > -
> > > +#ifndef CONFIG_WIN32
> > >      int mode;
> > >      gid_t gid;
> > >      int32_t fid;
> > > @@ -3691,6 +3747,10 @@ out:
> > >  out_nofid:
> > >      pdu_complete(pdu, err);
> > >      v9fs_string_free(&name);
> > > +#else
> > > +    V9fsPDU *pdu = opaque;
> > > +    pdu_complete(pdu, -1);
> > > +#endif
> > >  }
> > >
> > >  /*
> > > @@ -3963,7 +4023,7 @@ out_nofid:
> > >  #if defined(CONFIG_LINUX)
> > >  /* Currently, only Linux has XATTR_SIZE_MAX */  #define
> > > P9_XATTR_SIZE_MAX XATTR_SIZE_MAX -#elif defined(CONFIG_DARWIN)
> > > +#elif defined(CONFIG_DARWIN) || defined(CONFIG_WIN32)
> > >  /*
> > >   * Darwin doesn't seem to define a maximum xattr size in its user
> > >   * space header, so manually configure it across platforms as 64k.
> > > @@ -3980,6 +4040,7 @@ out_nofid:
> > >
> > >  static void coroutine_fn v9fs_xattrcreate(void *opaque)  {
> > > +#ifndef CONFIG_WIN32
> > >      int flags, rflags = 0;
> > >      int32_t fid;
> > >      uint64_t size;
> > > @@ -4041,10 +4102,15 @@ out_put_fid:
> > >  out_nofid:
> > >      pdu_complete(pdu, err);
> > >      v9fs_string_free(&name);
> > > +#else
> > > +    V9fsPDU *pdu = opaque;
> > > +    pdu_complete(pdu, -1);
> > > +#endif
> > >  }
> > >
> > >  static void coroutine_fn v9fs_readlink(void *opaque)  {
> > > +#ifndef CONFIG_WIN32
> > >      V9fsPDU *pdu = opaque;
> > >      size_t offset = 7;
> > >      V9fsString target;
> > > @@ -4080,6 +4146,10 @@ out:
> > >      put_fid(pdu, fidp);
> > >  out_nofid:
> > >      pdu_complete(pdu, err);
> > > +#else
> > > +    V9fsPDU *pdu = opaque;
> > > +    pdu_complete(pdu, -1);
> > > +#endif
> > 
> > Unnecessary double declaration of pdu.
> > 
> 
> OK, got it.
> 
> > >  }
> > >
> > >  static CoroutineEntry *pdu_co_handlers[] = { @@ -4341,6 +4411,7 @@
> > > void v9fs_reset(V9fsState *s)
> > >
> > >  static void __attribute__((__constructor__)) v9fs_set_fd_limit(void)
> > > {
> > > +#ifndef CONFIG_WIN32
> > >      struct rlimit rlim;
> > >      if (getrlimit(RLIMIT_NOFILE, &rlim) < 0) {
> > >          error_report("Failed to get the resource limit"); @@ -4348,4
> > > +4419,5 @@ static void __attribute__((__constructor__))
> > v9fs_set_fd_limit(void)
> > >      }
> > >      open_fd_hw = rlim.rlim_cur - MIN(400, rlim.rlim_cur / 3);
> > >      open_fd_rc = rlim.rlim_cur / 2;
> > > +#endif
> > 
> > Really?
> 
> Windows does not provide getrlimit()

But it has _getmaxstdio() and _setmaxstdio().

And even if Windows had no replacement functions, you need to initialize these
two global variables with some meaningful value. Otherwise they would be zero,
and that would mean 9p server would assume max. 0 files could be open at the
same time, so it would constantly close and re-open every single file
descriptor on every minor micro-transaction for no reason.

Best regards,
Christian Schoenebeck
Shi, Guohuai Nov. 2, 2022, 3:44 a.m. UTC | #4 
> -----Original Message-----
> From: Christian Schoenebeck <qemu_oss@crudebyte.com>
> Sent: Wednesday, November 2, 2022 02:59
> To: qemu-devel@nongnu.org
> Cc: Greg Kurz <groug@kaod.org>; Meng, Bin <Bin.Meng@windriver.com>; Shi,
> Guohuai <Guohuai.Shi@windriver.com>
> Subject: Re: [PATCH 09/16] hw/9pfs: Disable unsupported flags and features
> for Windows
> 
> [Please note: This e-mail is from an EXTERNAL e-mail address]
> 
> On Tuesday, November 1, 2022 4:34:54 PM CET Shi, Guohuai wrote:
> >
> > > -----Original Message-----
> > > From: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > Sent: Tuesday, November 1, 2022 23:04
> > > To: qemu-devel@nongnu.org
> > > Cc: Shi, Guohuai <Guohuai.Shi@windriver.com>; Greg Kurz
> > > <groug@kaod.org>; Meng, Bin <Bin.Meng@windriver.com>
> > > Subject: Re: [PATCH 09/16] hw/9pfs: Disable unsupported flags and
> > > features for Windows
> > >
> > > [Please note: This e-mail is from an EXTERNAL e-mail address]
> > >
> > > On Monday, October 24, 2022 6:57:52 AM CET Bin Meng wrote:
> > > > From: Guohuai Shi <guohuai.shi@windriver.com>
> > > >
> > > > Some flags and features are not supported on Windows, like mknod,
> > > > readlink, file mode, etc. Update the codes for Windows.
> > > >
> > > > Signed-off-by: Guohuai Shi <guohuai.shi@windriver.com>
> > > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > > ---
> > > >
> > > >  hw/9pfs/9p-util.h |  6 +++-
> > > >  hw/9pfs/9p.c      | 90 ++++++++++++++++++++++++++++++++++++++++++-----
> > > >  2 files changed, 86 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h index
> > > > 82b2d0c3e4..3d154e9103 100644
> > > > --- a/hw/9pfs/9p-util.h
> > > > +++ b/hw/9pfs/9p-util.h
> > > > @@ -53,8 +53,10 @@ static inline uint64_t makedev_dotl(uint32_t
> > > > dev_major,
> > > uint32_t dev_minor)
> > > >   */
> > > >  static inline uint64_t host_dev_to_dotl_dev(dev_t dev)  { -#ifdef
> > > > CONFIG_LINUX
> > > > +#if defined(CONFIG_LINUX)
> > > >      return dev;
> > > > +#elif defined(CONFIG_WIN32)
> > > > +    return 0;
> > >
> > > Really?
> >
> > Check MS this document:
> > https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fsta
> > t-fstat32-fstat64-fstati64-fstat32i64-fstat64i32?view=msvc-170
> > st_rdev: If a device, fd; otherwise 0.
> > st_dev: If a device, fd; otherwise 0.
> >
> > So for any file open, it should be 0.
> 
> Yeah, but that function translates a corresponding device ID for *Linux*
> guest side. And the intention is to avoid e.g. file ID collisions on guest
> side.
> Because for a Linux guest, the two-tuple (device number, inode number) makes
> a system-wide unique file ID.
> 
> If you just return zero here, that might be OK if only one 9p directory is
> exported to guest, but say you have "C:\foo\" exported and "D:\bar\" exported
> and mounted via 9p to guest, then guest would assume every file with the same
> inode number on those two to be the same files. But they are not. They are on
> two different drives actually.
> 

Got it.
Windows does not provide any numerical type ID for device, 
I think the solution could be using driver letter ASC code "C:", "D:", etc.

> >
> > >
> > > >  #else
> > > >      return makedev_dotl(major(dev), minor(dev));  #endif @@
> > > > -260,7
> > > > +262,9 @@ static inline struct dirent *qemu_dirent_dup(struct
> > > > +dirent
> > > > *dent)  #if defined CONFIG_DARWIN && defined
> > > > CONFIG_PTHREAD_FCHDIR_NP int pthread_fchdir_np(int fd)
> > > > __attribute__((weak_import));  #endif
> > > > +#ifndef CONFIG_WIN32
> > > >  int qemu_mknodat(P9_FILE_ID dirfd, const char *filename, mode_t mode,
> > > >                   dev_t dev);
> > > > +#endif
> > > >
> > > >  #endif
> > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index
> > > > 6c4af86240..771aab34ac
> > > > 100644
> > > > --- a/hw/9pfs/9p.c
> > > > +++ b/hw/9pfs/9p.c
> > > > @@ -39,6 +39,11 @@
> > > >  #include "qemu/xxhash.h"
> > > >  #include <math.h>
> > > >
> > > > +#ifdef CONFIG_WIN32
> > > > +#define UTIME_NOW   ((1l << 30) - 1l)
> > > > +#define UTIME_OMIT  ((1l << 30) - 2l) #endif
> > > > +
> > > >  int open_fd_hw;
> > > >  int total_open_fd;
> > > >  static int open_fd_rc;
> > > > @@ -132,13 +137,17 @@ static int dotl_to_open_flags(int flags)
> > > >      DotlOpenflagMap dotl_oflag_map[] = {
> > > >          { P9_DOTL_CREATE, O_CREAT },
> > > >          { P9_DOTL_EXCL, O_EXCL },
> > > > +#ifndef CONFIG_WIN32
> > > >          { P9_DOTL_NOCTTY , O_NOCTTY },
> > > > +#endif
> > > >          { P9_DOTL_TRUNC, O_TRUNC },
> > > >          { P9_DOTL_APPEND, O_APPEND },
> > > > +#ifndef CONFIG_WIN32
> > > >          { P9_DOTL_NONBLOCK, O_NONBLOCK } ,
> > > >          { P9_DOTL_DSYNC, O_DSYNC },
> > > >          { P9_DOTL_FASYNC, FASYNC }, -#ifndef CONFIG_DARWIN
> > > > +#endif
> > > > +#ifdef CONFIG_LINUX
> > >
> > > Better
> > >
> > >    #if !defined(CONFIG_DARWIN) && !defined(CONFIG_WIN32)
> > >
> >
> > It is OK.
> 
> You got my point, hopefully:
> 
> > > Otherwise it might automatically opt-out other future platforms
> > > unintentionally.
> > >
> > > >          { P9_DOTL_NOATIME, O_NOATIME },
> > > >          /*
> > > >           *  On Darwin, we could map to F_NOCACHE, which is @@
> > > > -151,8
> > > > +160,10 @@ static int dotl_to_open_flags(int flags)  #endif
> > > >          { P9_DOTL_LARGEFILE, O_LARGEFILE },
> > > >          { P9_DOTL_DIRECTORY, O_DIRECTORY },
> > > > +#ifndef CONFIG_WIN32
> > > >          { P9_DOTL_NOFOLLOW, O_NOFOLLOW },
> > > >          { P9_DOTL_SYNC, O_SYNC },
> > > > +#endif
> > > >      };
> > > >
> > > >      for (i = 0; i < ARRAY_SIZE(dotl_oflag_map); i++) { @@ -179,8
> > > > +190,11 @@ static int get_dotl_openflags(V9fsState *s, int oflags)
> > > >       * Filter the client open flags
> > > >       */
> > > >      flags = dotl_to_open_flags(oflags);
> > > > -    flags &= ~(O_NOCTTY | O_ASYNC | O_CREAT);
> > > > -#ifndef CONFIG_DARWIN
> > > > +    flags &= ~(O_CREAT);
> > > > +#ifndef CONFIG_WIN32
> > > > +    flags &= ~(O_NOCTTY | O_ASYNC); #endif #ifdef CONFIG_LINUX
> > >
> > > Same as above: better explicitly opt-out than the other way around.
> > >
> >
> > It is OK.
> >
> > > >      /*
> > > >       * Ignore direct disk access hint until the server supports it.
> > > >       */
> > > > @@ -986,9 +1000,11 @@ static int stat_to_qid(V9fsPDU *pdu, const
> > > > struct
> > > stat *stbuf, V9fsQID *qidp)
> > > >      if (S_ISDIR(stbuf->st_mode)) {
> > > >          qidp->type |= P9_QID_TYPE_DIR;
> > > >      }
> > > > +#ifndef CONFIG_WIN32
> > > >      if (S_ISLNK(stbuf->st_mode)) {
> > > >          qidp->type |= P9_QID_TYPE_SYMLINK;
> > > >      }
> > > > +#endif
> > > >
> > > >      return 0;
> > > >  }
> > > > @@ -1097,6 +1113,7 @@ static mode_t v9mode_to_mode(uint32_t mode,
> > > V9fsString *extension)
> > > >          ret |= S_IFDIR;
> > > >      }
> > > >
> > > > +#ifndef CONFIG_WIN32
> > > >      if (mode & P9_STAT_MODE_SYMLINK) {
> > > >          ret |= S_IFLNK;
> > > >      }
> > > > @@ -1106,6 +1123,7 @@ static mode_t v9mode_to_mode(uint32_t mode,
> > > V9fsString *extension)
> > > >      if (mode & P9_STAT_MODE_NAMED_PIPE) {
> > > >          ret |= S_IFIFO;
> > > >      }
> > > > +#endif
> > > >      if (mode & P9_STAT_MODE_DEVICE) {
> > > >          if (extension->size && extension->data[0] == 'c') {
> > > >              ret |= S_IFCHR;
> > > > @@ -1118,6 +1136,7 @@ static mode_t v9mode_to_mode(uint32_t mode,
> > > V9fsString *extension)
> > > >          ret |= S_IFREG;
> > > >      }
> > > >
> > > > +#ifndef CONFIG_WIN32
> > > >      if (mode & P9_STAT_MODE_SETUID) {
> > > >          ret |= S_ISUID;
> > > >      }
> > > > @@ -1127,6 +1146,7 @@ static mode_t v9mode_to_mode(uint32_t mode,
> > > V9fsString *extension)
> > > >      if (mode & P9_STAT_MODE_SETVTX) {
> > > >          ret |= S_ISVTX;
> > > >      }
> > > > +#endif
> > > >
> > > >      return ret;
> > > >  }
> > > > @@ -1182,6 +1202,7 @@ static uint32_t stat_to_v9mode(const struct
> > > > stat
> > > *stbuf)
> > > >          mode |= P9_STAT_MODE_DIR;
> > > >      }
> > > >
> > > > +#ifndef CONFIG_WIN32
> > > >      if (S_ISLNK(stbuf->st_mode)) {
> > > >          mode |= P9_STAT_MODE_SYMLINK;
> > > >      }
> > > > @@ -1193,11 +1214,13 @@ static uint32_t stat_to_v9mode(const
> > > > struct stat
> > > *stbuf)
> > > >      if (S_ISFIFO(stbuf->st_mode)) {
> > > >          mode |= P9_STAT_MODE_NAMED_PIPE;
> > > >      }
> > > > +#endif
> > > >
> > > >      if (S_ISBLK(stbuf->st_mode) || S_ISCHR(stbuf->st_mode)) {
> > > >          mode |= P9_STAT_MODE_DEVICE;
> > > >      }
> > > >
> > > > +#ifndef CONFIG_WIN32
> > > >      if (stbuf->st_mode & S_ISUID) {
> > > >          mode |= P9_STAT_MODE_SETUID;
> > > >      }
> > > > @@ -1209,6 +1232,7 @@ static uint32_t stat_to_v9mode(const struct
> > > > stat
> > > *stbuf)
> > > >      if (stbuf->st_mode & S_ISVTX) {
> > > >          mode |= P9_STAT_MODE_SETVTX;
> > > >      }
> > > > +#endif
> > > >
> > > >      return mode;
> > > >  }
> > > > @@ -1247,9 +1271,17 @@ static int coroutine_fn
> > > > stat_to_v9stat(V9fsPDU *pdu,
> > > V9fsPath *path,
> > > >              return err;
> > > >          }
> > > >      } else if (v9stat->mode & P9_STAT_MODE_DEVICE) {
> > > > +        unsigned maj, min;
> > > > +
> > > > +#ifndef CONFIG_WIN32
> > > > +        maj = major(stbuf->st_rdev);
> > > > +        min = minor(stbuf->st_rdev); #else
> > > > +        maj = min = 0;
> > > > +#endif
> > >
> > > Really?
> >
> > See above link.
> >
> > >
> > > >          v9fs_string_sprintf(&v9stat->extension, "%c %u %u",
> > > >                  S_ISCHR(stbuf->st_mode) ? 'c' : 'b',
> > > > -                major(stbuf->st_rdev), minor(stbuf->st_rdev));
> > > > +                maj, min);
> > > >      } else if (S_ISDIR(stbuf->st_mode) || S_ISREG(stbuf->st_mode)) {
> > > >          v9fs_string_sprintf(&v9stat->extension, "%s %lu",
> > > >                  "HARDLINKCOUNT", (unsigned long)stbuf->st_nlink);
> > > > @@
> > > > -1317,7 +1349,14 @@ static int32_t blksize_to_iounit(const V9fsPDU
> > > > *pdu, int32_t blksize)
> > > >
> > > >  static int32_t stat_to_iounit(const V9fsPDU *pdu, const struct
> > > > stat
> > > > *stbuf)  {
> > > > -    return blksize_to_iounit(pdu, stbuf->st_blksize);
> > > > +    int32_t blksize;
> > > > +
> > > > +#ifndef CONFIG_WIN32
> > > > +    blksize = stbuf->st_blksize); #else
> > > > +    blksize = 0;
> > > > +#endif
> > >
> > > Really?
> >
> > Windows struct stat does not have such field. See above link.
> 
> Yeah, but you cannot simply return zero here, because that information is
> interpreted on guest side as the optimum chunk size for I/O. So some apps
> might misbehave e.g. by trying allocate buffers with zero size, throwing
> division by zero exceptions, or trying to read() / write() with zero chunk
> size.
> 
> I'm pretty sure there is some kind of API to get the block size of the
> underlying drive on Windows. And if not, then something like 4k or 8k is
> still better than zero.
> 

The possible solution could be put a hard-code (e.g. 4096, 512) here.
This function does not have any context parameter for input.
To get block size, need a file handle or path. But this function does not have it.

> > >
> > > > +    return blksize_to_iounit(pdu, blksize);
> > > >  }
> > > >
> > > >  static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat
> > > > *stbuf, @@ -1332,7 +1371,11 @@ static int
> > > > stat_to_v9stat_dotl(V9fsPDU *pdu,
> > > const struct stat *stbuf,
> > > >      v9lstat->st_rdev = host_dev_to_dotl_dev(stbuf->st_rdev);
> > > >      v9lstat->st_size = stbuf->st_size;
> > > >      v9lstat->st_blksize = stat_to_iounit(pdu, stbuf);
> > > > +#ifndef CONFIG_WIN32
> > > >      v9lstat->st_blocks = stbuf->st_blocks;
> > > > +#else
> > > > +    v9lstat->st_blocks = 0;
> > > > +#endif
> > >
> > > Really?
> >
> > Windows struct stat does not have such field. See above link.
> 
> Then it probably has to be calculated by file size / block size.

Got it.

> 
> > >
> > > >      v9lstat->st_atime_sec = stbuf->st_atime;
> > > >      v9lstat->st_mtime_sec = stbuf->st_mtime;
> > > >      v9lstat->st_ctime_sec = stbuf->st_ctime; @@ -1340,7 +1383,8
> > > > @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat
> *stbuf,
> > > >      v9lstat->st_atime_nsec = stbuf->st_atimespec.tv_nsec;
> > > >      v9lstat->st_mtime_nsec = stbuf->st_mtimespec.tv_nsec;
> > > >      v9lstat->st_ctime_nsec = stbuf->st_ctimespec.tv_nsec; -#else
> > > > +#endif
> > > > +#ifdef CONFIG_LINUX
> > > >      v9lstat->st_atime_nsec = stbuf->st_atim.tv_nsec;
> > > >      v9lstat->st_mtime_nsec = stbuf->st_mtim.tv_nsec;
> > > >      v9lstat->st_ctime_nsec = stbuf->st_ctim.tv_nsec; @@ -2471,6
> > > > +2515,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu,
> > > V9fsFidState *fidp,
> > > >      struct dirent *dent;
> > > >      struct stat *st;
> > > >      struct V9fsDirEnt *entries = NULL;
> > > > +    unsigned char d_type = 0;
> > > >
> > > >      /*
> > > >       * inode remapping requires the device id, which in turn
> > > > might be @@ -2540,10 +2585,13 @@ static int coroutine_fn
> > > > v9fs_do_readdir(V9fsPDU
> > > *pdu, V9fsFidState *fidp,
> > > >          v9fs_string_init(&name);
> > > >          v9fs_string_sprintf(&name, "%s", dent->d_name);
> > > >
> > > > +#ifndef CONFIG_WIN32
> > > > +        d_type = dent->d_type;
> > > > +#endif
> > > >          /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count)
> */
> > > >          len = pdu_marshal(pdu, 11 + count, "Qqbs",
> > > >                            &qid, off,
> > > > -                          dent->d_type, &name);
> > > > +                          d_type, &name);
> > >
> > > Are you saying that d_type is not initialized with zero already?
> >
> > struct dirent is defined by MinGW, it does not have d_type member:
> >
> > https://github.com/Alexpux/mingw-w64/blob/master/mingw-w64-headers/crt
> > /dirent.h
> 
> My bad, I misread your code. That's fine.
> 
> >
> > >
> > > >          v9fs_string_free(&name);
> > > >
> > > > @@ -2873,8 +2921,12 @@ static void coroutine_fn v9fs_create(void
> *opaque)
> > > >          }
> > > >
> > > >          nmode |= perm & 0777;
> > > > +#ifndef CONFIG_WIN32
> > > >          err = v9fs_co_mknod(pdu, fidp, &name, fidp->uid, -1,
> > > >                              makedev(major, minor), nmode,
> > > > &stbuf);
> > > > +#else
> > > > +        err = -ENOTSUP;
> > > > +#endif
> > > >          if (err < 0) {
> > > >              goto out;
> > > >          }
> > > > @@ -2899,8 +2951,12 @@ static void coroutine_fn v9fs_create(void
> *opaque)
> > > >          v9fs_path_copy(&fidp->path, &path);
> > > >          v9fs_path_unlock(s);
> > > >      } else if (perm & P9_STAT_MODE_SOCKET) {
> > > > +#ifndef CONFIG_WIN32
> > > >          err = v9fs_co_mknod(pdu, fidp, &name, fidp->uid, -1,
> > > >                              0, S_IFSOCK | (perm & 0777), &stbuf);
> > > > +#else
> > > > +        err = -ENOTSUP;
> > > > +#endif
> > >
> > > As with previous patches, I would consider making the user aware to
> > > use
> > > mapped(-xattr) with something like error_report_once().
> >
> > OK, got it.
> >
> > >
> > > >          if (err < 0) {
> > > >              goto out;
> > > >          }
> > > > @@ -3634,7 +3690,7 @@ out_nofid:
> > > >
> > > >  static void coroutine_fn v9fs_mknod(void *opaque)  {
> > > > -
> > > > +#ifndef CONFIG_WIN32
> > > >      int mode;
> > > >      gid_t gid;
> > > >      int32_t fid;
> > > > @@ -3691,6 +3747,10 @@ out:
> > > >  out_nofid:
> > > >      pdu_complete(pdu, err);
> > > >      v9fs_string_free(&name);
> > > > +#else
> > > > +    V9fsPDU *pdu = opaque;
> > > > +    pdu_complete(pdu, -1);
> > > > +#endif
> > > >  }
> > > >
> > > >  /*
> > > > @@ -3963,7 +4023,7 @@ out_nofid:
> > > >  #if defined(CONFIG_LINUX)
> > > >  /* Currently, only Linux has XATTR_SIZE_MAX */  #define
> > > > P9_XATTR_SIZE_MAX XATTR_SIZE_MAX -#elif defined(CONFIG_DARWIN)
> > > > +#elif defined(CONFIG_DARWIN) || defined(CONFIG_WIN32)
> > > >  /*
> > > >   * Darwin doesn't seem to define a maximum xattr size in its user
> > > >   * space header, so manually configure it across platforms as 64k.
> > > > @@ -3980,6 +4040,7 @@ out_nofid:
> > > >
> > > >  static void coroutine_fn v9fs_xattrcreate(void *opaque)  {
> > > > +#ifndef CONFIG_WIN32
> > > >      int flags, rflags = 0;
> > > >      int32_t fid;
> > > >      uint64_t size;
> > > > @@ -4041,10 +4102,15 @@ out_put_fid:
> > > >  out_nofid:
> > > >      pdu_complete(pdu, err);
> > > >      v9fs_string_free(&name);
> > > > +#else
> > > > +    V9fsPDU *pdu = opaque;
> > > > +    pdu_complete(pdu, -1);
> > > > +#endif
> > > >  }
> > > >
> > > >  static void coroutine_fn v9fs_readlink(void *opaque)  {
> > > > +#ifndef CONFIG_WIN32
> > > >      V9fsPDU *pdu = opaque;
> > > >      size_t offset = 7;
> > > >      V9fsString target;
> > > > @@ -4080,6 +4146,10 @@ out:
> > > >      put_fid(pdu, fidp);
> > > >  out_nofid:
> > > >      pdu_complete(pdu, err);
> > > > +#else
> > > > +    V9fsPDU *pdu = opaque;
> > > > +    pdu_complete(pdu, -1);
> > > > +#endif
> > >
> > > Unnecessary double declaration of pdu.
> > >
> >
> > OK, got it.
> >
> > > >  }
> > > >
> > > >  static CoroutineEntry *pdu_co_handlers[] = { @@ -4341,6 +4411,7
> > > > @@ void v9fs_reset(V9fsState *s)
> > > >
> > > >  static void __attribute__((__constructor__))
> > > > v9fs_set_fd_limit(void) {
> > > > +#ifndef CONFIG_WIN32
> > > >      struct rlimit rlim;
> > > >      if (getrlimit(RLIMIT_NOFILE, &rlim) < 0) {
> > > >          error_report("Failed to get the resource limit"); @@
> > > > -4348,4
> > > > +4419,5 @@ static void __attribute__((__constructor__))
> > > v9fs_set_fd_limit(void)
> > > >      }
> > > >      open_fd_hw = rlim.rlim_cur - MIN(400, rlim.rlim_cur / 3);
> > > >      open_fd_rc = rlim.rlim_cur / 2;
> > > > +#endif
> > >
> > > Really?
> >
> > Windows does not provide getrlimit()
> 
> But it has _getmaxstdio() and _setmaxstdio().
> 
> And even if Windows had no replacement functions, you need to initialize
> these two global variables with some meaningful value. Otherwise they would
> be zero, and that would mean 9p server would assume max. 0 files could be
> open at the same time, so it would constantly close and re-open every single
> file descriptor on every minor micro-transaction for no reason.

I could implement this function by _getmaxstdio() and _setmaxstdio().
But these two functions are used for struct FILE, but not file descriptor (FD).
I wrote a basic test for this function:
  _getmaxstdio() returns 512, but I can use "open" to open at least 600 files without error.

Windows does not provide any document about the limititaion.

I think we can put a hard code here: 512, 1024, 2048, etc.


Thanks
Guohuai

> 
> Best regards,
> Christian Schoenebeck
>
Christian Schoenebeck Nov. 2, 2022, 11:34 a.m. UTC | #5 
On Wednesday, November 2, 2022 4:44:14 AM CET Shi, Guohuai wrote:
> 
> > -----Original Message-----
> > From: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > Sent: Wednesday, November 2, 2022 02:59
> > To: qemu-devel@nongnu.org
> > Cc: Greg Kurz <groug@kaod.org>; Meng, Bin <Bin.Meng@windriver.com>; Shi,
> > Guohuai <Guohuai.Shi@windriver.com>
> > Subject: Re: [PATCH 09/16] hw/9pfs: Disable unsupported flags and features
> > for Windows
> > 
> > [Please note: This e-mail is from an EXTERNAL e-mail address]
> > 
> > On Tuesday, November 1, 2022 4:34:54 PM CET Shi, Guohuai wrote:
> > >
> > > > -----Original Message-----
> > > > From: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > > Sent: Tuesday, November 1, 2022 23:04
> > > > To: qemu-devel@nongnu.org
> > > > Cc: Shi, Guohuai <Guohuai.Shi@windriver.com>; Greg Kurz
> > > > <groug@kaod.org>; Meng, Bin <Bin.Meng@windriver.com>
> > > > Subject: Re: [PATCH 09/16] hw/9pfs: Disable unsupported flags and
> > > > features for Windows
> > > >
> > > > [Please note: This e-mail is from an EXTERNAL e-mail address]
> > > >
> > > > On Monday, October 24, 2022 6:57:52 AM CET Bin Meng wrote:
> > > > > From: Guohuai Shi <guohuai.shi@windriver.com>
> > > > >
> > > > > Some flags and features are not supported on Windows, like mknod,
> > > > > readlink, file mode, etc. Update the codes for Windows.
> > > > >
> > > > > Signed-off-by: Guohuai Shi <guohuai.shi@windriver.com>
> > > > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > > > ---
> > > > >
> > > > >  hw/9pfs/9p-util.h |  6 +++-
> > > > >  hw/9pfs/9p.c      | 90 ++++++++++++++++++++++++++++++++++++++++++-----
> > > > >  2 files changed, 86 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h index
> > > > > 82b2d0c3e4..3d154e9103 100644
> > > > > --- a/hw/9pfs/9p-util.h
> > > > > +++ b/hw/9pfs/9p-util.h
> > > > > @@ -53,8 +53,10 @@ static inline uint64_t makedev_dotl(uint32_t
> > > > > dev_major,
> > > > uint32_t dev_minor)
> > > > >   */
> > > > >  static inline uint64_t host_dev_to_dotl_dev(dev_t dev)  { -#ifdef
> > > > > CONFIG_LINUX
> > > > > +#if defined(CONFIG_LINUX)
> > > > >      return dev;
> > > > > +#elif defined(CONFIG_WIN32)
> > > > > +    return 0;
> > > >
> > > > Really?
> > >
> > > Check MS this document:
> > > https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fsta
> > > t-fstat32-fstat64-fstati64-fstat32i64-fstat64i32?view=msvc-170
> > > st_rdev: If a device, fd; otherwise 0.
> > > st_dev: If a device, fd; otherwise 0.
> > >
> > > So for any file open, it should be 0.
> > 
> > Yeah, but that function translates a corresponding device ID for *Linux*
> > guest side. And the intention is to avoid e.g. file ID collisions on guest
> > side.
> > Because for a Linux guest, the two-tuple (device number, inode number) makes
> > a system-wide unique file ID.
> > 
> > If you just return zero here, that might be OK if only one 9p directory is
> > exported to guest, but say you have "C:\foo\" exported and "D:\bar\" exported
> > and mounted via 9p to guest, then guest would assume every file with the same
> > inode number on those two to be the same files. But they are not. They are on
> > two different drives actually.
> > 
> 
> Got it.
> Windows does not provide any numerical type ID for device, 
> I think the solution could be using driver letter ASC code "C:", "D:", etc.
> 
> > >
> > > >
> > > > >  #else
> > > > >      return makedev_dotl(major(dev), minor(dev));  #endif @@
> > > > > -260,7
> > > > > +262,9 @@ static inline struct dirent *qemu_dirent_dup(struct
> > > > > +dirent
> > > > > *dent)  #if defined CONFIG_DARWIN && defined
> > > > > CONFIG_PTHREAD_FCHDIR_NP int pthread_fchdir_np(int fd)
> > > > > __attribute__((weak_import));  #endif
> > > > > +#ifndef CONFIG_WIN32
> > > > >  int qemu_mknodat(P9_FILE_ID dirfd, const char *filename, mode_t mode,
> > > > >                   dev_t dev);
> > > > > +#endif
> > > > >
> > > > >  #endif
> > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index
> > > > > 6c4af86240..771aab34ac
> > > > > 100644
> > > > > --- a/hw/9pfs/9p.c
> > > > > +++ b/hw/9pfs/9p.c
> > > > > @@ -39,6 +39,11 @@
> > > > >  #include "qemu/xxhash.h"
> > > > >  #include <math.h>
> > > > >
> > > > > +#ifdef CONFIG_WIN32
> > > > > +#define UTIME_NOW   ((1l << 30) - 1l)
> > > > > +#define UTIME_OMIT  ((1l << 30) - 2l) #endif
> > > > > +
> > > > >  int open_fd_hw;
> > > > >  int total_open_fd;
> > > > >  static int open_fd_rc;
> > > > > @@ -132,13 +137,17 @@ static int dotl_to_open_flags(int flags)
> > > > >      DotlOpenflagMap dotl_oflag_map[] = {
> > > > >          { P9_DOTL_CREATE, O_CREAT },
> > > > >          { P9_DOTL_EXCL, O_EXCL },
> > > > > +#ifndef CONFIG_WIN32
> > > > >          { P9_DOTL_NOCTTY , O_NOCTTY },
> > > > > +#endif
> > > > >          { P9_DOTL_TRUNC, O_TRUNC },
> > > > >          { P9_DOTL_APPEND, O_APPEND },
> > > > > +#ifndef CONFIG_WIN32
> > > > >          { P9_DOTL_NONBLOCK, O_NONBLOCK } ,
> > > > >          { P9_DOTL_DSYNC, O_DSYNC },
> > > > >          { P9_DOTL_FASYNC, FASYNC }, -#ifndef CONFIG_DARWIN
> > > > > +#endif
> > > > > +#ifdef CONFIG_LINUX
> > > >
> > > > Better
> > > >
> > > >    #if !defined(CONFIG_DARWIN) && !defined(CONFIG_WIN32)
> > > >
> > >
> > > It is OK.
> > 
> > You got my point, hopefully:
> > 
> > > > Otherwise it might automatically opt-out other future platforms
> > > > unintentionally.
> > > >
> > > > >          { P9_DOTL_NOATIME, O_NOATIME },
> > > > >          /*
> > > > >           *  On Darwin, we could map to F_NOCACHE, which is @@
> > > > > -151,8
> > > > > +160,10 @@ static int dotl_to_open_flags(int flags)  #endif
> > > > >          { P9_DOTL_LARGEFILE, O_LARGEFILE },
> > > > >          { P9_DOTL_DIRECTORY, O_DIRECTORY },
> > > > > +#ifndef CONFIG_WIN32
> > > > >          { P9_DOTL_NOFOLLOW, O_NOFOLLOW },
> > > > >          { P9_DOTL_SYNC, O_SYNC },
> > > > > +#endif
> > > > >      };
> > > > >
> > > > >      for (i = 0; i < ARRAY_SIZE(dotl_oflag_map); i++) { @@ -179,8
> > > > > +190,11 @@ static int get_dotl_openflags(V9fsState *s, int oflags)
> > > > >       * Filter the client open flags
> > > > >       */
> > > > >      flags = dotl_to_open_flags(oflags);
> > > > > -    flags &= ~(O_NOCTTY | O_ASYNC | O_CREAT);
> > > > > -#ifndef CONFIG_DARWIN
> > > > > +    flags &= ~(O_CREAT);
> > > > > +#ifndef CONFIG_WIN32
> > > > > +    flags &= ~(O_NOCTTY | O_ASYNC); #endif #ifdef CONFIG_LINUX
> > > >
> > > > Same as above: better explicitly opt-out than the other way around.
> > > >
> > >
> > > It is OK.
> > >
> > > > >      /*
> > > > >       * Ignore direct disk access hint until the server supports it.
> > > > >       */
> > > > > @@ -986,9 +1000,11 @@ static int stat_to_qid(V9fsPDU *pdu, const
> > > > > struct
> > > > stat *stbuf, V9fsQID *qidp)
> > > > >      if (S_ISDIR(stbuf->st_mode)) {
> > > > >          qidp->type |= P9_QID_TYPE_DIR;
> > > > >      }
> > > > > +#ifndef CONFIG_WIN32
> > > > >      if (S_ISLNK(stbuf->st_mode)) {
> > > > >          qidp->type |= P9_QID_TYPE_SYMLINK;
> > > > >      }
> > > > > +#endif
> > > > >
> > > > >      return 0;
> > > > >  }
> > > > > @@ -1097,6 +1113,7 @@ static mode_t v9mode_to_mode(uint32_t mode,
> > > > V9fsString *extension)
> > > > >          ret |= S_IFDIR;
> > > > >      }
> > > > >
> > > > > +#ifndef CONFIG_WIN32
> > > > >      if (mode & P9_STAT_MODE_SYMLINK) {
> > > > >          ret |= S_IFLNK;
> > > > >      }
> > > > > @@ -1106,6 +1123,7 @@ static mode_t v9mode_to_mode(uint32_t mode,
> > > > V9fsString *extension)
> > > > >      if (mode & P9_STAT_MODE_NAMED_PIPE) {
> > > > >          ret |= S_IFIFO;
> > > > >      }
> > > > > +#endif
> > > > >      if (mode & P9_STAT_MODE_DEVICE) {
> > > > >          if (extension->size && extension->data[0] == 'c') {
> > > > >              ret |= S_IFCHR;
> > > > > @@ -1118,6 +1136,7 @@ static mode_t v9mode_to_mode(uint32_t mode,
> > > > V9fsString *extension)
> > > > >          ret |= S_IFREG;
> > > > >      }
> > > > >
> > > > > +#ifndef CONFIG_WIN32
> > > > >      if (mode & P9_STAT_MODE_SETUID) {
> > > > >          ret |= S_ISUID;
> > > > >      }
> > > > > @@ -1127,6 +1146,7 @@ static mode_t v9mode_to_mode(uint32_t mode,
> > > > V9fsString *extension)
> > > > >      if (mode & P9_STAT_MODE_SETVTX) {
> > > > >          ret |= S_ISVTX;
> > > > >      }
> > > > > +#endif
> > > > >
> > > > >      return ret;
> > > > >  }
> > > > > @@ -1182,6 +1202,7 @@ static uint32_t stat_to_v9mode(const struct
> > > > > stat
> > > > *stbuf)
> > > > >          mode |= P9_STAT_MODE_DIR;
> > > > >      }
> > > > >
> > > > > +#ifndef CONFIG_WIN32
> > > > >      if (S_ISLNK(stbuf->st_mode)) {
> > > > >          mode |= P9_STAT_MODE_SYMLINK;
> > > > >      }
> > > > > @@ -1193,11 +1214,13 @@ static uint32_t stat_to_v9mode(const
> > > > > struct stat
> > > > *stbuf)
> > > > >      if (S_ISFIFO(stbuf->st_mode)) {
> > > > >          mode |= P9_STAT_MODE_NAMED_PIPE;
> > > > >      }
> > > > > +#endif
> > > > >
> > > > >      if (S_ISBLK(stbuf->st_mode) || S_ISCHR(stbuf->st_mode)) {
> > > > >          mode |= P9_STAT_MODE_DEVICE;
> > > > >      }
> > > > >
> > > > > +#ifndef CONFIG_WIN32
> > > > >      if (stbuf->st_mode & S_ISUID) {
> > > > >          mode |= P9_STAT_MODE_SETUID;
> > > > >      }
> > > > > @@ -1209,6 +1232,7 @@ static uint32_t stat_to_v9mode(const struct
> > > > > stat
> > > > *stbuf)
> > > > >      if (stbuf->st_mode & S_ISVTX) {
> > > > >          mode |= P9_STAT_MODE_SETVTX;
> > > > >      }
> > > > > +#endif
> > > > >
> > > > >      return mode;
> > > > >  }
> > > > > @@ -1247,9 +1271,17 @@ static int coroutine_fn
> > > > > stat_to_v9stat(V9fsPDU *pdu,
> > > > V9fsPath *path,
> > > > >              return err;
> > > > >          }
> > > > >      } else if (v9stat->mode & P9_STAT_MODE_DEVICE) {
> > > > > +        unsigned maj, min;
> > > > > +
> > > > > +#ifndef CONFIG_WIN32
> > > > > +        maj = major(stbuf->st_rdev);
> > > > > +        min = minor(stbuf->st_rdev); #else
> > > > > +        maj = min = 0;
> > > > > +#endif
> > > >
> > > > Really?
> > >
> > > See above link.
> > >
> > > >
> > > > >          v9fs_string_sprintf(&v9stat->extension, "%c %u %u",
> > > > >                  S_ISCHR(stbuf->st_mode) ? 'c' : 'b',
> > > > > -                major(stbuf->st_rdev), minor(stbuf->st_rdev));
> > > > > +                maj, min);
> > > > >      } else if (S_ISDIR(stbuf->st_mode) || S_ISREG(stbuf->st_mode)) {
> > > > >          v9fs_string_sprintf(&v9stat->extension, "%s %lu",
> > > > >                  "HARDLINKCOUNT", (unsigned long)stbuf->st_nlink);
> > > > > @@
> > > > > -1317,7 +1349,14 @@ static int32_t blksize_to_iounit(const V9fsPDU
> > > > > *pdu, int32_t blksize)
> > > > >
> > > > >  static int32_t stat_to_iounit(const V9fsPDU *pdu, const struct
> > > > > stat
> > > > > *stbuf)  {
> > > > > -    return blksize_to_iounit(pdu, stbuf->st_blksize);
> > > > > +    int32_t blksize;
> > > > > +
> > > > > +#ifndef CONFIG_WIN32
> > > > > +    blksize = stbuf->st_blksize); #else
> > > > > +    blksize = 0;
> > > > > +#endif
> > > >
> > > > Really?
> > >
> > > Windows struct stat does not have such field. See above link.
> > 
> > Yeah, but you cannot simply return zero here, because that information is
> > interpreted on guest side as the optimum chunk size for I/O. So some apps
> > might misbehave e.g. by trying allocate buffers with zero size, throwing
> > division by zero exceptions, or trying to read() / write() with zero chunk
> > size.
> > 
> > I'm pretty sure there is some kind of API to get the block size of the
> > underlying drive on Windows. And if not, then something like 4k or 8k is
> > still better than zero.
> > 
> 
> The possible solution could be put a hard-code (e.g. 4096, 512) here.
> This function does not have any context parameter for input.
> To get block size, need a file handle or path. But this function does not have it.

Another major difference on Windows compared to other systems is that you
don't have to worry about potential submounts in the exported directory tree.
Because Windows does not have that concept. So on Windows you can assume that
all files of the exported file tree are on the same storage device, right?

So another solution would be to query the block size with a regular Windows
API only on startup, simply by using the export root, and then storing that
block size e.g. as a new member variable to struct V9fsState.

In the end you could actually also use some meaningful hard coded value for
now, as it is not such of a big deal whether that's 512, 4k or 8k. But zero is
clearly a no go, for the reasons previously described.

> > > >
> > > > > +    return blksize_to_iounit(pdu, blksize);
> > > > >  }
> > > > >
> > > > >  static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat
> > > > > *stbuf, @@ -1332,7 +1371,11 @@ static int
> > > > > stat_to_v9stat_dotl(V9fsPDU *pdu,
> > > > const struct stat *stbuf,
> > > > >      v9lstat->st_rdev = host_dev_to_dotl_dev(stbuf->st_rdev);
> > > > >      v9lstat->st_size = stbuf->st_size;
> > > > >      v9lstat->st_blksize = stat_to_iounit(pdu, stbuf);
> > > > > +#ifndef CONFIG_WIN32
> > > > >      v9lstat->st_blocks = stbuf->st_blocks;
> > > > > +#else
> > > > > +    v9lstat->st_blocks = 0;
> > > > > +#endif
> > > >
> > > > Really?
> > >
> > > Windows struct stat does not have such field. See above link.
> > 
> > Then it probably has to be calculated by file size / block size.
> 
> Got it.
> 
> > 
> > > >
> > > > >      v9lstat->st_atime_sec = stbuf->st_atime;
> > > > >      v9lstat->st_mtime_sec = stbuf->st_mtime;
> > > > >      v9lstat->st_ctime_sec = stbuf->st_ctime; @@ -1340,7 +1383,8
> > > > > @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat
> > *stbuf,
> > > > >      v9lstat->st_atime_nsec = stbuf->st_atimespec.tv_nsec;
> > > > >      v9lstat->st_mtime_nsec = stbuf->st_mtimespec.tv_nsec;
> > > > >      v9lstat->st_ctime_nsec = stbuf->st_ctimespec.tv_nsec; -#else
> > > > > +#endif
> > > > > +#ifdef CONFIG_LINUX
> > > > >      v9lstat->st_atime_nsec = stbuf->st_atim.tv_nsec;
> > > > >      v9lstat->st_mtime_nsec = stbuf->st_mtim.tv_nsec;
> > > > >      v9lstat->st_ctime_nsec = stbuf->st_ctim.tv_nsec; @@ -2471,6
> > > > > +2515,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu,
> > > > V9fsFidState *fidp,
> > > > >      struct dirent *dent;
> > > > >      struct stat *st;
> > > > >      struct V9fsDirEnt *entries = NULL;
> > > > > +    unsigned char d_type = 0;
> > > > >
> > > > >      /*
> > > > >       * inode remapping requires the device id, which in turn
> > > > > might be @@ -2540,10 +2585,13 @@ static int coroutine_fn
> > > > > v9fs_do_readdir(V9fsPDU
> > > > *pdu, V9fsFidState *fidp,
> > > > >          v9fs_string_init(&name);
> > > > >          v9fs_string_sprintf(&name, "%s", dent->d_name);
> > > > >
> > > > > +#ifndef CONFIG_WIN32
> > > > > +        d_type = dent->d_type;
> > > > > +#endif
> > > > >          /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count)
> > */
> > > > >          len = pdu_marshal(pdu, 11 + count, "Qqbs",
> > > > >                            &qid, off,
> > > > > -                          dent->d_type, &name);
> > > > > +                          d_type, &name);
> > > >
> > > > Are you saying that d_type is not initialized with zero already?
> > >
> > > struct dirent is defined by MinGW, it does not have d_type member:
> > >
> > > https://github.com/Alexpux/mingw-w64/blob/master/mingw-w64-headers/crt
> > > /dirent.h
> > 
> > My bad, I misread your code. That's fine.
> > 
> > >
> > > >
> > > > >          v9fs_string_free(&name);
> > > > >
> > > > > @@ -2873,8 +2921,12 @@ static void coroutine_fn v9fs_create(void
> > *opaque)
> > > > >          }
> > > > >
> > > > >          nmode |= perm & 0777;
> > > > > +#ifndef CONFIG_WIN32
> > > > >          err = v9fs_co_mknod(pdu, fidp, &name, fidp->uid, -1,
> > > > >                              makedev(major, minor), nmode,
> > > > > &stbuf);
> > > > > +#else
> > > > > +        err = -ENOTSUP;
> > > > > +#endif
> > > > >          if (err < 0) {
> > > > >              goto out;
> > > > >          }
> > > > > @@ -2899,8 +2951,12 @@ static void coroutine_fn v9fs_create(void
> > *opaque)
> > > > >          v9fs_path_copy(&fidp->path, &path);
> > > > >          v9fs_path_unlock(s);
> > > > >      } else if (perm & P9_STAT_MODE_SOCKET) {
> > > > > +#ifndef CONFIG_WIN32
> > > > >          err = v9fs_co_mknod(pdu, fidp, &name, fidp->uid, -1,
> > > > >                              0, S_IFSOCK | (perm & 0777), &stbuf);
> > > > > +#else
> > > > > +        err = -ENOTSUP;
> > > > > +#endif
> > > >
> > > > As with previous patches, I would consider making the user aware to
> > > > use
> > > > mapped(-xattr) with something like error_report_once().
> > >
> > > OK, got it.
> > >
> > > >
> > > > >          if (err < 0) {
> > > > >              goto out;
> > > > >          }
> > > > > @@ -3634,7 +3690,7 @@ out_nofid:
> > > > >
> > > > >  static void coroutine_fn v9fs_mknod(void *opaque)  {
> > > > > -
> > > > > +#ifndef CONFIG_WIN32
> > > > >      int mode;
> > > > >      gid_t gid;
> > > > >      int32_t fid;
> > > > > @@ -3691,6 +3747,10 @@ out:
> > > > >  out_nofid:
> > > > >      pdu_complete(pdu, err);
> > > > >      v9fs_string_free(&name);
> > > > > +#else
> > > > > +    V9fsPDU *pdu = opaque;
> > > > > +    pdu_complete(pdu, -1);
> > > > > +#endif
> > > > >  }
> > > > >
> > > > >  /*
> > > > > @@ -3963,7 +4023,7 @@ out_nofid:
> > > > >  #if defined(CONFIG_LINUX)
> > > > >  /* Currently, only Linux has XATTR_SIZE_MAX */  #define
> > > > > P9_XATTR_SIZE_MAX XATTR_SIZE_MAX -#elif defined(CONFIG_DARWIN)
> > > > > +#elif defined(CONFIG_DARWIN) || defined(CONFIG_WIN32)
> > > > >  /*
> > > > >   * Darwin doesn't seem to define a maximum xattr size in its user
> > > > >   * space header, so manually configure it across platforms as 64k.
> > > > > @@ -3980,6 +4040,7 @@ out_nofid:
> > > > >
> > > > >  static void coroutine_fn v9fs_xattrcreate(void *opaque)  {
> > > > > +#ifndef CONFIG_WIN32
> > > > >      int flags, rflags = 0;
> > > > >      int32_t fid;
> > > > >      uint64_t size;
> > > > > @@ -4041,10 +4102,15 @@ out_put_fid:
> > > > >  out_nofid:
> > > > >      pdu_complete(pdu, err);
> > > > >      v9fs_string_free(&name);
> > > > > +#else
> > > > > +    V9fsPDU *pdu = opaque;
> > > > > +    pdu_complete(pdu, -1);
> > > > > +#endif
> > > > >  }
> > > > >
> > > > >  static void coroutine_fn v9fs_readlink(void *opaque)  {
> > > > > +#ifndef CONFIG_WIN32
> > > > >      V9fsPDU *pdu = opaque;
> > > > >      size_t offset = 7;
> > > > >      V9fsString target;
> > > > > @@ -4080,6 +4146,10 @@ out:
> > > > >      put_fid(pdu, fidp);
> > > > >  out_nofid:
> > > > >      pdu_complete(pdu, err);
> > > > > +#else
> > > > > +    V9fsPDU *pdu = opaque;
> > > > > +    pdu_complete(pdu, -1);
> > > > > +#endif
> > > >
> > > > Unnecessary double declaration of pdu.
> > > >
> > >
> > > OK, got it.
> > >
> > > > >  }
> > > > >
> > > > >  static CoroutineEntry *pdu_co_handlers[] = { @@ -4341,6 +4411,7
> > > > > @@ void v9fs_reset(V9fsState *s)
> > > > >
> > > > >  static void __attribute__((__constructor__))
> > > > > v9fs_set_fd_limit(void) {
> > > > > +#ifndef CONFIG_WIN32
> > > > >      struct rlimit rlim;
> > > > >      if (getrlimit(RLIMIT_NOFILE, &rlim) < 0) {
> > > > >          error_report("Failed to get the resource limit"); @@
> > > > > -4348,4
> > > > > +4419,5 @@ static void __attribute__((__constructor__))
> > > > v9fs_set_fd_limit(void)
> > > > >      }
> > > > >      open_fd_hw = rlim.rlim_cur - MIN(400, rlim.rlim_cur / 3);
> > > > >      open_fd_rc = rlim.rlim_cur / 2;
> > > > > +#endif
> > > >
> > > > Really?
> > >
> > > Windows does not provide getrlimit()
> > 
> > But it has _getmaxstdio() and _setmaxstdio().
> > 
> > And even if Windows had no replacement functions, you need to initialize
> > these two global variables with some meaningful value. Otherwise they would
> > be zero, and that would mean 9p server would assume max. 0 files could be
> > open at the same time, so it would constantly close and re-open every single
> > file descriptor on every minor micro-transaction for no reason.
> 
> I could implement this function by _getmaxstdio() and _setmaxstdio().
> But these two functions are used for struct FILE, but not file descriptor (FD).
> I wrote a basic test for this function:
>   _getmaxstdio() returns 512, but I can use "open" to open at least 600 files without error.
> 
> Windows does not provide any document about the limititaion.
> 
> I think we can put a hard code here: 512, 1024, 2048, etc.

Yeah, that's pretty badly documented, I agree. The fact that it's prefixed
with an underscore suggests that they were probably not too convinced about
their API either. But it is as it is.

The fact that you can open a bit more files than what this function advertises
is not a huge surprise or speaking against using it though. Because it is not
untypical for systems to allow overallocation of system resources to a certain
extent.

At least from what I can see right now, this function is commonly used on
Windows to get exactly that metric. So I would rather use that than using hard
coded values, because from what I can see on the web, the numbers people got
experimentally are varying quite a bit.

Best regards,
Christian Schoenebeck
Shi, Guohuai Nov. 2, 2022, 12:19 p.m. UTC | #6 
> -----Original Message-----
> From: Christian Schoenebeck <qemu_oss@crudebyte.com>
> Sent: Wednesday, November 2, 2022 19:34
> To: qemu-devel@nongnu.org
> Cc: Greg Kurz <groug@kaod.org>; Meng, Bin <Bin.Meng@windriver.com>; Shi,
> Guohuai <Guohuai.Shi@windriver.com>
> Subject: Re: [PATCH 09/16] hw/9pfs: Disable unsupported flags and features
> for Windows
> 
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and
> know the content is safe.
> 
> On Wednesday, November 2, 2022 4:44:14 AM CET Shi, Guohuai wrote:
> >
> > > -----Original Message-----
> > > From: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > Sent: Wednesday, November 2, 2022 02:59
> > > To: qemu-devel@nongnu.org
> > > Cc: Greg Kurz <groug@kaod.org>; Meng, Bin <Bin.Meng@windriver.com>;
> > > Shi, Guohuai <Guohuai.Shi@windriver.com>
> > > Subject: Re: [PATCH 09/16] hw/9pfs: Disable unsupported flags and
> > > features for Windows
> > >
> > > [Please note: This e-mail is from an EXTERNAL e-mail address]
> > >
> > > On Tuesday, November 1, 2022 4:34:54 PM CET Shi, Guohuai wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > > > Sent: Tuesday, November 1, 2022 23:04
> > > > > To: qemu-devel@nongnu.org
> > > > > Cc: Shi, Guohuai <Guohuai.Shi@windriver.com>; Greg Kurz
> > > > > <groug@kaod.org>; Meng, Bin <Bin.Meng@windriver.com>
> > > > > Subject: Re: [PATCH 09/16] hw/9pfs: Disable unsupported flags
> > > > > and features for Windows
> > > > >
> > > > > [Please note: This e-mail is from an EXTERNAL e-mail address]
> > > > >
> > > > > On Monday, October 24, 2022 6:57:52 AM CET Bin Meng wrote:
> > > > > > From: Guohuai Shi <guohuai.shi@windriver.com>
> > > > > >
> > > > > > Some flags and features are not supported on Windows, like
> > > > > > mknod, readlink, file mode, etc. Update the codes for Windows.
> > > > > >
> > > > > > Signed-off-by: Guohuai Shi <guohuai.shi@windriver.com>
> > > > > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > > > > ---
> > > > > >
> > > > > >  hw/9pfs/9p-util.h |  6 +++-
> > > > > >  hw/9pfs/9p.c      | 90 ++++++++++++++++++++++++++++++++++++++++++-
> ----
> > > > > >  2 files changed, 86 insertions(+), 10 deletions(-)
> > > > > >
> > > > > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h index
> > > > > > 82b2d0c3e4..3d154e9103 100644
> > > > > > --- a/hw/9pfs/9p-util.h
> > > > > > +++ b/hw/9pfs/9p-util.h
> > > > > > @@ -53,8 +53,10 @@ static inline uint64_t
> > > > > > makedev_dotl(uint32_t dev_major,
> > > > > uint32_t dev_minor)
> > > > > >   */
> > > > > >  static inline uint64_t host_dev_to_dotl_dev(dev_t dev)  {
> > > > > > -#ifdef CONFIG_LINUX
> > > > > > +#if defined(CONFIG_LINUX)
> > > > > >      return dev;
> > > > > > +#elif defined(CONFIG_WIN32)
> > > > > > +    return 0;
> > > > >
> > > > > Really?
> > > >
> > > > Check MS this document:
> > > > https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/
> > > > fsta
> > > > t-fstat32-fstat64-fstati64-fstat32i64-fstat64i32?view=msvc-170
> > > > st_rdev: If a device, fd; otherwise 0.
> > > > st_dev: If a device, fd; otherwise 0.
> > > >
> > > > So for any file open, it should be 0.
> > >
> > > Yeah, but that function translates a corresponding device ID for
> > > *Linux* guest side. And the intention is to avoid e.g. file ID
> > > collisions on guest side.
> > > Because for a Linux guest, the two-tuple (device number, inode
> > > number) makes a system-wide unique file ID.
> > >
> > > If you just return zero here, that might be OK if only one 9p
> > > directory is exported to guest, but say you have "C:\foo\" exported
> > > and "D:\bar\" exported and mounted via 9p to guest, then guest would
> > > assume every file with the same inode number on those two to be the
> > > same files. But they are not. They are on two different drives actually.
> > >
> >
> > Got it.
> > Windows does not provide any numerical type ID for device, I think the
> > solution could be using driver letter ASC code "C:", "D:", etc.
> >
> > > >
> > > > >
> > > > > >  #else
> > > > > >      return makedev_dotl(major(dev), minor(dev));  #endif @@
> > > > > > -260,7
> > > > > > +262,9 @@ static inline struct dirent *qemu_dirent_dup(struct
> > > > > > +dirent
> > > > > > *dent)  #if defined CONFIG_DARWIN && defined
> > > > > > CONFIG_PTHREAD_FCHDIR_NP int pthread_fchdir_np(int fd)
> > > > > > __attribute__((weak_import));  #endif
> > > > > > +#ifndef CONFIG_WIN32
> > > > > >  int qemu_mknodat(P9_FILE_ID dirfd, const char *filename, mode_t
> mode,
> > > > > >                   dev_t dev);
> > > > > > +#endif
> > > > > >
> > > > > >  #endif
> > > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index
> > > > > > 6c4af86240..771aab34ac
> > > > > > 100644
> > > > > > --- a/hw/9pfs/9p.c
> > > > > > +++ b/hw/9pfs/9p.c
> > > > > > @@ -39,6 +39,11 @@
> > > > > >  #include "qemu/xxhash.h"
> > > > > >  #include <math.h>
> > > > > >
> > > > > > +#ifdef CONFIG_WIN32
> > > > > > +#define UTIME_NOW   ((1l << 30) - 1l)
> > > > > > +#define UTIME_OMIT  ((1l << 30) - 2l) #endif
> > > > > > +
> > > > > >  int open_fd_hw;
> > > > > >  int total_open_fd;
> > > > > >  static int open_fd_rc;
> > > > > > @@ -132,13 +137,17 @@ static int dotl_to_open_flags(int flags)
> > > > > >      DotlOpenflagMap dotl_oflag_map[] = {
> > > > > >          { P9_DOTL_CREATE, O_CREAT },
> > > > > >          { P9_DOTL_EXCL, O_EXCL },
> > > > > > +#ifndef CONFIG_WIN32
> > > > > >          { P9_DOTL_NOCTTY , O_NOCTTY },
> > > > > > +#endif
> > > > > >          { P9_DOTL_TRUNC, O_TRUNC },
> > > > > >          { P9_DOTL_APPEND, O_APPEND },
> > > > > > +#ifndef CONFIG_WIN32
> > > > > >          { P9_DOTL_NONBLOCK, O_NONBLOCK } ,
> > > > > >          { P9_DOTL_DSYNC, O_DSYNC },
> > > > > >          { P9_DOTL_FASYNC, FASYNC }, -#ifndef CONFIG_DARWIN
> > > > > > +#endif
> > > > > > +#ifdef CONFIG_LINUX
> > > > >
> > > > > Better
> > > > >
> > > > >    #if !defined(CONFIG_DARWIN) && !defined(CONFIG_WIN32)
> > > > >
> > > >
> > > > It is OK.
> > >
> > > You got my point, hopefully:
> > >
> > > > > Otherwise it might automatically opt-out other future platforms
> > > > > unintentionally.
> > > > >
> > > > > >          { P9_DOTL_NOATIME, O_NOATIME },
> > > > > >          /*
> > > > > >           *  On Darwin, we could map to F_NOCACHE, which is @@
> > > > > > -151,8
> > > > > > +160,10 @@ static int dotl_to_open_flags(int flags)  #endif
> > > > > >          { P9_DOTL_LARGEFILE, O_LARGEFILE },
> > > > > >          { P9_DOTL_DIRECTORY, O_DIRECTORY },
> > > > > > +#ifndef CONFIG_WIN32
> > > > > >          { P9_DOTL_NOFOLLOW, O_NOFOLLOW },
> > > > > >          { P9_DOTL_SYNC, O_SYNC },
> > > > > > +#endif
> > > > > >      };
> > > > > >
> > > > > >      for (i = 0; i < ARRAY_SIZE(dotl_oflag_map); i++) { @@
> > > > > > -179,8
> > > > > > +190,11 @@ static int get_dotl_openflags(V9fsState *s, int
> > > > > > +oflags)
> > > > > >       * Filter the client open flags
> > > > > >       */
> > > > > >      flags = dotl_to_open_flags(oflags);
> > > > > > -    flags &= ~(O_NOCTTY | O_ASYNC | O_CREAT);
> > > > > > -#ifndef CONFIG_DARWIN
> > > > > > +    flags &= ~(O_CREAT);
> > > > > > +#ifndef CONFIG_WIN32
> > > > > > +    flags &= ~(O_NOCTTY | O_ASYNC); #endif #ifdef
> > > > > > +CONFIG_LINUX
> > > > >
> > > > > Same as above: better explicitly opt-out than the other way around.
> > > > >
> > > >
> > > > It is OK.
> > > >
> > > > > >      /*
> > > > > >       * Ignore direct disk access hint until the server supports it.
> > > > > >       */
> > > > > > @@ -986,9 +1000,11 @@ static int stat_to_qid(V9fsPDU *pdu,
> > > > > > const struct
> > > > > stat *stbuf, V9fsQID *qidp)
> > > > > >      if (S_ISDIR(stbuf->st_mode)) {
> > > > > >          qidp->type |= P9_QID_TYPE_DIR;
> > > > > >      }
> > > > > > +#ifndef CONFIG_WIN32
> > > > > >      if (S_ISLNK(stbuf->st_mode)) {
> > > > > >          qidp->type |= P9_QID_TYPE_SYMLINK;
> > > > > >      }
> > > > > > +#endif
> > > > > >
> > > > > >      return 0;
> > > > > >  }
> > > > > > @@ -1097,6 +1113,7 @@ static mode_t v9mode_to_mode(uint32_t
> > > > > > mode,
> > > > > V9fsString *extension)
> > > > > >          ret |= S_IFDIR;
> > > > > >      }
> > > > > >
> > > > > > +#ifndef CONFIG_WIN32
> > > > > >      if (mode & P9_STAT_MODE_SYMLINK) {
> > > > > >          ret |= S_IFLNK;
> > > > > >      }
> > > > > > @@ -1106,6 +1123,7 @@ static mode_t v9mode_to_mode(uint32_t
> > > > > > mode,
> > > > > V9fsString *extension)
> > > > > >      if (mode & P9_STAT_MODE_NAMED_PIPE) {
> > > > > >          ret |= S_IFIFO;
> > > > > >      }
> > > > > > +#endif
> > > > > >      if (mode & P9_STAT_MODE_DEVICE) {
> > > > > >          if (extension->size && extension->data[0] == 'c') {
> > > > > >              ret |= S_IFCHR;
> > > > > > @@ -1118,6 +1136,7 @@ static mode_t v9mode_to_mode(uint32_t
> > > > > > mode,
> > > > > V9fsString *extension)
> > > > > >          ret |= S_IFREG;
> > > > > >      }
> > > > > >
> > > > > > +#ifndef CONFIG_WIN32
> > > > > >      if (mode & P9_STAT_MODE_SETUID) {
> > > > > >          ret |= S_ISUID;
> > > > > >      }
> > > > > > @@ -1127,6 +1146,7 @@ static mode_t v9mode_to_mode(uint32_t
> > > > > > mode,
> > > > > V9fsString *extension)
> > > > > >      if (mode & P9_STAT_MODE_SETVTX) {
> > > > > >          ret |= S_ISVTX;
> > > > > >      }
> > > > > > +#endif
> > > > > >
> > > > > >      return ret;
> > > > > >  }
> > > > > > @@ -1182,6 +1202,7 @@ static uint32_t stat_to_v9mode(const
> > > > > > struct stat
> > > > > *stbuf)
> > > > > >          mode |= P9_STAT_MODE_DIR;
> > > > > >      }
> > > > > >
> > > > > > +#ifndef CONFIG_WIN32
> > > > > >      if (S_ISLNK(stbuf->st_mode)) {
> > > > > >          mode |= P9_STAT_MODE_SYMLINK;
> > > > > >      }
> > > > > > @@ -1193,11 +1214,13 @@ static uint32_t stat_to_v9mode(const
> > > > > > struct stat
> > > > > *stbuf)
> > > > > >      if (S_ISFIFO(stbuf->st_mode)) {
> > > > > >          mode |= P9_STAT_MODE_NAMED_PIPE;
> > > > > >      }
> > > > > > +#endif
> > > > > >
> > > > > >      if (S_ISBLK(stbuf->st_mode) || S_ISCHR(stbuf->st_mode)) {
> > > > > >          mode |= P9_STAT_MODE_DEVICE;
> > > > > >      }
> > > > > >
> > > > > > +#ifndef CONFIG_WIN32
> > > > > >      if (stbuf->st_mode & S_ISUID) {
> > > > > >          mode |= P9_STAT_MODE_SETUID;
> > > > > >      }
> > > > > > @@ -1209,6 +1232,7 @@ static uint32_t stat_to_v9mode(const
> > > > > > struct stat
> > > > > *stbuf)
> > > > > >      if (stbuf->st_mode & S_ISVTX) {
> > > > > >          mode |= P9_STAT_MODE_SETVTX;
> > > > > >      }
> > > > > > +#endif
> > > > > >
> > > > > >      return mode;
> > > > > >  }
> > > > > > @@ -1247,9 +1271,17 @@ static int coroutine_fn
> > > > > > stat_to_v9stat(V9fsPDU *pdu,
> > > > > V9fsPath *path,
> > > > > >              return err;
> > > > > >          }
> > > > > >      } else if (v9stat->mode & P9_STAT_MODE_DEVICE) {
> > > > > > +        unsigned maj, min;
> > > > > > +
> > > > > > +#ifndef CONFIG_WIN32
> > > > > > +        maj = major(stbuf->st_rdev);
> > > > > > +        min = minor(stbuf->st_rdev); #else
> > > > > > +        maj = min = 0;
> > > > > > +#endif
> > > > >
> > > > > Really?
> > > >
> > > > See above link.
> > > >
> > > > >
> > > > > >          v9fs_string_sprintf(&v9stat->extension, "%c %u %u",
> > > > > >                  S_ISCHR(stbuf->st_mode) ? 'c' : 'b',
> > > > > > -                major(stbuf->st_rdev), minor(stbuf->st_rdev));
> > > > > > +                maj, min);
> > > > > >      } else if (S_ISDIR(stbuf->st_mode) || S_ISREG(stbuf->st_mode))
> {
> > > > > >          v9fs_string_sprintf(&v9stat->extension, "%s %lu",
> > > > > >                  "HARDLINKCOUNT", (unsigned
> > > > > > long)stbuf->st_nlink); @@
> > > > > > -1317,7 +1349,14 @@ static int32_t blksize_to_iounit(const
> > > > > > V9fsPDU *pdu, int32_t blksize)
> > > > > >
> > > > > >  static int32_t stat_to_iounit(const V9fsPDU *pdu, const
> > > > > > struct stat
> > > > > > *stbuf)  {
> > > > > > -    return blksize_to_iounit(pdu, stbuf->st_blksize);
> > > > > > +    int32_t blksize;
> > > > > > +
> > > > > > +#ifndef CONFIG_WIN32
> > > > > > +    blksize = stbuf->st_blksize); #else
> > > > > > +    blksize = 0;
> > > > > > +#endif
> > > > >
> > > > > Really?
> > > >
> > > > Windows struct stat does not have such field. See above link.
> > >
> > > Yeah, but you cannot simply return zero here, because that
> > > information is interpreted on guest side as the optimum chunk size
> > > for I/O. So some apps might misbehave e.g. by trying allocate
> > > buffers with zero size, throwing division by zero exceptions, or
> > > trying to read() / write() with zero chunk size.
> > >
> > > I'm pretty sure there is some kind of API to get the block size of
> > > the underlying drive on Windows. And if not, then something like 4k
> > > or 8k is still better than zero.
> > >
> >
> > The possible solution could be put a hard-code (e.g. 4096, 512) here.
> > This function does not have any context parameter for input.
> > To get block size, need a file handle or path. But this function does not
> have it.
> 
> Another major difference on Windows compared to other systems is that you
> don't have to worry about potential submounts in the exported directory tree.
> Because Windows does not have that concept. So on Windows you can assume that
> all files of the exported file tree are on the same storage device, right?
> 
> So another solution would be to query the block size with a regular Windows
> API only on startup, simply by using the export root, and then storing that
> block size e.g. as a new member variable to struct V9fsState.
> 
> In the end you could actually also use some meaningful hard coded value for
> now, as it is not such of a big deal whether that's 512, 4k or 8k. But zero
> is clearly a no go, for the reasons previously described.
> 

Got it, thanks!

> > > > >
> > > > > > +    return blksize_to_iounit(pdu, blksize);
> > > > > >  }
> > > > > >
> > > > > >  static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct
> > > > > > stat *stbuf, @@ -1332,7 +1371,11 @@ static int
> > > > > > stat_to_v9stat_dotl(V9fsPDU *pdu,
> > > > > const struct stat *stbuf,
> > > > > >      v9lstat->st_rdev = host_dev_to_dotl_dev(stbuf->st_rdev);
> > > > > >      v9lstat->st_size = stbuf->st_size;
> > > > > >      v9lstat->st_blksize = stat_to_iounit(pdu, stbuf);
> > > > > > +#ifndef CONFIG_WIN32
> > > > > >      v9lstat->st_blocks = stbuf->st_blocks;
> > > > > > +#else
> > > > > > +    v9lstat->st_blocks = 0;
> > > > > > +#endif
> > > > >
> > > > > Really?
> > > >
> > > > Windows struct stat does not have such field. See above link.
> > >
> > > Then it probably has to be calculated by file size / block size.
> >
> > Got it.
> >
> > >
> > > > >
> > > > > >      v9lstat->st_atime_sec = stbuf->st_atime;
> > > > > >      v9lstat->st_mtime_sec = stbuf->st_mtime;
> > > > > >      v9lstat->st_ctime_sec = stbuf->st_ctime; @@ -1340,7
> > > > > > +1383,8 @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu, const
> > > > > > struct stat
> > > *stbuf,
> > > > > >      v9lstat->st_atime_nsec = stbuf->st_atimespec.tv_nsec;
> > > > > >      v9lstat->st_mtime_nsec = stbuf->st_mtimespec.tv_nsec;
> > > > > >      v9lstat->st_ctime_nsec = stbuf->st_ctimespec.tv_nsec;
> > > > > > -#else
> > > > > > +#endif
> > > > > > +#ifdef CONFIG_LINUX
> > > > > >      v9lstat->st_atime_nsec = stbuf->st_atim.tv_nsec;
> > > > > >      v9lstat->st_mtime_nsec = stbuf->st_mtim.tv_nsec;
> > > > > >      v9lstat->st_ctime_nsec = stbuf->st_ctim.tv_nsec; @@
> > > > > > -2471,6
> > > > > > +2515,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU
> > > > > > +*pdu,
> > > > > V9fsFidState *fidp,
> > > > > >      struct dirent *dent;
> > > > > >      struct stat *st;
> > > > > >      struct V9fsDirEnt *entries = NULL;
> > > > > > +    unsigned char d_type = 0;
> > > > > >
> > > > > >      /*
> > > > > >       * inode remapping requires the device id, which in turn
> > > > > > might be @@ -2540,10 +2585,13 @@ static int coroutine_fn
> > > > > > v9fs_do_readdir(V9fsPDU
> > > > > *pdu, V9fsFidState *fidp,
> > > > > >          v9fs_string_init(&name);
> > > > > >          v9fs_string_sprintf(&name, "%s", dent->d_name);
> > > > > >
> > > > > > +#ifndef CONFIG_WIN32
> > > > > > +        d_type = dent->d_type; #endif
> > > > > >          /* 11 = 7 + 4 (7 = start offset, 4 = space for
> > > > > > storing count)
> > > */
> > > > > >          len = pdu_marshal(pdu, 11 + count, "Qqbs",
> > > > > >                            &qid, off,
> > > > > > -                          dent->d_type, &name);
> > > > > > +                          d_type, &name);
> > > > >
> > > > > Are you saying that d_type is not initialized with zero already?
> > > >
> > > > struct dirent is defined by MinGW, it does not have d_type member:
> > > >
> > > > https://github.com/Alexpux/mingw-w64/blob/master/mingw-w64-headers
> > > > /crt
> > > > /dirent.h
> > >
> > > My bad, I misread your code. That's fine.
> > >
> > > >
> > > > >
> > > > > >          v9fs_string_free(&name);
> > > > > >
> > > > > > @@ -2873,8 +2921,12 @@ static void coroutine_fn
> > > > > > v9fs_create(void
> > > *opaque)
> > > > > >          }
> > > > > >
> > > > > >          nmode |= perm & 0777;
> > > > > > +#ifndef CONFIG_WIN32
> > > > > >          err = v9fs_co_mknod(pdu, fidp, &name, fidp->uid, -1,
> > > > > >                              makedev(major, minor), nmode,
> > > > > > &stbuf);
> > > > > > +#else
> > > > > > +        err = -ENOTSUP;
> > > > > > +#endif
> > > > > >          if (err < 0) {
> > > > > >              goto out;
> > > > > >          }
> > > > > > @@ -2899,8 +2951,12 @@ static void coroutine_fn
> > > > > > v9fs_create(void
> > > *opaque)
> > > > > >          v9fs_path_copy(&fidp->path, &path);
> > > > > >          v9fs_path_unlock(s);
> > > > > >      } else if (perm & P9_STAT_MODE_SOCKET) {
> > > > > > +#ifndef CONFIG_WIN32
> > > > > >          err = v9fs_co_mknod(pdu, fidp, &name, fidp->uid, -1,
> > > > > >                              0, S_IFSOCK | (perm & 0777),
> > > > > > &stbuf);
> > > > > > +#else
> > > > > > +        err = -ENOTSUP;
> > > > > > +#endif
> > > > >
> > > > > As with previous patches, I would consider making the user aware
> > > > > to use
> > > > > mapped(-xattr) with something like error_report_once().
> > > >
> > > > OK, got it.
> > > >
> > > > >
> > > > > >          if (err < 0) {
> > > > > >              goto out;
> > > > > >          }
> > > > > > @@ -3634,7 +3690,7 @@ out_nofid:
> > > > > >
> > > > > >  static void coroutine_fn v9fs_mknod(void *opaque)  {
> > > > > > -
> > > > > > +#ifndef CONFIG_WIN32
> > > > > >      int mode;
> > > > > >      gid_t gid;
> > > > > >      int32_t fid;
> > > > > > @@ -3691,6 +3747,10 @@ out:
> > > > > >  out_nofid:
> > > > > >      pdu_complete(pdu, err);
> > > > > >      v9fs_string_free(&name);
> > > > > > +#else
> > > > > > +    V9fsPDU *pdu = opaque;
> > > > > > +    pdu_complete(pdu, -1);
> > > > > > +#endif
> > > > > >  }
> > > > > >
> > > > > >  /*
> > > > > > @@ -3963,7 +4023,7 @@ out_nofid:
> > > > > >  #if defined(CONFIG_LINUX)
> > > > > >  /* Currently, only Linux has XATTR_SIZE_MAX */  #define
> > > > > > P9_XATTR_SIZE_MAX XATTR_SIZE_MAX -#elif defined(CONFIG_DARWIN)
> > > > > > +#elif defined(CONFIG_DARWIN) || defined(CONFIG_WIN32)
> > > > > >  /*
> > > > > >   * Darwin doesn't seem to define a maximum xattr size in its user
> > > > > >   * space header, so manually configure it across platforms as 64k.
> > > > > > @@ -3980,6 +4040,7 @@ out_nofid:
> > > > > >
> > > > > >  static void coroutine_fn v9fs_xattrcreate(void *opaque)  {
> > > > > > +#ifndef CONFIG_WIN32
> > > > > >      int flags, rflags = 0;
> > > > > >      int32_t fid;
> > > > > >      uint64_t size;
> > > > > > @@ -4041,10 +4102,15 @@ out_put_fid:
> > > > > >  out_nofid:
> > > > > >      pdu_complete(pdu, err);
> > > > > >      v9fs_string_free(&name);
> > > > > > +#else
> > > > > > +    V9fsPDU *pdu = opaque;
> > > > > > +    pdu_complete(pdu, -1);
> > > > > > +#endif
> > > > > >  }
> > > > > >
> > > > > >  static void coroutine_fn v9fs_readlink(void *opaque)  {
> > > > > > +#ifndef CONFIG_WIN32
> > > > > >      V9fsPDU *pdu = opaque;
> > > > > >      size_t offset = 7;
> > > > > >      V9fsString target;
> > > > > > @@ -4080,6 +4146,10 @@ out:
> > > > > >      put_fid(pdu, fidp);
> > > > > >  out_nofid:
> > > > > >      pdu_complete(pdu, err);
> > > > > > +#else
> > > > > > +    V9fsPDU *pdu = opaque;
> > > > > > +    pdu_complete(pdu, -1);
> > > > > > +#endif
> > > > >
> > > > > Unnecessary double declaration of pdu.
> > > > >
> > > >
> > > > OK, got it.
> > > >
> > > > > >  }
> > > > > >
> > > > > >  static CoroutineEntry *pdu_co_handlers[] = { @@ -4341,6
> > > > > > +4411,7 @@ void v9fs_reset(V9fsState *s)
> > > > > >
> > > > > >  static void __attribute__((__constructor__))
> > > > > > v9fs_set_fd_limit(void) {
> > > > > > +#ifndef CONFIG_WIN32
> > > > > >      struct rlimit rlim;
> > > > > >      if (getrlimit(RLIMIT_NOFILE, &rlim) < 0) {
> > > > > >          error_report("Failed to get the resource limit"); @@
> > > > > > -4348,4
> > > > > > +4419,5 @@ static void __attribute__((__constructor__))
> > > > > v9fs_set_fd_limit(void)
> > > > > >      }
> > > > > >      open_fd_hw = rlim.rlim_cur - MIN(400, rlim.rlim_cur / 3);
> > > > > >      open_fd_rc = rlim.rlim_cur / 2;
> > > > > > +#endif
> > > > >
> > > > > Really?
> > > >
> > > > Windows does not provide getrlimit()
> > >
> > > But it has _getmaxstdio() and _setmaxstdio().
> > >
> > > And even if Windows had no replacement functions, you need to
> > > initialize these two global variables with some meaningful value.
> > > Otherwise they would be zero, and that would mean 9p server would
> > > assume max. 0 files could be open at the same time, so it would
> > > constantly close and re-open every single file descriptor on every minor
> micro-transaction for no reason.
> >
> > I could implement this function by _getmaxstdio() and _setmaxstdio().
> > But these two functions are used for struct FILE, but not file descriptor
> (FD).
> > I wrote a basic test for this function:
> >   _getmaxstdio() returns 512, but I can use "open" to open at least 600
> files without error.
> >
> > Windows does not provide any document about the limititaion.
> >
> > I think we can put a hard code here: 512, 1024, 2048, etc.
> 
> Yeah, that's pretty badly documented, I agree. The fact that it's prefixed
> with an underscore suggests that they were probably not too convinced about
> their API either. But it is as it is.
> 
> The fact that you can open a bit more files than what this function
> advertises is not a huge surprise or speaking against using it though.
> Because it is not untypical for systems to allow overallocation of system
> resources to a certain extent.
> 
> At least from what I can see right now, this function is commonly used on
> Windows to get exactly that metric. So I would rather use that than using
> hard coded values, because from what I can see on the web, the numbers people
> got experimentally are varying quite a bit.
> 

Got it.

Thanks
Guohuai

> Best regards,
> Christian Schoenebeck
>
diff mbox series

Patch

diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index 82b2d0c3e4..3d154e9103 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -53,8 +53,10 @@  static inline uint64_t makedev_dotl(uint32_t dev_major, uint32_t dev_minor)
  */
 static inline uint64_t host_dev_to_dotl_dev(dev_t dev)
 {
-#ifdef CONFIG_LINUX
+#if defined(CONFIG_LINUX)
     return dev;
+#elif defined(CONFIG_WIN32)
+    return 0;
 #else
     return makedev_dotl(major(dev), minor(dev));
 #endif
@@ -260,7 +262,9 @@  static inline struct dirent *qemu_dirent_dup(struct dirent *dent)
 #if defined CONFIG_DARWIN && defined CONFIG_PTHREAD_FCHDIR_NP
 int pthread_fchdir_np(int fd) __attribute__((weak_import));
 #endif
+#ifndef CONFIG_WIN32
 int qemu_mknodat(P9_FILE_ID dirfd, const char *filename, mode_t mode,
                  dev_t dev);
+#endif
 
 #endif
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 6c4af86240..771aab34ac 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -39,6 +39,11 @@ 
 #include "qemu/xxhash.h"
 #include <math.h>
 
+#ifdef CONFIG_WIN32
+#define UTIME_NOW   ((1l << 30) - 1l)
+#define UTIME_OMIT  ((1l << 30) - 2l)
+#endif
+
 int open_fd_hw;
 int total_open_fd;
 static int open_fd_rc;
@@ -132,13 +137,17 @@  static int dotl_to_open_flags(int flags)
     DotlOpenflagMap dotl_oflag_map[] = {
         { P9_DOTL_CREATE, O_CREAT },
         { P9_DOTL_EXCL, O_EXCL },
+#ifndef CONFIG_WIN32
         { P9_DOTL_NOCTTY , O_NOCTTY },
+#endif
         { P9_DOTL_TRUNC, O_TRUNC },
         { P9_DOTL_APPEND, O_APPEND },
+#ifndef CONFIG_WIN32
         { P9_DOTL_NONBLOCK, O_NONBLOCK } ,
         { P9_DOTL_DSYNC, O_DSYNC },
         { P9_DOTL_FASYNC, FASYNC },
-#ifndef CONFIG_DARWIN
+#endif
+#ifdef CONFIG_LINUX
         { P9_DOTL_NOATIME, O_NOATIME },
         /*
          *  On Darwin, we could map to F_NOCACHE, which is
@@ -151,8 +160,10 @@  static int dotl_to_open_flags(int flags)
 #endif
         { P9_DOTL_LARGEFILE, O_LARGEFILE },
         { P9_DOTL_DIRECTORY, O_DIRECTORY },
+#ifndef CONFIG_WIN32
         { P9_DOTL_NOFOLLOW, O_NOFOLLOW },
         { P9_DOTL_SYNC, O_SYNC },
+#endif
     };
 
     for (i = 0; i < ARRAY_SIZE(dotl_oflag_map); i++) {
@@ -179,8 +190,11 @@  static int get_dotl_openflags(V9fsState *s, int oflags)
      * Filter the client open flags
      */
     flags = dotl_to_open_flags(oflags);
-    flags &= ~(O_NOCTTY | O_ASYNC | O_CREAT);
-#ifndef CONFIG_DARWIN
+    flags &= ~(O_CREAT);
+#ifndef CONFIG_WIN32
+    flags &= ~(O_NOCTTY | O_ASYNC);
+#endif
+#ifdef CONFIG_LINUX
     /*
      * Ignore direct disk access hint until the server supports it.
      */
@@ -986,9 +1000,11 @@  static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
     if (S_ISDIR(stbuf->st_mode)) {
         qidp->type |= P9_QID_TYPE_DIR;
     }
+#ifndef CONFIG_WIN32
     if (S_ISLNK(stbuf->st_mode)) {
         qidp->type |= P9_QID_TYPE_SYMLINK;
     }
+#endif
 
     return 0;
 }
@@ -1097,6 +1113,7 @@  static mode_t v9mode_to_mode(uint32_t mode, V9fsString *extension)
         ret |= S_IFDIR;
     }
 
+#ifndef CONFIG_WIN32
     if (mode & P9_STAT_MODE_SYMLINK) {
         ret |= S_IFLNK;
     }
@@ -1106,6 +1123,7 @@  static mode_t v9mode_to_mode(uint32_t mode, V9fsString *extension)
     if (mode & P9_STAT_MODE_NAMED_PIPE) {
         ret |= S_IFIFO;
     }
+#endif
     if (mode & P9_STAT_MODE_DEVICE) {
         if (extension->size && extension->data[0] == 'c') {
             ret |= S_IFCHR;
@@ -1118,6 +1136,7 @@  static mode_t v9mode_to_mode(uint32_t mode, V9fsString *extension)
         ret |= S_IFREG;
     }
 
+#ifndef CONFIG_WIN32
     if (mode & P9_STAT_MODE_SETUID) {
         ret |= S_ISUID;
     }
@@ -1127,6 +1146,7 @@  static mode_t v9mode_to_mode(uint32_t mode, V9fsString *extension)
     if (mode & P9_STAT_MODE_SETVTX) {
         ret |= S_ISVTX;
     }
+#endif
 
     return ret;
 }
@@ -1182,6 +1202,7 @@  static uint32_t stat_to_v9mode(const struct stat *stbuf)
         mode |= P9_STAT_MODE_DIR;
     }
 
+#ifndef CONFIG_WIN32
     if (S_ISLNK(stbuf->st_mode)) {
         mode |= P9_STAT_MODE_SYMLINK;
     }
@@ -1193,11 +1214,13 @@  static uint32_t stat_to_v9mode(const struct stat *stbuf)
     if (S_ISFIFO(stbuf->st_mode)) {
         mode |= P9_STAT_MODE_NAMED_PIPE;
     }
+#endif
 
     if (S_ISBLK(stbuf->st_mode) || S_ISCHR(stbuf->st_mode)) {
         mode |= P9_STAT_MODE_DEVICE;
     }
 
+#ifndef CONFIG_WIN32
     if (stbuf->st_mode & S_ISUID) {
         mode |= P9_STAT_MODE_SETUID;
     }
@@ -1209,6 +1232,7 @@  static uint32_t stat_to_v9mode(const struct stat *stbuf)
     if (stbuf->st_mode & S_ISVTX) {
         mode |= P9_STAT_MODE_SETVTX;
     }
+#endif
 
     return mode;
 }
@@ -1247,9 +1271,17 @@  static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, V9fsPath *path,
             return err;
         }
     } else if (v9stat->mode & P9_STAT_MODE_DEVICE) {
+        unsigned maj, min;
+
+#ifndef CONFIG_WIN32
+        maj = major(stbuf->st_rdev);
+        min = minor(stbuf->st_rdev);
+#else
+        maj = min = 0;
+#endif
         v9fs_string_sprintf(&v9stat->extension, "%c %u %u",
                 S_ISCHR(stbuf->st_mode) ? 'c' : 'b',
-                major(stbuf->st_rdev), minor(stbuf->st_rdev));
+                maj, min);
     } else if (S_ISDIR(stbuf->st_mode) || S_ISREG(stbuf->st_mode)) {
         v9fs_string_sprintf(&v9stat->extension, "%s %lu",
                 "HARDLINKCOUNT", (unsigned long)stbuf->st_nlink);
@@ -1317,7 +1349,14 @@  static int32_t blksize_to_iounit(const V9fsPDU *pdu, int32_t blksize)
 
 static int32_t stat_to_iounit(const V9fsPDU *pdu, const struct stat *stbuf)
 {
-    return blksize_to_iounit(pdu, stbuf->st_blksize);
+    int32_t blksize;
+
+#ifndef CONFIG_WIN32
+    blksize = stbuf->st_blksize);
+#else
+    blksize = 0;
+#endif
+    return blksize_to_iounit(pdu, blksize);
 }
 
 static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
@@ -1332,7 +1371,11 @@  static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
     v9lstat->st_rdev = host_dev_to_dotl_dev(stbuf->st_rdev);
     v9lstat->st_size = stbuf->st_size;
     v9lstat->st_blksize = stat_to_iounit(pdu, stbuf);
+#ifndef CONFIG_WIN32
     v9lstat->st_blocks = stbuf->st_blocks;
+#else
+    v9lstat->st_blocks = 0;
+#endif
     v9lstat->st_atime_sec = stbuf->st_atime;
     v9lstat->st_mtime_sec = stbuf->st_mtime;
     v9lstat->st_ctime_sec = stbuf->st_ctime;
@@ -1340,7 +1383,8 @@  static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
     v9lstat->st_atime_nsec = stbuf->st_atimespec.tv_nsec;
     v9lstat->st_mtime_nsec = stbuf->st_mtimespec.tv_nsec;
     v9lstat->st_ctime_nsec = stbuf->st_ctimespec.tv_nsec;
-#else
+#endif
+#ifdef CONFIG_LINUX
     v9lstat->st_atime_nsec = stbuf->st_atim.tv_nsec;
     v9lstat->st_mtime_nsec = stbuf->st_mtim.tv_nsec;
     v9lstat->st_ctime_nsec = stbuf->st_ctim.tv_nsec;
@@ -2471,6 +2515,7 @@  static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
     struct dirent *dent;
     struct stat *st;
     struct V9fsDirEnt *entries = NULL;
+    unsigned char d_type = 0;
 
     /*
      * inode remapping requires the device id, which in turn might be
@@ -2540,10 +2585,13 @@  static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
         v9fs_string_init(&name);
         v9fs_string_sprintf(&name, "%s", dent->d_name);
 
+#ifndef CONFIG_WIN32
+        d_type = dent->d_type;
+#endif
         /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
         len = pdu_marshal(pdu, 11 + count, "Qqbs",
                           &qid, off,
-                          dent->d_type, &name);
+                          d_type, &name);
 
         v9fs_string_free(&name);
 
@@ -2873,8 +2921,12 @@  static void coroutine_fn v9fs_create(void *opaque)
         }
 
         nmode |= perm & 0777;
+#ifndef CONFIG_WIN32
         err = v9fs_co_mknod(pdu, fidp, &name, fidp->uid, -1,
                             makedev(major, minor), nmode, &stbuf);
+#else
+        err = -ENOTSUP;
+#endif
         if (err < 0) {
             goto out;
         }
@@ -2899,8 +2951,12 @@  static void coroutine_fn v9fs_create(void *opaque)
         v9fs_path_copy(&fidp->path, &path);
         v9fs_path_unlock(s);
     } else if (perm & P9_STAT_MODE_SOCKET) {
+#ifndef CONFIG_WIN32
         err = v9fs_co_mknod(pdu, fidp, &name, fidp->uid, -1,
                             0, S_IFSOCK | (perm & 0777), &stbuf);
+#else
+        err = -ENOTSUP;
+#endif
         if (err < 0) {
             goto out;
         }
@@ -3634,7 +3690,7 @@  out_nofid:
 
 static void coroutine_fn v9fs_mknod(void *opaque)
 {
-
+#ifndef CONFIG_WIN32
     int mode;
     gid_t gid;
     int32_t fid;
@@ -3691,6 +3747,10 @@  out:
 out_nofid:
     pdu_complete(pdu, err);
     v9fs_string_free(&name);
+#else
+    V9fsPDU *pdu = opaque;
+    pdu_complete(pdu, -1);
+#endif
 }
 
 /*
@@ -3963,7 +4023,7 @@  out_nofid:
 #if defined(CONFIG_LINUX)
 /* Currently, only Linux has XATTR_SIZE_MAX */
 #define P9_XATTR_SIZE_MAX XATTR_SIZE_MAX
-#elif defined(CONFIG_DARWIN)
+#elif defined(CONFIG_DARWIN) || defined(CONFIG_WIN32)
 /*
  * Darwin doesn't seem to define a maximum xattr size in its user
  * space header, so manually configure it across platforms as 64k.
@@ -3980,6 +4040,7 @@  out_nofid:
 
 static void coroutine_fn v9fs_xattrcreate(void *opaque)
 {
+#ifndef CONFIG_WIN32
     int flags, rflags = 0;
     int32_t fid;
     uint64_t size;
@@ -4041,10 +4102,15 @@  out_put_fid:
 out_nofid:
     pdu_complete(pdu, err);
     v9fs_string_free(&name);
+#else
+    V9fsPDU *pdu = opaque;
+    pdu_complete(pdu, -1);
+#endif
 }
 
 static void coroutine_fn v9fs_readlink(void *opaque)
 {
+#ifndef CONFIG_WIN32
     V9fsPDU *pdu = opaque;
     size_t offset = 7;
     V9fsString target;
@@ -4080,6 +4146,10 @@  out:
     put_fid(pdu, fidp);
 out_nofid:
     pdu_complete(pdu, err);
+#else
+    V9fsPDU *pdu = opaque;
+    pdu_complete(pdu, -1);
+#endif
 }
 
 static CoroutineEntry *pdu_co_handlers[] = {
@@ -4341,6 +4411,7 @@  void v9fs_reset(V9fsState *s)
 
 static void __attribute__((__constructor__)) v9fs_set_fd_limit(void)
 {
+#ifndef CONFIG_WIN32
     struct rlimit rlim;
     if (getrlimit(RLIMIT_NOFILE, &rlim) < 0) {
         error_report("Failed to get the resource limit");
@@ -4348,4 +4419,5 @@  static void __attribute__((__constructor__)) v9fs_set_fd_limit(void)
     }
     open_fd_hw = rlim.rlim_cur - MIN(400, rlim.rlim_cur / 3);
     open_fd_rc = rlim.rlim_cur / 2;
+#endif
 }