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

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Wednesday, November 2, 2022 12:51:23 PM CET Christian Schoenebeck wrote:
> On Wednesday, November 2, 2022 12:28:08 PM CET Shi, Guohuai wrote:
> > 
> [...]
> > > >
> > > > 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?
> 
> We don't have a crystal ball, but there are ways to handle this responsibly:
> At compile-time you could error out badly if there is anything we don't
> expect, like all of a sudden some of the macros we explicitly define for
> Windows are unexpectly there, and telling developers, hey somebody should look
> at this.
> 
> And at runtime you could add an assert() if some these values are all of a
> sudden filled. Because that's what we currently don't expect, as we are
> occupying them.

On a second thought, as far as the runtime aspect is concerned: killing QEMU
with assert() is probably overkill and unnecessarily too harsh. What can
happen if Windows starts to use the new bits for some purpose? As long as you
explicitly clear those bits out after retrieval from Windows, we can still use
them for our purposes. And instead you could simply log one less drastical
warning once to the user.

Does that make sense?

Best regards,
Christian Schoenebeck