RE: [PATCH 07/16] hw/9pfs: Implement Windows specific utilities functions for 9pfs

[Shi, Guohuai]

> -----Original Message-----
> From: Christian Schoenebeck <qemu_oss@crudebyte.com>
> Sent: Wednesday, November 2, 2022 19:06
> 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 07/16] hw/9pfs: Implement Windows specific utilities
> functions for 9pfs
> 
> 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:07:35 AM CET Shi, Guohuai wrote:
> >
> > > -----Original Message-----
> > > From: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > Sent: Wednesday, November 2, 2022 02:22
> > > 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 07/16] hw/9pfs: Implement Windows specific
> > > utilities functions for 9pfs
> > >
> > > [Please note: This e-mail is from an EXTERNAL e-mail address]
> > >
> > > On Tuesday, November 1, 2022 4:20:53 PM CET Shi, Guohuai wrote:
> > > >
> > > [...]
> > > > > > > Windows POSIX API and MinGW library do not provide the
> > > > > > > NO_FOLLOW flag, and do not allow opening a directory by
> > > > > > > POSIX open(). This causes all
> > > > > > > xxx_at() functions cannot work directly. However, we can
> > > > > > > provide Windows handle based functions to emulate xxx_at()
> functions (e.g.:
> > > > > > > openat_win32, utimensat_win32, etc.).
> > > > > > >
> > > > > > > Windows does not support extended attributes. 9pfs for
> > > > > > > Windows uses NTFS ADS (Alternate Data Streams) to emulate
> > > > > > > extended
> > > attributes.
> > > > > > >
> > > > > > > Windows does not provide POSIX compatible readlink(), and
> > > > > > > symbolic link feature in 9pfs will be disabled on Windows.
> > > > > >
> > > > > > Wouldn't it be more user friendly if the relevant error
> > > > > > locations would use something like error_report_once() and
> > > > > > suggesting to enable
> > > > > > mapped(-xattr) to make 9p symlinks on guest working if desired
> > > > > > by the
> > > user?
> > > > > >
> > > > > > Probably this error case would need to wrapped into a
> > > > > > dedicated function, otherwise I guess error_report_once()
> > > > > > would fire several times by different callers.
> > > > > >
> > > > >
> > > > > Windows (MinGW) does not only support symlink, but also does not
> > > > > have symlink definitions.
> > > > > Windows does not support symlink flags S_IFLNK.
> > > > >
> > > > > So even I add symlink support by mapped-xattr, the MinGW library
> > > > > does not have symlink flags and get a build error.
> > > > > And this flags is defined by Windows header files.
> > > > > The impact of adding a new flags to an pre-defined structure
> > > > > (struct
> > > > > stat) is unknown.
> > > > >
> > > > > So I think it is not a good idea to do that.
> > > >
> > > > Because Windows does not support symlink, so error_report_once()
> > > > and report
> > > it to user will be OK.
> > > > But mapped-xattr could not work.
> > >
> > > Showing an error makes sense for "passthrough" security model, but
> > > not for the "mapped" one.
> > >
> > > Just to avoid misapprehensions: are you aware that there is already
> > > a system- agnostic implementation for symlinks if "mapped" is used?
> > >
> > > When mapped security model is enabled, then creating symlinks on
> > > guest will simply create a corresponding *regular* file on host and
> > > the content of that regular file on host is the pointing path as raw
> > > string. Additionally the symlink flag is added to "virtfs.mode"
> > > xattr to mark that regular file as a symlink, a virtual one that is.
> > > So this does not require any support for symlinks by either the
> underlying host file system, nor by host OS.
> > >
> > > Likewise interpreting and walking those virtual symlinks in "mapped"
> > > mode is also implemented in the local fs driver already.
> >
> > Yes, symlink can be supported by "mapped" mode.
> > I mean that MinGW does not provide symlink mode flags "S_IFLNK" and some
> other related functions and defines.
> > You can check with "9p.c": S_ISLNK, S_ISSOCK and S_ISFIFO are not valid on
> Windows (MinGW) host.
> > So even I enabled symlink supported by "mapped" mode on local-agent code,
> "9p.c" can not be built.
> >
> > So I disabled symlink totally, because MinGW interface does not support it.
> >
> > To resolve this issue, MinGW should add the missing defines at first.
> 
> And what's wrong with something like the following?
> 
> #ifdef CONFIG_WIN32
> ...
> #ifndef S_ISLNK
> #define S_ISLNK(x) ...
> #endif
> ...
> #endif
> 

It is OK to add this just for current solution.
My concern is:
mode_t is a 16-bit value which store permission value in lower 12-bit and file 
type in higher 4-bit.
Windows does not document the other value for file type defines:

// from MS SDK header file:

#define _S_IFMT   0xF000 // File type mask
#define _S_IFDIR  0x4000 // Directory
#define _S_IFCHR  0x2000 // Character special
#define _S_IFIFO  0x1000 // Pipe
#define _S_IFREG  0x8000 // Regular

If we add a new type, is it a risk that the un-document value may be used by 
Windows internally and cause some compatible issue?
Or if Windows use this some values in the future cause conflict?

Thanks
Guohuai

> Best regards,
> Christian Schoenebeck
>