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

[Christian Schoenebeck]

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