[PATCH] qemu vnc updates

[PATCH] qemu vnc updates

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1153 bytes --]

Hi all,
reading qemu code I realized that the qemu vnc server sometimes sends 
framebuffer updates even if the client didn't request any.
This is not consistent with the RFB protocol spec and can break some 
clients.
The patch I am attaching strictly enforces compliance with the RFB 
protocol making sure framebuffer updates are sent only if the client 
requested one.
Doing so is more difficult than it seems because some framebuffer 
pseudo-encoding updates cannot be discarded but must be sent anyway: for 
example desktop resize and pixel format change messages. To solve the 
problem I wrote a queue that stores those messages and sends them as 
soon as the client asks for an update.
Since 90% of the times the queue is used to store only few elements, the 
queue allocates 10 elements at the beginning and every time it runs out 
of elements allocates other 10 elements. This is should drastically 
limit the number of malloc and free needed to maintain the queue.
I did some stress tests in the last couple of days and seems to work well.
Best Regards,

Stefano Stabellini


Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

[-- Attachment #2: vnc-update.patch --]
[-- Type: text/x-patch, Size: 10984 bytes --]

diff -r d34e99141506 tools/ioemu/vnc.c
--- a/tools/ioemu/vnc.c	Mon Feb 25 16:34:49 2008 +0000
+++ b/tools/ioemu/vnc.c	Mon Feb 25 17:18:17 2008 +0000
@@ -137,6 +137,23 @@ enum {
 
 #endif /* CONFIG_VNC_TLS */
 
+#define QUEUE_ALLOC_UNIT 10
+
+typedef struct _QueueItem
+{
+    int x, y, w, h;
+    int32_t enc;
+    struct _QueueItem *next;
+} QueueItem;
+
+typedef struct _Queue
+{
+    QueueItem *queue_start;
+    int start_count;
+    QueueItem *queue_end;
+    int end_count;
+} Queue;
+
 struct VncState
 {
     QEMUTimer *timer;
@@ -152,6 +169,9 @@ struct VncState
     uint64_t *update_row;	/* outstanding updates */
     int has_update;		/* there's outstanding updates in the
 				 * visible area */
+
+    int update_requested;       /* the client requested an update */
+
     uint8_t *old_data;
     int depth; /* internal VNC frame buffer byte per pixel */
     int has_resize;
@@ -186,6 +206,9 @@ struct VncState
 
     Buffer output;
     Buffer input;
+    
+    Queue upqueue;
+
     kbd_layout_t *kbd_layout;
     /* current output mode information */
     VncWritePixels *write_pixels;
@@ -248,6 +271,11 @@ static void vnc_update_client(void *opaq
 static void vnc_update_client(void *opaque);
 static void vnc_client_read(void *opaque);
 static void framebuffer_set_updated(VncState *vs, int x, int y, int w, int h);
+static void pixel_format_message (VncState *vs);
+static void enqueue_framebuffer_update(VncState *vs, int x, int y, int w, int h, int32_t encoding);
+static void dequeue_framebuffer_update(VncState *vs);
+static int is_empty_queue(VncState *vs);
+static void free_queue(VncState *vs);
 
 #if 0
 static inline void vnc_set_bit(uint32_t *d, int k)
@@ -370,13 +398,18 @@ static void vnc_dpy_resize(DisplayState 
     ds->height = h;
     ds->linesize = w * vs->depth;
     if (vs->csock != -1 && vs->has_resize && size_changed) {
-	vnc_write_u8(vs, 0);  /* msg id */
-	vnc_write_u8(vs, 0);
-	vnc_write_u16(vs, 1); /* number of rects */
-	vnc_framebuffer_update(vs, 0, 0, ds->width, ds->height, -223);
-	vnc_flush(vs);
-	vs->width = ds->width;
-	vs->height = ds->height;
+        vs->width = ds->width;
+        vs->height = ds->height;
+        if (vs->update_requested) {
+	    vnc_write_u8(vs, 0);  /* msg id */
+	    vnc_write_u8(vs, 0);
+	    vnc_write_u16(vs, 1); /* number of rects */
+	    vnc_framebuffer_update(vs, 0, 0, ds->width, ds->height, -223);
+	    vnc_flush(vs);
+            vs->update_requested--;
+        } else {
+            enqueue_framebuffer_update(vs, 0, 0, ds->width, ds->height, -223);
+        }
     }
     vs->dirty_pixel_shift = 0;
     for (o = DIRTY_PIXEL_BITS; o < ds->width; o *= 2)
@@ -553,7 +586,8 @@ static void vnc_copy(DisplayState *ds, i
         return;
     }
 
-    if (src_x < vs->visible_x || src_y < vs->visible_y ||
+    if (!vs->update_requested ||
+        src_x < vs->visible_x || src_y < vs->visible_y ||
 	dst_x < vs->visible_x || dst_y < vs->visible_y ||
 	(src_x + w) > (vs->visible_x + vs->visible_w) ||
 	(src_y + h) > (vs->visible_y + vs->visible_h) ||
@@ -592,6 +626,7 @@ static void vnc_copy(DisplayState *ds, i
 	vnc_write_u16(vs, src_x);
 	vnc_write_u16(vs, src_y);
 	vnc_flush(vs);
+        vs->update_requested--;
     } else
 	framebuffer_set_updated(vs, dst_x, dst_y, w, h);
 }
@@ -624,8 +659,21 @@ static void _vnc_update_client(void *opa
     int maxx, maxy;
     int tile_bytes = vs->depth * DP2X(vs, 1);
 
-    if (vs->csock == -1)
+    if (!vs->update_requested || vs->csock == -1)
 	return;
+    while (!is_empty_queue(vs) && vs->update_requested) {
+        int enc = vs->upqueue.queue_end->enc; 
+        dequeue_framebuffer_update(vs);
+        switch (enc) {
+            case 0x574D5669:
+                pixel_format_message(vs);
+                break;
+            default:
+                break;
+        }
+        vs->update_requested--;
+    }
+    if (!vs->update_requested) return;
 
     now = qemu_get_clock(rt_clock);
 
@@ -717,8 +765,11 @@ static void _vnc_update_client(void *opa
     vs->output.buffer[saved_offset] = (n_rectangles >> 8) & 0xFF;
     vs->output.buffer[saved_offset + 1] = n_rectangles & 0xFF;
 
-    if (n_rectangles == 0)
+    if (n_rectangles == 0) {
+        vs->output.offset = saved_offset - 2;
 	goto backoff;
+    } else
+        vs->update_requested--;
 
     vs->has_update = 0;
     vnc_flush(vs);
@@ -735,7 +786,8 @@ static void _vnc_update_client(void *opa
     vs->timer_interval += VNC_REFRESH_INTERVAL_INC;
     if (vs->timer_interval > VNC_REFRESH_INTERVAL_MAX) {
 	vs->timer_interval = VNC_REFRESH_INTERVAL_MAX;
-	if (now - vs->last_update_time >= VNC_MAX_UPDATE_INTERVAL) {
+	if (now - vs->last_update_time >= VNC_MAX_UPDATE_INTERVAL &&
+            vs->update_requested) {
 	    /* Send a null update.  If the client is no longer
 	       interested (e.g. minimised) it'll ignore this, and we
 	       can stop scanning the buffer until it sends another
@@ -752,6 +804,7 @@ static void _vnc_update_client(void *opa
 	    send_framebuffer_update(vs, 0, 0, 1, 1);
 	    vnc_flush(vs);
 	    vs->last_update_time = now;
+            vs->update_requested--;
 	    return;
 	}
     }
@@ -821,6 +874,88 @@ static void buffer_append(Buffer *buffer
     buffer->offset += len;
 }
 
+static void enqueue_framebuffer_update(VncState *vs, int x, int y, int w, int h,
+                                   int32_t encoding)
+{
+    Queue *q = &vs->upqueue; 
+    if (q->queue_end != NULL) {
+        if (q->queue_end != q->queue_start || q->start_count != q->end_count) {
+            if (q->queue_end->next == NULL) {
+                q->queue_end->next = (QueueItem *) qemu_mallocz (sizeof(QueueItem) * QUEUE_ALLOC_UNIT);
+                q->end_count = QUEUE_ALLOC_UNIT;
+            }
+            q->queue_end = q->queue_end->next;
+        }
+    } else {
+        q->queue_end = (QueueItem *) qemu_mallocz (sizeof(QueueItem) * QUEUE_ALLOC_UNIT);
+        q->queue_start = q->queue_end;
+        q->start_count = QUEUE_ALLOC_UNIT;
+        q->end_count = QUEUE_ALLOC_UNIT;
+    }
+    q->end_count--;
+
+    q->queue_end->x = x;
+    q->queue_end->y = y;
+    q->queue_end->w = w;
+    q->queue_end->h = h;
+    q->queue_end->enc = encoding;
+    q->queue_end->next = (q->end_count > 0) ? (q->queue_end + 1) : NULL;
+}
+
+static void dequeue_framebuffer_update(VncState *vs)
+{
+    Queue *q = &vs->upqueue;
+    if (q->queue_start == NULL || 
+            (q->queue_end == q->queue_start && q->start_count == q->end_count))
+        return;
+
+    vnc_write_u8(vs, 0);
+    vnc_write_u8(vs, 0);
+    vnc_write_u16(vs, 1);
+    vnc_framebuffer_update(vs, q->queue_start->x, q->queue_start->y,
+            q->queue_start->w, q->queue_start->h, q->queue_start->enc);
+
+    q->start_count--;
+    if (q->queue_end != q->queue_start) {
+        if (!q->start_count) {
+            QueueItem *i = q->queue_start;
+            q->queue_start = q->queue_start->next;
+            q->start_count = QUEUE_ALLOC_UNIT;
+            free (i - QUEUE_ALLOC_UNIT + 1);
+        } else
+            q->queue_start = q->queue_start->next;
+    } else {
+        q->queue_end = q->queue_end - QUEUE_ALLOC_UNIT + q->end_count + 1;
+        q->queue_start = q->queue_end;
+        q->end_count = QUEUE_ALLOC_UNIT;
+        q->start_count = QUEUE_ALLOC_UNIT;
+    }
+}
+
+static int is_empty_queue(VncState *vs)
+{
+    Queue *q = &vs->upqueue;
+    if (q->queue_end == NULL) return 1;
+    if (q->queue_end == q->queue_start && q->start_count == q->end_count) return 1;
+    return 0;
+}
+
+static void free_queue(VncState *vs)
+{
+    Queue *q = &vs->upqueue;
+    while (q->queue_start != NULL) {
+        QueueItem *i;
+        q->queue_start = q->queue_start + q->start_count - 1;
+        i = q->queue_start;
+        q->queue_start = q->queue_start->next;
+        free(i - QUEUE_ALLOC_UNIT + 1);
+        q->start_count = QUEUE_ALLOC_UNIT;
+    }
+    q->queue_end = NULL;
+    q->start_count = 0;
+    q->end_count = 0;
+}
+
 static int vnc_client_io_error(VncState *vs, int ret, int last_errno)
 {
     if (ret == 0 || ret == -1) {
@@ -833,6 +968,8 @@ static int vnc_client_io_error(VncState 
 	vs->csock = -1;
 	buffer_reset(&vs->input);
 	buffer_reset(&vs->output);
+        free_queue(vs);
+        vs->update_requested = 0;
 #if CONFIG_VNC_TLS
 	if (vs->tls_session) {
 	    gnutls_deinit(vs->tls_session);
@@ -1044,12 +1181,18 @@ static void check_pointer_type_change(Vn
 static void check_pointer_type_change(VncState *vs, int absolute)
 {
     if (vs->has_pointer_type_change && vs->absolute != absolute) {
-	vnc_write_u8(vs, 0);
-	vnc_write_u8(vs, 0);
-	vnc_write_u16(vs, 1);
-	vnc_framebuffer_update(vs, absolute, 0,
+        if (vs->update_requested) {
+	    vnc_write_u8(vs, 0);
+	    vnc_write_u8(vs, 0);
+	    vnc_write_u16(vs, 1);
+	    vnc_framebuffer_update(vs, absolute, 0,
 			       vs->ds->width, vs->ds->height, -257);
-	vnc_flush(vs);
+	    vnc_flush(vs);
+            vs->update_requested--;
+        } else {
+            enqueue_framebuffer_update(vs, absolute, 0,
+                               vs->ds->width, vs->ds->height, -257);
+        }
     }
     vs->absolute = absolute;
 }
@@ -1316,6 +1459,7 @@ static void framebuffer_update_request(V
     vs->visible_w = w;
     vs->visible_h = h;
 
+    vs->update_requested++;
     qemu_mod_timer(vs->timer, qemu_get_clock(rt_clock));
 }
 
@@ -1536,12 +1680,17 @@ static void vnc_dpy_colourdepth(DisplayS
         vnc_client_error(vs);
     } else if (vs->csock != -1 && vs->has_WMVi) {
         /* Sending a WMVi message to notify the client*/
-        vnc_write_u8(vs, 0);  /* msg id */
-        vnc_write_u8(vs, 0);
-        vnc_write_u16(vs, 1); /* number of rects */
-        vnc_framebuffer_update(vs, 0, 0, ds->width, ds->height, 0x574D5669);
-        pixel_format_message(vs);
-        vnc_flush(vs);
+        if (vs->update_requested) {
+            vnc_write_u8(vs, 0);  /* msg id */
+            vnc_write_u8(vs, 0);
+            vnc_write_u16(vs, 1); /* number of rects */
+            vnc_framebuffer_update(vs, 0, 0, ds->width, ds->height, 0x574D5669);
+            pixel_format_message(vs);
+            vnc_flush(vs);
+            vs->update_requested--;
+        } else {
+            enqueue_framebuffer_update(vs, 0, 0, ds->width, ds->height, 0x574D5669);
+        }
     } else {
         if (vs->pix_bpp == 4 && vs->depth == 4 &&
             host_big_endian_flag == vs->pix_big_endian &&
@@ -2291,6 +2440,7 @@ static void vnc_listen_read(void *opaque
 	framebuffer_set_updated(vs, 0, 0, vs->ds->width, vs->ds->height);
 	vs->has_resize = 0;
 	vs->has_hextile = 0;
+        vs->update_requested = 0;
 	vs->ds->dpy_copy = NULL;
 	vnc_timer_init(vs);
     }
@@ -2413,6 +2563,8 @@ void vnc_display_close(DisplayState *ds)
 	vs->csock = -1;
 	buffer_reset(&vs->input);
 	buffer_reset(&vs->output);
+        free_queue(vs);
+        vs->update_requested = 0;
 #if CONFIG_VNC_TLS
 	if (vs->tls_session) {
 	    gnutls_deinit(vs->tls_session);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] qemu vnc updates

[Anthony Liguori]

Stefano Stabellini wrote:
> Hi all,
> reading qemu code I realized that the qemu vnc server sometimes sends 
> framebuffer updates even if the client didn't request any.
> This is not consistent with the RFB protocol spec and can break some 
> clients.


It's actually consistent with the RFB spec.  Have you seen any clients 
break?

The RFB spec states pretty clearly that a single 
FramebufferUpdateRequest may generate 0 or more FramebufferUpdate 
events.  Once a client has sent a single FramebufferUpdate request, it 
should expect to continue to receive more FramebufferUpdates for an 
indefinite period of time according to the specification.

Regards,

Anthony Liguori

> The patch I am attaching strictly enforces compliance with the RFB 
> protocol making sure framebuffer updates are sent only if the client 
> requested one.
> Doing so is more difficult than it seems because some framebuffer 
> pseudo-encoding updates cannot be discarded but must be sent anyway: for 
> example desktop resize and pixel format change messages. To solve the 
> problem I wrote a queue that stores those messages and sends them as 
> soon as the client asks for an update.
> Since 90% of the times the queue is used to store only few elements, the 
> queue allocates 10 elements at the beginning and every time it runs out 
> of elements allocates other 10 elements. This is should drastically 
> limit the number of malloc and free needed to maintain the queue.
> I did some stress tests in the last couple of days and seems to work well.
> Best Regards,
> 
> Stefano Stabellini
> 
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: Re: [PATCH] qemu vnc updates

[Daniel P. Berrange]

On Tue, Feb 26, 2008 at 10:15:51AM -0600, Anthony Liguori wrote:
> Stefano Stabellini wrote:
> >Hi all,
> >reading qemu code I realized that the qemu vnc server sometimes sends 
> >framebuffer updates even if the client didn't request any.
> >This is not consistent with the RFB protocol spec and can break some 
> >clients.
> 
> 
> It's actually consistent with the RFB spec.  Have you seen any clients 
> break?
> 
> The RFB spec states pretty clearly that a single 
> FramebufferUpdateRequest may generate 0 or more FramebufferUpdate 
> events.  Once a client has sent a single FramebufferUpdate request, it 
> should expect to continue to receive more FramebufferUpdates for an 
> indefinite period of time according to the specification.

The reverse is true too - the server may coallese multiple FramebufferUpdateRequest
into a single FramebufferUpdate reply. There is no 1-to-1 mapping between
request & reply as this patch attempts to enforce.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: Re: [PATCH] qemu vnc updates

[Stefano Stabellini]

Daniel P. Berrange wrote:
> On Tue, Feb 26, 2008 at 10:15:51AM -0600, Anthony Liguori wrote:
>> Stefano Stabellini wrote:
>>> Hi all,
>>> reading qemu code I realized that the qemu vnc server sometimes sends 
>>> framebuffer updates even if the client didn't request any.
>>> This is not consistent with the RFB protocol spec and can break some 
>>> clients.
>>
>> It's actually consistent with the RFB spec.  Have you seen any clients 
>> break?
>>
>> The RFB spec states pretty clearly that a single 
>> FramebufferUpdateRequest may generate 0 or more FramebufferUpdate 
>> events.  Once a client has sent a single FramebufferUpdate request, it 
>> should expect to continue to receive more FramebufferUpdates for an 
>> indefinite period of time according to the specification.
> 
> The reverse is true too - the server may coallese multiple FramebufferUpdateRequest
> into a single FramebufferUpdate reply. There is no 1-to-1 mapping between
> request & reply as this patch attempts to enforce.
> 

I have just re-read the rfb protocol spec and it specifies quite clearly 
that framebuffer updates are sent in response to framebuffer update 
requests.
However it is true that the server can collapse multiple requests in a 
single reply. It is also clear that the reply can come at any time.

I have seen the linux vncviewer client breaking because of race 
conditions due to this problem.

Re: Re: [PATCH] qemu vnc updates

[Anthony Liguori]

Stefano Stabellini wrote:
>
> I have just re-read the rfb protocol spec and it specifies quite 
> clearly that framebuffer updates are sent in response to framebuffer 
> update requests.
> However it is true that the server can collapse multiple requests in a 
> single reply. It is also clear that the reply can come at any time.
>
> I have seen the linux vncviewer client breaking because of race 
> conditions due to this problem.

You mean realvnc?  The race condition is due to it's use of 
SetPixelFormat.  By slowing the updates, what you're really doing is 
just working around that race condition.  That's not a proper solution 
though.

This goes away if you set the realvnc options so that it doesn't change 
the pixel format from what the server specifies.

Regards,

Anthony Liguori

Re: Re: [PATCH] qemu vnc updates

[Stefano Stabellini]

Anthony Liguori wrote:
> You mean realvnc?  The race condition is due to it's use of 
> SetPixelFormat.  By slowing the updates, what you're really doing is 
> just working around that race condition.  That's not a proper solution 
> though.
> 
> This goes away if you set the realvnc options so that it doesn't change 
> the pixel format from what the server specifies.
> 

I think that the realvnc "bug" is due to the fact that they assume that 
if they don't send any update requests, they shouldn't expect any.
This is not a wrong assumption to make unless you are right about the 1 
request -> N reply argument (I still think that this is not what the rfb 
spec says).
Besides my patch doesn't slow down the connection so much, I cannot tell 
the difference on a quick connections. I'll let you know what is the 
behaviour on a slow connection.

Re: Re: [PATCH] qemu vnc updates

[Anthony Liguori]

Stefano Stabellini wrote:
> Anthony Liguori wrote:
>> You mean realvnc?  The race condition is due to it's use of 
>> SetPixelFormat.  By slowing the updates, what you're really doing is 
>> just working around that race condition.  That's not a proper 
>> solution though.
>>
>> This goes away if you set the realvnc options so that it doesn't 
>> change the pixel format from what the server specifies.
>>
>
> I think that the realvnc "bug" is due to the fact that they assume 
> that if they don't send any update requests, they shouldn't expect any.
> This is not a wrong assumption to make unless you are right about the 
> 1 request -> N reply argument (I still think that this is not what the 
> rfb spec says).

Regardless of whether you interpret the spec to allow 1 req -> N 
replies, the spec is clear that each multiple requests can result in a 
single reply.  The requests/replies aren't sequenced though so you have 
no way of knowing what reply the request is in response too.  For 
instance, consider the following:

Request  =>  Reply
Request  =>  Reply
Request  =>
Request  =>  Reply
Request  =>  Reply

This is entirely indistinguishable from:

Request  =>  Reply
Request  =>  Reply
Request  =>
Request  =>
Request  =>  Reply
                =>  Reply

Because the requests and replies don't carry any sort of sequence number.

Regards,

Anthony Liguori

> Besides my patch doesn't slow down the connection so much, I cannot 
> tell the difference on a quick connections. I'll let you know what is 
> the behaviour on a slow connection.