[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH 09/16] hw/9pfs: Disable unsupported flags and features for Wi
|
From: |
Shi, Guohuai |
|
Subject: |
RE: [PATCH 09/16] hw/9pfs: Disable unsupported flags and features for Windows |
|
Date: |
Wed, 2 Nov 2022 03:44:14 +0000 |
> -----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
>