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

[Alon Levy]

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.

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