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

[RafaÅ WojdyÅa <omeg@xxxxxxxxxxxxxxxxxxxxxx>]

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?

>> +#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