[Spice-devel,2/2] server: init all fields on SpiceMsgDisplayStreamCreate

Submitted by Christophe Fergeau on Aug. 2, 2011, 5:31 p.m.

Details

Message ID 1312281117-1970-2-git-send-email-cfergeau@redhat.com
State New, archived
Headers show

Not browsing as part of any series.

Commit Message

Christophe Fergeau Aug. 2, 2011, 5:31 p.m.
This patch is an RFC

red_display_marshall_stream_start initializes a
SpiceMsgDisplayStreamCreate structure before marshalling it and
sending it on the wire. However, it never fills
SpiceMsgDisplayStreamCreate::stamp which then causes a complaint
from valgrind. Initializing it is easy enough, however I have no idea
if 0 is an acceptable value. I put a semi-random value for now in the
hope that someone can enlighten me as to what I can use for ::stamp.
---
 server/red_worker.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/red_worker.c b/server/red_worker.c
index efedc19..dc80259 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -7718,6 +7718,8 @@  static void red_display_marshall_stream_start(DisplayChannel *display_channel,
         stream_create.clip.rects = &clip_rects;
     }
 
+    stream_create.stamp = 0xdeadbeef;
+
     spice_marshall_msg_display_stream_create(base_marshaller, &stream_create);
 }
 

Comments

On Tue, Aug 02, 2011 at 12:31:57PM +0200, Christophe Fergeau wrote:
> This patch is an RFC
> 
> red_display_marshall_stream_start initializes a
> SpiceMsgDisplayStreamCreate structure before marshalling it and
> sending it on the wire. However, it never fills
> SpiceMsgDisplayStreamCreate::stamp which then causes a complaint
> from valgrind. Initializing it is easy enough, however I have no idea
> if 0 is an acceptable value. I put a semi-random value for now in the
> hope that someone can enlighten me as to what I can use for ::stamp.
> ---
>  server/red_worker.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/server/red_worker.c b/server/red_worker.c
> index efedc19..dc80259 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -7718,6 +7718,8 @@ static void red_display_marshall_stream_start(DisplayChannel *display_channel,
>          stream_create.clip.rects = &clip_rects;
>      }
>  
> +    stream_create.stamp = 0xdeadbeef;
> +

Good question. I see mm_time is 32 bit, and this is 64. You could pass the mm_time here. Is
it even used on the other side? I think we only use the timestamps on the frames for synchronization.

0 is still a better value in the meantime :)

>      spice_marshall_msg_display_stream_create(base_marshaller, &stream_create);
>  }
>  
> -- 
> 1.7.6
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On 08/02/2011 01:54 PM, Alon Levy wrote:
> On Tue, Aug 02, 2011 at 12:31:57PM +0200, Christophe Fergeau wrote:
>> This patch is an RFC
>>
>> red_display_marshall_stream_start initializes a
>> SpiceMsgDisplayStreamCreate structure before marshalling it and
>> sending it on the wire. However, it never fills
>> SpiceMsgDisplayStreamCreate::stamp which then causes a complaint
>> from valgrind. Initializing it is easy enough, however I have no idea
>> if 0 is an acceptable value. I put a semi-random value for now in the
>> hope that someone can enlighten me as to what I can use for ::stamp.
>> ---
>>   server/red_worker.c |    2 ++
>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/server/red_worker.c b/server/red_worker.c
>> index efedc19..dc80259 100644
>> --- a/server/red_worker.c
>> +++ b/server/red_worker.c
>> @@ -7718,6 +7718,8 @@ static void red_display_marshall_stream_start(DisplayChannel *display_channel,
>>           stream_create.clip.rects =&clip_rects;
>>       }
>>
>> +    stream_create.stamp = 0xdeadbeef;
>> +
> Good question. I see mm_time is 32 bit, and this is 64. You could pass the mm_time here. Is
> it even used on the other side? I think we only use the timestamps on the frames for synchronization.
>
> 0 is still a better value in the meantime :)

On the wire it looked like zero'ed 8 bytes in STREAM_CREATE, while the 
very next STREAM_DATA message had 4 bytes with reasonable data 
(0x0017940e, which indeed incremented nicely in the next STREAM_DATA 
messages). Coincidence?
(Using git 3582adb989cdb6e1e75bf9341ffcebf35e58b737).

BTW: another 8 bytes we could shave off the protocol one day, I presume.
Y.

>
>>       spice_marshall_msg_display_stream_create(base_marshaller,&stream_create);
>>   }
>>
>> -- 
>> 1.7.6
>>
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On Tue, Aug 02, 2011 at 01:54:37PM +0300, Alon Levy wrote:
> Good question. I see mm_time is 32 bit, and this is 64. You could pass
> the mm_time here. Is it even used on the other side?

You're right, it doesn't seem to be used in client/

> 0 is still a better value in the meantime :)

Yep, sure I wanted a value that stands out to make it clear I have no idea
what value the timestamp should be set to :)

Christophe