[Spice-devel] [PATCH spice-protocol 1/2] Add support for a header without sub list.

[Yonit Halperin]

On 12/29/2011 10:30 AM, Alon Levy wrote:
> On Thu, Dec 29, 2011 at 08:11:27AM +0200, Yonit Halperin wrote:
>> On 12/28/2011 10:04 PM, Yaniv Kaul wrote:
>>> On 12/28/2011 07:14 PM, Yonit Halperin wrote:
>>>> Add SpiceDataHeaderNoSub.
>>>> Introduce capability SPICE_COMMON_CAP_HEADER_NO_SUB.
>>>> Introduce SPICE_MSG_LIST: the msg body is SpiceSubMessageList.
>>>>
>>>> The advantage of using a header without sub list is to spare the 4
>>>> bytes that were sent
>>>> for a lot of messages without sublist.
>>>> Instead, messages that previously contained sub lists, will be split
>>>> to two msgs.
>>>> The first one will be SPICE_MSG_LIST, holding the sub list, and the
>>>> second will be the
>>>> main msg.
>>>> When most of the messages do not contain sub lists, the overhead of
>>>> the additional 10 bytes
>>>> for the header of SPICE_MSG_LIST is negligible.
>>>> ---
>>>> spice/enums.h | 1 +
>>>> spice/protocol.h | 9 ++++++++-
>>>> 2 files changed, 9 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/spice/enums.h b/spice/enums.h
>>>> index a587b00..0314f0b 100644
>>>> --- a/spice/enums.h
>>>> +++ b/spice/enums.h
>>>> @@ -344,6 +344,7 @@ enum {
>>>> SPICE_MSG_WAIT_FOR_CHANNELS,
>>>> SPICE_MSG_DISCONNECTING,
>>>> SPICE_MSG_NOTIFY,
>>>> + SPICE_MSG_LIST,
>>>> };
>>>>
>>>> enum {
>>>> diff --git a/spice/protocol.h b/spice/protocol.h
>>>> index ddfe84b..cbd8295 100644
>>>> --- a/spice/protocol.h
>>>> +++ b/spice/protocol.h
>>>> @@ -37,7 +37,7 @@
>>>>
>>>> #define SPICE_MAGIC (*(uint32_t*)"REDQ")
>>>> #define SPICE_VERSION_MAJOR 2
>>>> -#define SPICE_VERSION_MINOR 1
>>>> +#define SPICE_VERSION_MINOR 2
>>>
>>> if you are bumping the protocol version, why negotiate the capability?
>>> Isn't it implicit (if I can do 2.2 or above, I will omit the sublist) ?
>>> Conversely, if you do have the capability exchange, why bump the version?
>>>
>>>>
>>>> // Encryption&  Ticketing Parameters
>>>> #define SPICE_MAX_PASSWORD_LENGTH 60
>>>> @@ -55,6 +55,7 @@ enum {
>>>> SPICE_COMMON_CAP_PROTOCOL_AUTH_SELECTION,
>>>> SPICE_COMMON_CAP_AUTH_SPICE,
>>>> SPICE_COMMON_CAP_AUTH_SASL,
>>>> + SPICE_COMMON_CAP_HEADER_NO_SUB,
>>>> };
>>>>
>>>> typedef struct SPICE_ATTR_PACKED SpiceLinkMess {
>>>> @@ -89,6 +90,12 @@ typedef struct SPICE_ATTR_PACKED SpiceDataHeader {
>>>> uint32_t sub_list; //offset to SpiceSubMessageList[]
>>>> } SpiceDataHeader;
>>>>
>>>> +typedef struct SPICE_ATTR_PACKED SpiceDataHeaderNoSub {
>>>> + uint64_t serial;
>>>
>>> Ah, if you could also get rid of the serial in the same sweep... More
>>> useless bytes.
>>> At least for some channels, it's completely useless. I suspect some do
>>> use it, for no real reason, though.
>> They are used for real reason. For example, keeping the bitmaps
>> cache consistent, when the guest have several monitors. It was also
>> used in other channels for seamless migration, and we hope to have
>> seamless migration back in the future. It is not used for messages
>> from the client to the server. Even if we want to remove it, it will
>> require different patches then those in this series.
>
> We did discuss reducing the field from 64 bit to 32 bit. Also, since we
> are using TCP transport they are redundant, no? each side can deduce the
> serial based on 1) it starts from 0 (or the starting one can be sent in
> the link message if it isn't zero, i.e. migration) 2) it increments by 1
> per message.
you have a point Alon, I will resend the patches, with a revised header.
>
> Totally agree about it belonging to a seperate patchset. Kaul, hold your
> horses :)
>
>>
>>> Y.
>>>
>>>> + uint16_t type;
>>>> + uint32_t size;
>>>> +} SpiceDataHeaderNoSub;
>>>> +
>>>> typedef struct SPICE_ATTR_PACKED SpiceSubMessage {
>>>> uint16_t type;
>>>> uint32_t size;
>>>
>>
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/spice-devel