RE: [PATCH v3 07/17] hw/9pfs: Support getting current directory offset for Windows

[Shi, Guohuai]

> -----Original Message-----
> From: Christian Schoenebeck <qemu_oss@crudebyte.com>
> Sent: Wednesday, December 21, 2022 22:48
> To: Greg Kurz <groug@kaod.org>; qemu-devel@nongnu.org
> Cc: Shi, Guohuai <Guohuai.Shi@windriver.com>; Meng, Bin
> <Bin.Meng@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.shi@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.shi@windriver.com>
> > Signed-off-by: Bin Meng <bin.meng@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.

> 
> > -#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.

Thanks
Guohuai

> 
> > 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 */
> >
> 
>