Re: [win-pv-devel] [PATCH 3/5] Define new IOCTLs for user-mode clients

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx [mailto:win-pv-devel-
> bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf Of Rafal Wojdyla
> Sent: 14 October 2015 14:07
> To: Paul Durrant; win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [win-pv-devel] [PATCH 3/5] Define new IOCTLs for user-mode
> clients
> 
> On 2015-10-14 10:47, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx [mailto:win-pv-devel-
> >> bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf Of Rafal Wojdyla
> >> Sent: 07 October 2015 05:49
> >> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> >> Subject: [win-pv-devel] [PATCH 3/5] Define new IOCTLs for user-mode
> >> clients
> >>
> >> IOCTL input is defined as XENIFACE_*_IN structs.
> >> IOCTL output is defined as XENIFACE_*_OUT structs.
> >>
> >> Signed-off-by: Rafal Wojdyla <omeg@xxxxxxxxxxxxxxxxxxxxxx>
> 
> I'll omit the code where there's no remarks or I generally agree with
> your comments.
> 
> >> +// Define only for user mode clients.
> >> +#ifndef XENIFACE_KERNEL_MODE
> >> +
> >
> > Could you not use _WINDLL to determine kernel-mode vs. user-mode
> inclusion, the same as the interface headers do?
> >
> I wanted to but it's not defined for EXEs. Couldn't find an alternative
> in predefined macros...
> 
> >> +typedef enum _XENBUS_STORE_PERMISSION_MASK {
> >> +    XENBUS_STORE_PERM_NONE = 0,
> >> +    XENBUS_STORE_PERM_READ = 1,
> >> +    XENBUS_STORE_PERM_WRITE = 2,
> >> +} XENBUS_STORE_PERMISSION_MASK;
> >> +
> >> +typedef struct _XENBUS_STORE_PERMISSION {
> >> +    USHORT Domain;
> >> +    XENBUS_STORE_PERMISSION_MASK Mask;
> >> +} XENBUS_STORE_PERMISSION, *PXENBUS_STORE_PERMISSION;
> >> +
> >> +#endif
> >
> > Shouldn't these be prefixed XENIFACE_ rather than XENBUS_?
> >
> These are defined in xenbus' store_interface.h. I don't like copying
> code like that but the data format is what both sides expect. I could
> use something else for user mode but that would be just inventing a new
> format for no real reason. Or maybe it would make sense for readability
> for someone writing a user mode client?
> 

I think it would be worth defining a separate enumeration. This is a new 
interface and should really be self-contained and consistently named.

  Paul

> >> +#pragma warning(push)
> >> +#pragma warning(disable:4200) // nonstandard extension used : zero-
> sized
> >> array in struct/union
> >
> > Could you not just specify an array length of one to avoid the need for this
> #pragma. That's generally how Windows APIs of this nature specify their
> structures.
> >
> I was used to using zero-sized arrays in cases like this because it made
> later size calculations cleaner. FIELD_OFFSET makes this a non issue
> though so I agree with your comment.
> 
> >> +typedef struct _XENIFACE_GNTTAB_PERMIT_FOREIGN_ACCESS_IN {
> >> +    ULONG RequestId; // should be unique for each request
> >
> > Is that unique globally, per-process or per-thread? Per-thread would be
> most convenient for the user, I guess.
> >
> It's per-process right now.
> 
> --
> RafaÅ WojdyÅa
> Qubes Tools for Windows developer
> https://www.qubes-os.org/
> 
> _______________________________________________
> win-pv-devel mailing list
> win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> http://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel
_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel