RE: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for Windows

[Shi, Guohuai]

> -----Original Message-----
> From: Christian Schoenebeck <qemu_oss@crudebyte.com>
> Sent: 2022年5月5日 19:44
> To: qemu-devel@nongnu.org; Shi, Guohuai <Guohuai.Shi@windriver.com>; Greg Kurz
> <groug@kaod.org>
> Cc: Meng, Bin <Bin.Meng@windriver.com>; Bin Meng <bmeng.cn@gmail.com>
> Subject: Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for
> Windows
> 
> [Please note: This e-mail is from an EXTERNAL e-mail address]
> 
> On Mittwoch, 4. Mai 2022 21:34:22 CEST Shi, Guohuai wrote:
> [...]
> > > > index 0000000000..aab7c9f7b5
> > > > --- /dev/null
> > > > +++ b/hw/9pfs/9p-local-win32.c
> > > > @@ -0,0 +1,1242 @@
> > > > +/*
> > > > + * 9p Windows callback
> > > > + *
> > > > + * Copyright (c) 2022 Wind River Systems, Inc.
> > > > + *
> > > > + * Based on hw/9pfs/9p-local.c
> > > > + *
> > > > + * This work is licensed under the terms of the GNU GPL, version 2.
> > > > See
> > > > + * the COPYING file in the top-level directory.
> > > > + */
> > > > +
> > > > +/*
> > > > + * Not so fast! You might want to read the 9p developer docs first:
> > > > + * https://wiki.qemu.org/Documentation/9p
> > > > + */
> > > > +
> > > > +#include "qemu/osdep.h"
> > > > +#include <windows.h>
> > > > +#include <dirent.h>
> > > > +#include "9p.h"
> > > > +#include "9p-local.h"
> > > > +#include "9p-xattr.h"
> > > > +#include "9p-util.h"
> > > > +#include "fsdev/qemu-fsdev.h"   /* local_ops */
> > > > +#include "qapi/error.h"
> > > > +#include "qemu/cutils.h"
> > > > +#include "qemu/error-report.h"
> > > > +#include "qemu/option.h"
> > > > +#include <libgen.h>
> > >
> > > I'm not sure whether all of this (i.e. 9p-local-win32.c in general)
> > > is really needed. I mean yes, it probably does the job, but you
> > > basically add a complete separate 'local' backend implementation
> > > just for the Windows host platform.
> > >
> > > My expectation would be to stick to 9p-local.c being OS-agnostic,
> > > and only define OS-specific functionality in 9p-util-windows.c if needed.
> > >
> > > And most importantly: don't duplicate code as far as possible! I
> > > mean somebody would need to also review and maintain all of this.
> >
> > Actually, almost all FileOperations functions are re-written for
> > Windows host.
> >
> > Here is the comparison statistics for 9p-local.c and 9p-local-win32.c:
> > Total lines : 1611
> > Changed lines: 590
> > Inserted lines: 138
> > Removed lines: 414
> >
> > For function level difference count:
> >
> > Changed function: 39
> > Unchanged function: 13
> >
> > If use "#ifdef _WIN32" to sperate Windows host code, it may need to be
> > inserted about 150 code blocks (or 39 code blocks for all changed
> > functions).
> >
> > I am not sure if it is a good idea to insert so many "#ifdef _WIN32",
> > it may cause this file not readable.
> >
> > If stick to 9p-local.c being OS-agnostic, I think it is better to
> > create two new files: 9p-local-linux.c and 9p-local-win32.c
> 
> The thing is, as this is presented right now, I can hardly even see where 
> deviating
> behaviour for Windows would be, where not, and I'm missing any explanations 
> and
> reasons for the individual deviations right now. Chances are that you are
> unnecessarilly adding duplicate code and unnecessary code deviations. For me 
> this
> 9p-local-win32.c approach looks overly complex and not appropriately 
> abstracted
> on first sight.
> 
> I suggest waiting for Greg to give his opinion on this as well before 
> continuing.
> 
> Best regards,
> Christian Schoenebeck
> 

I can make this commit to be split into several commits:
Commit #1: only make Windows host built successfully.
Commit #2: provide some basic file system operations and leave other functions 
not workable (return -1).
Commit #3: provide full file system operations.
Commit #4: provide security mode "mapped" by NTFS ADS
Commit #5: provide security mode "mapped-file"
Commit #6: fix Windows (MinGW) directory access API compatible issues

However, I think I may still need a standalone file "9p-local-win32.c". Some 
functions could be moved (or merged) back to 9p-local.c. But there are still 
some functions are too complex to be merged.

Thanks
Guohuai