Re: [PATCH 1/2] xen/kbdif: update protocol documentation

[Oleksandr Andrushchenko <andr2000@gmail.com>]

On 01/11/2017 07:35 PM, Dario Faggioli wrote:
> On Tue, 2017-01-10 at 09:21 +0200, Oleksandr Andrushchenko wrote:
>> On 01/07/2017 12:20 AM, Stefano Stabellini wrote:
>>> On Fri, 6 Jan 2017, Oleksandr Andrushchenko wrote:
>>>> |                               reserved
>>>>         |
>>>> + * +-----------------+-----------------+-----------------+----
>>>> -------------+
>>>> + *
>>>> |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\
>>>> /\/\/\/|
>>>> + * +-----------------+-----------------+-----------------+----
>>>> -------------+
>>> I guess this means that we are skipping some bytes because they are
>>> all
>>> reserved, right?  If so, it would be useful to write the byte count
>>> at this
>>> point. What's the total size of the event struct?
>>>
>> IMO, we shouldn't put any sizes here because:
>> 1. Above we say "All event packets have the same
>>      length (40 octets)"
>> 2. All the event structures are part of the
>> union xenkbd_in_event, which has
>> char pad[XENKBD_IN_EVENT_SIZE];
>> which effectively regulates the size of the event.
>>
> In which case, you can use either 40 or XENKBD_IN_EVENT_SIZE (probably
> the latter).
>
> It's indeed a repetition, but a good one, IMO: it helps the reader, as
> she won't have to go back to figure out how big the struct was, how the
> macro was call and to what value it was defined).
I am still not convinced that we should put it.
Probably we can go the way other RFCs do, like TCP [1], 802.11 [2] etc:
those do not define any reserved fields at the bottom of structures,
(which are effectively padding?) but are limited to only those fields
which are defined. This means that, for example, instead of

  *          0                 1                  2 3        octet
  * 
+-----------------+-----------------+-----------------+-----------------+
  * |   _TYPE_MOTION  | reserved                      |
  * 
+-----------------+-----------------+-----------------+-----------------+
  * | rel_x                                 |
  * 
+-----------------+-----------------+-----------------+-----------------+
  * | rel_y                                 |
  * 
+-----------------+-----------------+-----------------+-----------------+
  * | rel_z                                 |
  * 
+-----------------+-----------------+-----------------+-----------------+
  * | reserved                                |
  * 
+-----------------+-----------------+-----------------+-----------------+
  * 
|/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
  * 
+-----------------+-----------------+-----------------+-----------------+
  * | reserved                                |
  * 
+-----------------+-----------------+-----------------+-----------------+

simply put

  *          0                 1                  2 3        octet
  * 
+-----------------+-----------------+-----------------+-----------------+
  * |   _TYPE_MOTION  | reserved                      |
  * 
+-----------------+-----------------+-----------------+-----------------+
  * | rel_x                                 |
  * 
+-----------------+-----------------+-----------------+-----------------+
  * | rel_y                                 |
  * 
+-----------------+-----------------+-----------------+-----------------+
  * | rel_z                                 |
  * 
+-----------------+-----------------+-----------------+-----------------+

What do you think?


[1] https://www.ietf.org/rfc/rfc793.txt
[2] https://tools.ietf.org/html/rfc5416
> Regards,
> Dario
Thank you,
Oleksandr

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel