On Wednesday, December 21, 2022 7:02:43 PM CET Shi, Guohuai wrote:
>
> > -----Original Message-----
> > From: Christian Schoenebeck <qemu_...@crudebyte.com>
> > Sent: Wednesday, December 21, 2022 22:48
> > To: Greg Kurz <gr...@kaod.org>; qemu-devel@nongnu.org
> > Cc: Shi, Guohuai <guohuai....@windriver.com>; Meng, Bin
> > <bin.m...@windriver.com>
> > Subject: Re: [PATCH v3 07/17] hw/9pfs: Support getting current directory
> > offset 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 Monday, December 19, 2022 11:20:11 AM CET Bin Meng wrote:
> > > From: Guohuai Shi <guohuai....@windriver.com>
> > >
> > > On Windows 'struct dirent' does not have current directory offset.
> > > Update qemu_dirent_off() to support Windows.
> > >
> > > While we are here, add a build time check to error out if a new host
> > > does not implement this helper.
> > >
> > > Signed-off-by: Guohuai Shi <guohuai....@windriver.com>
> > > Signed-off-by: Bin Meng <bin.m...@windriver.com>
> > > ---
> > >
> > > (no changes since v1)
> > >
> > > hw/9pfs/9p-util.h | 11 ++++++++---
> > > hw/9pfs/9p-util-win32.c | 7 +++++++
> > > hw/9pfs/9p.c | 4 ++--
> > > hw/9pfs/codir.c | 2 +-
> > > 4 files changed, 18 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h index
> > > 90420a7578..e395936b30 100644
> > > --- a/hw/9pfs/9p-util.h
> > > +++ b/hw/9pfs/9p-util.h
> > > @@ -127,6 +127,7 @@ int unlinkat_win32(int dirfd, const char
> > > *pathname, int flags); int statfs_win32(const char *root_path, struct
> > > statfs *stbuf); int openat_dir(int dirfd, const char *name); int
> > > openat_file(int dirfd, const char *name, int flags, mode_t mode);
> > > +off_t qemu_dirent_off_win32(void *s, void *fs);
> > > #endif
> > >
> > > static inline void close_preserve_errno(int fd) @@ -200,12 +201,16 @@
> > > ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
> > > * so ensure it is manually injected earlier and call here when
> > > * needed.
> > > */
> > > -static inline off_t qemu_dirent_off(struct dirent *dent)
> > > +static inline off_t qemu_dirent_off(struct dirent *dent, void *s,
> > > +void *fs)
> > > {
> >
> > Not sure why you chose void* here.
>
> It is "V9fsState *" here, but 9p-util.h may be included by some other source
> file which is not have definition of "V9fsState".
> So change it to "void *" to prevent unnecessary type declarations.
You can anonymously declare the struct to avoid cyclic dependencies (e.g. like
in 9p.h):
typedef struct V9fsState V9fsState;
Avoid the void.
> >
> > > -#ifdef CONFIG_DARWIN
> > > +#if defined(CONFIG_DARWIN)
> > > return dent->d_seekoff;
> > > -#else
> > > +#elif defined(CONFIG_LINUX)
> > > return dent->d_off;
> > > +#elif defined(CONFIG_WIN32)
> > > + return qemu_dirent_off_win32(s, fs); #else #error Missing
> > > +qemu_dirent_off() implementation for this host system
> > > #endif
> > > }
> > >
> > > diff --git a/hw/9pfs/9p-util-win32.c b/hw/9pfs/9p-util-win32.c index
> > > 7a270a7bd5..3592e057ce 100644
> > > --- a/hw/9pfs/9p-util-win32.c
> > > +++ b/hw/9pfs/9p-util-win32.c
> > > @@ -929,3 +929,10 @@ int qemu_mknodat(int dirfd, const char *filename,
> > mode_t mode, dev_t dev)
> > > errno = ENOTSUP;
> > > return -1;
> > > }
> > > +
> > > +off_t qemu_dirent_off_win32(void *s, void *fs) {
> > > + V9fsState *v9fs = s;
> > > +
> > > + return v9fs->ops->telldir(&v9fs->ctx, (V9fsFidOpenState *)fs); }
> >
> > That's a bit tricky. So far (i.e. for Linux host, macOS host) we are storing
> > the directory offset directly to the dirent struct. We are not using
> > telldir() as the stream might have mutated in the meantime, hence calling
> > telldir() might return a value that does no longer correlate to dirent in
> > question.
> >
> > Hence my idea was to use the same hack for Windows as we did for macOS,
> > where
> > we simply misuse a dirent field known to be not used, on macOS that's
> > d_seekoff which we are misusing for that purpose.
> >
> > Looking at the mingw dirent.h header though, there is not much we can choose
> > from. d_reclen is not used, but too short, d_ino is not used by mingw, but
> > currently we actually read this field in server common code to assemble a
> > file ID for guest. I mean it is always zero on Windows, so we could still
> > misuse it, but we would need to inject more hacks there. :/
>
> If you check seekdir and telldir() implement in MinGW,
> you can see that MinGW (and Windows) does not have a safety way to seek/tell
> directory offset.
> Because Windows POSIX and native API does not provide a way to seek directory.
> Windows native API only allow to read directory forward, but not backward.
> So even we store the d_seekoff to some places, the directory may still be
> modified.
>
> And Windows file system actually has inode number, MinGW does not introduce
> it to MinGW API.
> So I think it is not good way to use d_ino.
Scratch that d_ino hack. My original concern was that we might get concurrency
on the directory stream, however after a recap I stumbled over one of my
comments on this:
static int do_readdir_many(V9fsPDU *pdu, V9fsFidState *fidp,
struct V9fsDirEnt **entries, off_t offset,
int32_t maxsize, bool dostat)
{
...
/*
* TODO: Here should be a warn_report_once() if lock failed.
*
* With a good 9p client we should not get into concurrency here,
* because a good client would not use the same fid for concurrent
* requests. We do the lock here for safety reasons though. However
* the client would then suffer performance issues, so better log that
* issue here.
*/
v9fs_readdir_lock(&fidp->fs.dir);
...
}
So it boils down to whether or not we would want to care about broken 9p
clients on Windows host or not, and considering the huge amount of code to
deal with here for Windows host, we might say that we have more important
(real) problems to deal with than caring about a broken 9p client that does
not clone a FID before sending Treaddir (9p2000.L) or Tread (9p2000.u).
However looking at current MinGW implementation I start to think whether we
should use that API for directory retrieval at all, because for seekdir() they
are rewinding the directory stream to the very beginning on *each* call of
seekdir() and then readdir() in a while loop to the requested location for
which they use a simple, self-incremented consecutive number as stream
position:
https://github.com/Alexpux/mingw-w64/blob/master/mingw-w64-crt/misc/dirent.c#L319
Which can lead to two problems:
1. n-square performance issue (minor issue).
2. Inconsistent state if directory is modified in between (major issue), e.g.
the same directory appearing more than once in output, or directories not
appearing at all in output even though they were neither newly created nor
deleted. POSIX does not define what happens with deleted or newly created
directories in between, but it guarantees a consistent state.
What about calling _findfirst() and _findnext() directly, fetching all
directory entries at once, closing the stream, and 9p would just access that
snapshot instead? As long as the stream is not closed, Windows blocks all
directory changes, so we would have a consistent state.
> >
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 072cf67956..be247eeb30
> > > 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> > > @@ -2336,7 +2336,7 @@ static int coroutine_fn
> > v9fs_do_readdir_with_stat(V9fsPDU *pdu,
> > > count += len;
> > > v9fs_stat_free(&v9stat);
> > > v9fs_path_free(&path);
> > > - saved_dir_pos = qemu_dirent_off(dent);
> > > + saved_dir_pos = qemu_dirent_off(dent, pdu->s, &fidp->fs);
> > > }
> > >
> > > v9fs_readdir_unlock(&fidp->fs.dir);
> > > @@ -2537,7 +2537,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU
> > > *pdu,
> > V9fsFidState *fidp,
> > > qid.version = 0;
> > > }
> > >
> > > - off = qemu_dirent_off(dent);
> > > + off = qemu_dirent_off(dent, pdu->s, &fidp->fs);
> > > v9fs_string_init(&name);
> > > v9fs_string_sprintf(&name, "%s", dent->d_name);
> > >
> > > diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c index
> > > 93ba44fb75..d40515a607 100644
> > > --- a/hw/9pfs/codir.c
> > > +++ b/hw/9pfs/codir.c
> > > @@ -168,7 +168,7 @@ static int do_readdir_many(V9fsPDU *pdu, V9fsFidState
> > *fidp,
> > > }
> > >
> > > size += len;
> > > - saved_dir_pos = qemu_dirent_off(dent);
> > > + saved_dir_pos = qemu_dirent_off(dent, s, &fidp->fs);
> > > }
> > >
> > > /* restore (last) saved position */
> > >
> >
> >
>
>
>
