Re: [PATCH 2/3] server: Added server-side raw input function implementations (try 4)

[Vitaliy Margolen]

On 06/20/2011 02:55 AM, Vincas Miliūnas wrote:
> (see the message in the first part of the patch)
> +struct raw_input
> +{
> - struct list    entry;
> - unsigned short usage_page;
> - unsigned short usage;
> - BOOL           processed;
> - BOOL           retrieved;
> - RAWINPUT       raw;
> -
> - /* common fields */
> - unsigned int   type;
> - unsigned int   message;
> - unsigned int   flags;
> - unsigned int   extra_info;
> -
> - /* mouse fields */
> - unsigned int   point_x;
> - unsigned int   point_y;
> - unsigned int   mouse_data;
> - unsigned int   cursor_x;
> - unsigned int   cursor_y;
> -
> - /* keyboard fields */
> - unsigned int   vk_code;
> - unsigned int   scan_code;
>
> +};

Mouse and keyboard fields should be a union to save space.
> +#define NUM_RAW_DEVICES (sizeof( raw_devices ) / sizeof( struct raw_device ))

Please use "sizeof(array)/sizeof(array[0])" construct instead.
> - UINT i, size_in_bytes;

In server please use standard C types ("unsigned int" in this case).
> -
>    /* Currently fake handles are provided, however they are only used for the GetRawInputDeviceInfo function,
> -
>       thus it should not create any undesirable side effects */
> -
>    result[i].hDevice = (HANDLE)raw_devices[i].handle;

This doesn't sound right. What this handle can be used for?
> - if (!(raw = malloc( sizeof( *raw ) )))

In server please use mem_alloc() instead.
> +/* Unregister a raw input device */
> +DECL_HANDLER(unregister_raw_input_device)

I don't see handing of case where device is not found. Should you return 
error instead?
> - LIST_FOR_EACH( registration_entry, &current->process->raw_registered )
> - {
> -
>    struct raw_registration *registration = LIST_ENTRY( registration_entry, struct raw_registration, entry );

You can use this instead:
LIST_FOR_EACH_ENTRY( registration, &current->process->raw_registered, 
struct raw_registration, entry )
> - /* Prevent unprocessed raw input entries from being queued indefinitely */
> - while (current->process->raw_inputs_len >= MAX_RAW_INPUT_QUEUE_LENGTH)
> - {
> -
>    struct raw_input *head = LIST_ENTRY( list_head( &current->process->raw_inputs ), struct raw_input, entry );
> -
>    list_remove( &head->entry );
> -
>    current->process->raw_inputs_len -= 1;
> -
>    free( head );
> - }

Looks like you need a circular buffer (an array of fixed size with head and 
tail pointers) instead of a list here.

Vitaliy.

[PATCH 2/3] server: Added server-side raw input function implementations (try 4)

[Vincas Miliūnas]

On 06/20/2011 05:24 PM, Vitaliy Margolen wrote:
> On 06/20/2011 02:55 AM, Vincas Miliūnas wrote:
> > (see the message in the first part of the patch)
> > +struct raw_input
> > +{
> > - struct list    entry;
> > - unsigned short usage_page;
> > - unsigned short usage;
> > - BOOL           processed;
> > - BOOL           retrieved;
> > - RAWINPUT       raw;
> > -
> > - /* common fields */
> > - unsigned int   type;
> > - unsigned int   message;
> > - unsigned int   flags;
> > - unsigned int   extra_info;
> > -
> > - /* mouse fields */
> > - unsigned int   point_x;
> > - unsigned int   point_y;
> > - unsigned int   mouse_data;
> > - unsigned int   cursor_x;
> > - unsigned int   cursor_y;
> > -
> > - /* keyboard fields */
> > - unsigned int   vk_code;
> > - unsigned int   scan_code;
> >
> > +};
>
> Mouse and keyboard fields should be a union to save space.
> > +#define NUM_RAW_DEVICES (sizeof( raw_devices ) / sizeof( struct raw_device ))
>
> Please use "sizeof(array)/sizeof(array[0])" construct instead.
> > - UINT i, size_in_bytes;
>
> In server please use standard C types ("unsigned int" in this case).
> > -
> >    /* Currently fake handles are provided, however they are only used for the GetRawInputDeviceInfo function,
> > -
> >       thus it should not create any undesirable side effects */
> > -
> >    result[i].hDevice = (HANDLE)raw_devices[i].handle;
>
> This doesn't sound right. What this handle can be used for?
> > - if (!(raw = malloc( sizeof( *raw ) )))
>
> In server please use mem_alloc() instead.
> > +/* Unregister a raw input device */
> > +DECL_HANDLER(unregister_raw_input_device)
>
> I don't see handing of case where device is not found. Should you return 
> error instead?
> > - LIST_FOR_EACH( registration_entry, &current->process->raw_registered )
> > - {
> > -
> >    struct raw_registration *registration = LIST_ENTRY( registration_entry, struct raw_registration, entry );
>
> You can use this instead:
> LIST_FOR_EACH_ENTRY( registration, &current->process->raw_registered, 
> struct raw_registration, entry )
> > - /* Prevent unprocessed raw input entries from being queued indefinitely */
> > - while (current->process->raw_inputs_len >= MAX_RAW_INPUT_QUEUE_LENGTH)
> > - {
> > -
> >    struct raw_input *head = LIST_ENTRY( list_head( &current->process->raw_inputs ), struct raw_input, entry );
> > -
> >    list_remove( &head->entry );
> > -
> >    current->process->raw_inputs_len -= 1;
> > -
> >    free( head );
> > - }
>
> Looks like you need a circular buffer (an array of fixed size with head and 
> tail pointers) instead of a list here.
>
> Vitaliy.

I've updated the patch and replied -
http://www.winehq.org/pipermail/wine-patches/2011-June/103346.html