Re: [PATCH v2 05/19] hw/9pfs: Update 9pfs to use the new QemuFd_t type

[Bin Meng]

Hi Greg,

On Sat, Nov 19, 2022 at 6:20 PM Greg Kurz <groug@kaod.org> wrote:
>
> On Fri, 18 Nov 2022 14:38:00 +0100
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
>
> > On Friday, November 18, 2022 10:29:51 AM CET Greg Kurz wrote:
> > > On Fri, 11 Nov 2022 12:22:11 +0800
> > > Bin Meng <bin.meng@windriver.com> wrote:
> > >
> > > > With this new QemuFd_t type, it significantly reduces the number of
> > >
> > > I cannot find the definition of this type, nor the definition of
> > > qemu_fd_invalid(). Missing patch ?
> >
> > It's in patch 4. Looks like we were not CCed on that patch. :(
> >
>
> Oh I didn't check the numbering. I guess we were not CCed automatically...
>
> > 20221111042225.1115931-5-bin.meng@windriver.com/">https://lore.kernel.org/qemu-devel/20221111042225.1115931-5-bin.meng@windriver.com/
> >
>
> ... because this only touches include/qemu/osdep.h .
>
> Bin,
>
> Please ensure that the maintainers are in the Cc list for all
> patches in such a series, e.g. with explicit --cc arguments to
> git-send-email.

Sorry, I was only using the get maintainer script for each patch. Will
cc you explicitly next time.

>
> > > Anyway, IIUC this type is an int for linux and a HANDLE for windows,
> > > right ?
> > >
> > > According to win32 documentation at [1] :
> > >
> > > HANDLE
> > > A handle to an object.
> > >
> > > This type is declared in WinNT.h as follows:
> > >
> > > typedef PVOID HANDLE;
> > >
> > > and
> > >
> > > PVOID
> > > A pointer to any type.
> > >
> > > This type is declared in WinNT.h as follows:
> > >
> > > typedef void *PVOID;
> > >
> > > HANDLE is void *.
> > >
> > > From docs/devel/style.rst:
> > >
> > > Naming
> > > ======
> > >
> > > Variables are lower_case_with_underscores; easy to type and read.  
> > > Structured
> > > type names are in CamelCase; harder to type but standing out.  Enum type
> > > names and function type names should also be in CamelCase.  Scalar type
> > > names are lower_case_with_underscores_ending_with_a_t, like the POSIX
> > > uint64_t and family.  Note that this last convention contradicts POSIX
> > > and is therefore likely to be changed.
> > >
> > > Both int and void * are scalar types, so I'd rather name it qemu_fd_t,
> > > not using CamelCase at all so that it cannot be confused with a struct.
> > >
> > > [1] 
> > > https://learn.microsoft.com/en-us/windows/win32/winprog/windows-data-types
> >
> > Not that I had a strong opinion about this issue (as in general with coding
> > style topics). It was one of my suggested type names. To make this type 
> > long-
> > term proof I suggested to handle it as if it was a truly opaque type in 
> > QEMU:
> >
>
> A true opaque type in C is implemented with a structured type and pointers
> to this type.
>
> > https://lore.kernel.org/qemu-devel/4620086.XpUeK0iDWE@silver/
> >
> > That is to explicitly not try to do things like:
> >
> >     if (fd == -1)
> >
> > at least not hard wired in user code. According to QEMU code style you 
> > should
> > probably then drop the trailing "_t" though.
> >
>
> Yes, either one is fine I guess. Most important part is to provide
> a documented API to manipulate that type since, no matter the name,
> it is still a scalar type that can be manipulated as such.
>

This patch will be dropped in the next version, that means we still
use the posix fd for the 9p helpers on different platforms, but on
Windows it will be translated to Windows HANDLE internally in the
helper.

Regards,
Bin