qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH RESEND v2 1/2] fix vnc regression
@ 2011-03-02  3:46 Wen Congyang
  2011-03-02  3:58 ` [Qemu-devel] [PATCH RESEND 2/2] vnc: Fix heap corruption Wen Congyang
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Wen Congyang @ 2011-03-02  3:46 UTC (permalink / raw)
  To: qemu-devel, Stefan Weil, Anthony Liguori, Corentin Chary

This patch fix the following two regressions:
1. we should use bitmap_set() and bitmap_clear() to replace vnc_set_bits().
2. The unit of bitmap_intersects()'third parameter is bit, not words.
   But we pass the num of words to bitmap_intersects().

Changes from v1 to v2:
1. fix the third argument of bitmap_clear()

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>

---
 ui/vnc.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 610f884..e3761b0 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1645,17 +1645,21 @@ static void framebuffer_update_request(VncState *vs, int incremental,
                                        int x_position, int y_position,
                                        int w, int h)
 {
+    int i;
+    const size_t width = ds_get_width(vs->ds) / 16;
+
     if (y_position > ds_get_height(vs->ds))
         y_position = ds_get_height(vs->ds);
     if (y_position + h >= ds_get_height(vs->ds))
         h = ds_get_height(vs->ds) - y_position;
 
-    int i;
     vs->need_update = 1;
     if (!incremental) {
         vs->force_update = 1;
         for (i = 0; i < h; i++) {
-            bitmap_set(vs->dirty[y_position + i], x_position / 16, w / 16);
+            bitmap_set(vs->dirty[y_position + i], 0, width);
+            bitmap_clear(vs->dirty[y_position + i], width,
+                         VNC_DIRTY_WORDS * BITS_PER_LONG - width);
         }
     }
 }
@@ -2406,7 +2410,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
     guest_row  = vd->guest.ds->data;
     server_row = vd->server->data;
     for (y = 0; y < vd->guest.ds->height; y++) {
-        if (bitmap_intersects(vd->guest.dirty[y], width_mask, VNC_DIRTY_WORDS)) {
+        if (bitmap_intersects(vd->guest.dirty[y], width_mask,
+            VNC_DIRTY_WORDS * BITS_PER_LONG)) {
             int x;
             uint8_t *guest_ptr;
             uint8_t *server_ptr;
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PATCH RESEND 2/2] vnc: Fix heap corruption
  2011-03-02  3:46 [Qemu-devel] [PATCH RESEND v2 1/2] fix vnc regression Wen Congyang
@ 2011-03-02  3:58 ` Wen Congyang
  2011-03-02 10:57   ` [Qemu-devel] " Corentin Chary
  2011-03-02 10:56 ` [Qemu-devel] Re: [PATCH RESEND v2 1/2] fix vnc regression Corentin Chary
  2011-03-03  2:44 ` [Qemu-devel] [PATCH 3/3] correct VNC_DIRTY_WORDS on 64 bit machine Wen Congyang
  2 siblings, 1 reply; 14+ messages in thread
From: Wen Congyang @ 2011-03-02  3:58 UTC (permalink / raw)
  To: qemu-devel, Stefan Weil, Anthony Liguori, Corentin Chary

This bug is reported by Stefan Weil:
========
Commit bc2429b9174ac2d3c56b7fd35884b0d89ec7fb02 introduced
a severe bug (heap corruption).

bitmap_clear was called with a wrong argument
which caused out-of-bound writes to width_mask.

This bug was detected with QEMU running on windows.
It also occurs with wine:

*** stack smashing detected ***:  terminated
wine: Unhandled illegal instruction at address 0x6115c7 (thread 0009), starting debugger...

The bug is not windows specific!
========

The third argument of bitmap_clear() is number of bits to be cleared, but we pass
the end bits to be cleared to bitmap_clear().

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Reported-by: Stefan Weil <weil@mail.berlios.de>

---
 ui/vnc.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index e3761b0..e7d0b5b 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2390,6 +2390,7 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
     unsigned long width_mask[VNC_DIRTY_WORDS];
     VncState *vs;
     int has_dirty = 0;
+    const size_t width = ds_get_width(vd->ds) / 16;
 
     struct timeval tv = { 0, 0 };
 
@@ -2403,9 +2404,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
      * Check and copy modified bits from guest to server surface.
      * Update server dirty map.
      */
-    bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
-    bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
-                 VNC_DIRTY_WORDS * BITS_PER_LONG);
+    bitmap_set(width_mask, 0, width);
+    bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG - width);
     cmp_bytes = 16 * ds_get_bytes_per_pixel(vd->ds);
     guest_row  = vd->guest.ds->data;
     server_row = vd->server->data;
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Qemu-devel] Re: [PATCH RESEND v2 1/2] fix vnc regression
  2011-03-02  3:46 [Qemu-devel] [PATCH RESEND v2 1/2] fix vnc regression Wen Congyang
  2011-03-02  3:58 ` [Qemu-devel] [PATCH RESEND 2/2] vnc: Fix heap corruption Wen Congyang
@ 2011-03-02 10:56 ` Corentin Chary
  2011-03-03  2:44 ` [Qemu-devel] [PATCH 3/3] correct VNC_DIRTY_WORDS on 64 bit machine Wen Congyang
  2 siblings, 0 replies; 14+ messages in thread
From: Corentin Chary @ 2011-03-02 10:56 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: qemu-devel

On Wed, Mar 2, 2011 at 3:46 AM, Wen Congyang <wency@cn.fujitsu.com> wrote:
> This patch fix the following two regressions:
> 1. we should use bitmap_set() and bitmap_clear() to replace vnc_set_bits().
> 2. The unit of bitmap_intersects()'third parameter is bit, not words.
>   But we pass the num of words to bitmap_intersects().
>
> Changes from v1 to v2:
> 1. fix the third argument of bitmap_clear()
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>

Acked-by: Corentin Chary <corentin.chary@gmail.com>

> ---
>  ui/vnc.c |   11 ++++++++---
>  1 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 610f884..e3761b0 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -1645,17 +1645,21 @@ static void framebuffer_update_request(VncState *vs, int incremental,
>                                        int x_position, int y_position,
>                                        int w, int h)
>  {
> +    int i;
> +    const size_t width = ds_get_width(vs->ds) / 16;
> +
>     if (y_position > ds_get_height(vs->ds))
>         y_position = ds_get_height(vs->ds);
>     if (y_position + h >= ds_get_height(vs->ds))
>         h = ds_get_height(vs->ds) - y_position;
>
> -    int i;
>     vs->need_update = 1;
>     if (!incremental) {
>         vs->force_update = 1;
>         for (i = 0; i < h; i++) {
> -            bitmap_set(vs->dirty[y_position + i], x_position / 16, w / 16);
> +            bitmap_set(vs->dirty[y_position + i], 0, width);
> +            bitmap_clear(vs->dirty[y_position + i], width,
> +                         VNC_DIRTY_WORDS * BITS_PER_LONG - width);
>         }
>     }
>  }
> @@ -2406,7 +2410,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
>     guest_row  = vd->guest.ds->data;
>     server_row = vd->server->data;
>     for (y = 0; y < vd->guest.ds->height; y++) {
> -        if (bitmap_intersects(vd->guest.dirty[y], width_mask, VNC_DIRTY_WORDS)) {
> +        if (bitmap_intersects(vd->guest.dirty[y], width_mask,
> +            VNC_DIRTY_WORDS * BITS_PER_LONG)) {
>             int x;
>             uint8_t *guest_ptr;
>             uint8_t *server_ptr;
> --
> 1.7.1
>



-- 
Corentin Chary
http://xf.iksaif.net

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
  2011-03-02  3:58 ` [Qemu-devel] [PATCH RESEND 2/2] vnc: Fix heap corruption Wen Congyang
@ 2011-03-02 10:57   ` Corentin Chary
  2011-03-02 18:36     ` Stefan Weil
  0 siblings, 1 reply; 14+ messages in thread
From: Corentin Chary @ 2011-03-02 10:57 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: qemu-devel

On Wed, Mar 2, 2011 at 3:58 AM, Wen Congyang <wency@cn.fujitsu.com> wrote:
> This bug is reported by Stefan Weil:
> ========
> Commit bc2429b9174ac2d3c56b7fd35884b0d89ec7fb02 introduced
> a severe bug (heap corruption).
>
> bitmap_clear was called with a wrong argument
> which caused out-of-bound writes to width_mask.
>
> This bug was detected with QEMU running on windows.
> It also occurs with wine:
>
> *** stack smashing detected ***:  terminated
> wine: Unhandled illegal instruction at address 0x6115c7 (thread 0009), starting debugger...
>
> The bug is not windows specific!
> ========
>
> The third argument of bitmap_clear() is number of bits to be cleared, but we pass
> the end bits to be cleared to bitmap_clear().
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Reported-by: Stefan Weil <weil@mail.berlios.de>

Acked-by: Corentin Chary <corentin.chary@gmail.com>

> ---
>  ui/vnc.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index e3761b0..e7d0b5b 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2390,6 +2390,7 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
>     unsigned long width_mask[VNC_DIRTY_WORDS];
>     VncState *vs;
>     int has_dirty = 0;
> +    const size_t width = ds_get_width(vd->ds) / 16;
>
>     struct timeval tv = { 0, 0 };
>
> @@ -2403,9 +2404,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
>      * Check and copy modified bits from guest to server surface.
>      * Update server dirty map.
>      */
> -    bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
> -    bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
> -                 VNC_DIRTY_WORDS * BITS_PER_LONG);
> +    bitmap_set(width_mask, 0, width);
> +    bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG - width);
>     cmp_bytes = 16 * ds_get_bytes_per_pixel(vd->ds);
>     guest_row  = vd->guest.ds->data;
>     server_row = vd->server->data;
> --
> 1.7.1
>
>



-- 
Corentin Chary
http://xf.iksaif.net

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
  2011-03-02 10:57   ` [Qemu-devel] " Corentin Chary
@ 2011-03-02 18:36     ` Stefan Weil
  2011-03-02 18:47       ` Peter Maydell
  0 siblings, 1 reply; 14+ messages in thread
From: Stefan Weil @ 2011-03-02 18:36 UTC (permalink / raw)
  To: Corentin Chary; +Cc: Anthony Liguori, qemu-devel

Am 02.03.2011 11:57, schrieb Corentin Chary:
> On Wed, Mar 2, 2011 at 3:58 AM, Wen Congyang <wency@cn.fujitsu.com> wrote:
>> This bug is reported by Stefan Weil:
>> ========
>> Commit bc2429b9174ac2d3c56b7fd35884b0d89ec7fb02 introduced
>> a severe bug (heap corruption).
>>
>> bitmap_clear was called with a wrong argument
>> which caused out-of-bound writes to width_mask.
>>
>> This bug was detected with QEMU running on windows.
>> It also occurs with wine:
>>
>> *** stack smashing detected ***:  terminated
>> wine: Unhandled illegal instruction at address 0x6115c7 (thread 
>> 0009), starting debugger...
>>
>> The bug is not windows specific!
>> ========
>>
>> The third argument of bitmap_clear() is number of bits to be cleared, 
>> but we pass
>> the end bits to be cleared to bitmap_clear().
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Reported-by: Stefan Weil <weil@mail.berlios.de>
>
> Acked-by: Corentin Chary <corentin.chary@gmail.com>
>

No. I dont't think that the third parameter of bitmap_clear is
ok like that. See my patch for the correct value.

My own patch is also incomplete, so I'll send an update.

Stefan


>> ---
>>   ui/vnc.c |    6 +++---
>>   1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index e3761b0..e7d0b5b 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -2390,6 +2390,7 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
>>      unsigned long width_mask[VNC_DIRTY_WORDS];
>>      VncState *vs;
>>      int has_dirty = 0;
>> +    const size_t width = ds_get_width(vd->ds) / 16;
>>
>>      struct timeval tv = { 0, 0 };
>>
>> @@ -2403,9 +2404,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
>>       * Check and copy modified bits from guest to server surface.
>>       * Update server dirty map.
>>       */
>> -    bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
>> -    bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
>> -                 VNC_DIRTY_WORDS * BITS_PER_LONG);
>> +    bitmap_set(width_mask, 0, width);
>> +    bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG - width);
>>      cmp_bytes = 16 * ds_get_bytes_per_pixel(vd->ds);
>>      guest_row  = vd->guest.ds->data;
>>      server_row = vd->server->data;
>> --
>> 1.7.1
>>
>>
>
>
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
  2011-03-02 18:36     ` Stefan Weil
@ 2011-03-02 18:47       ` Peter Maydell
  2011-03-02 22:01         ` Stefan Weil
  0 siblings, 1 reply; 14+ messages in thread
From: Peter Maydell @ 2011-03-02 18:47 UTC (permalink / raw)
  To: Stefan Weil; +Cc: Anthony Liguori, qemu-devel, Corentin Chary

On 2 March 2011 18:36, Stefan Weil <weil@mail.berlios.de> wrote:
> No. I dont't think that the third parameter of bitmap_clear is
> ok like that. See my patch for the correct value.

Wen's patch:

+    const size_t width = ds_get_width(vd->ds) / 16;
[...]
-    bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
-    bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
-                 VNC_DIRTY_WORDS * BITS_PER_LONG);
+    bitmap_set(width_mask, 0, width);
+    bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG - width);

Your patch:

     bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
-                 VNC_DIRTY_WORDS * BITS_PER_LONG);
+                 (VNC_MAX_WIDTH - ds_get_width(vd->ds)) / 16);

Since ui/vnc.h has:

#define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))

the third parameter to bitmap_clear is the same value in
both cases, isn't it? Or is this a rounding bug?

-- PMM

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
  2011-03-02 18:47       ` Peter Maydell
@ 2011-03-02 22:01         ` Stefan Weil
  2011-03-02 22:27           ` Stefan Weil
  2011-03-02 22:40           ` Peter Maydell
  0 siblings, 2 replies; 14+ messages in thread
From: Stefan Weil @ 2011-03-02 22:01 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Anthony Liguori, qemu-devel, Corentin Chary

Am 02.03.2011 19:47, schrieb Peter Maydell:
> On 2 March 2011 18:36, Stefan Weil <weil@mail.berlios.de> wrote:
>> No. I dont't think that the third parameter of bitmap_clear is
>> ok like that. See my patch for the correct value.
>
> Wen's patch:
>
> + const size_t width = ds_get_width(vd->ds) / 16;
> [...]
> -    bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
> -    bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
> -                 VNC_DIRTY_WORDS * BITS_PER_LONG);
> +    bitmap_set(width_mask, 0, width);
> +    bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG - 
> width);
>
> Your patch:
>
> bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
> - VNC_DIRTY_WORDS * BITS_PER_LONG);
> + (VNC_MAX_WIDTH - ds_get_width(vd->ds)) / 16);
>
> Since ui/vnc.h has:
>
> #define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))
>
> the third parameter to bitmap_clear is the same value in
> both cases, isn't it? Or is this a rounding bug?
>
> -- PMM

Because of rounding effects, both values can be different.

The part missing in my patch is correct handling of another
rounding effect:

VNC_DIRTY_WORDS is exact for 32 bit long values (and the
"old" code which used uint32_t until some weeks ago), where
VNC_DIRTY_WORDS = 2560/16/32 = 5.

For 64 bit values, VNC_DIRTY_WORDS = 2560/16/64 = 2 (rounded)!

Stefan W.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
  2011-03-02 22:01         ` Stefan Weil
@ 2011-03-02 22:27           ` Stefan Weil
  2011-03-03  1:37             ` Wen Congyang
  2011-03-02 22:40           ` Peter Maydell
  1 sibling, 1 reply; 14+ messages in thread
From: Stefan Weil @ 2011-03-02 22:27 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Anthony Liguori, qemu-devel, Corentin Chary

Am 02.03.2011 23:01, schrieb Stefan Weil:
> Am 02.03.2011 19:47, schrieb Peter Maydell:
>> On 2 March 2011 18:36, Stefan Weil <weil@mail.berlios.de> wrote:
>>> No. I dont't think that the third parameter of bitmap_clear is
>>> ok like that. See my patch for the correct value.
>>
>> Wen's patch:
>>
>> + const size_t width = ds_get_width(vd->ds) / 16;
>> [...]
>> -    bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
>> -    bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
>> -                 VNC_DIRTY_WORDS * BITS_PER_LONG);
>> +    bitmap_set(width_mask, 0, width);
>> +    bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG 
>> - width);
>>
>> Your patch:
>>
>> bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
>> - VNC_DIRTY_WORDS * BITS_PER_LONG);
>> + (VNC_MAX_WIDTH - ds_get_width(vd->ds)) / 16);
>>
>> Since ui/vnc.h has:
>>
>> #define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))
>>
>> the third parameter to bitmap_clear is the same value in
>> both cases, isn't it? Or is this a rounding bug?
>>
>> -- PMM
>
> Because of rounding effects, both values can be different.
>
> The part missing in my patch is correct handling of another
> rounding effect:
>
> VNC_DIRTY_WORDS is exact for 32 bit long values (and the
> "old" code which used uint32_t until some weeks ago), where
> VNC_DIRTY_WORDS = 2560/16/32 = 5.
>
> For 64 bit values, VNC_DIRTY_WORDS = 2560/16/64 = 2 (rounded)!
>
> Stefan W.


Is bitmap_clear() really needed here? Meanwhile I think it is not,
so this might be a new patch variant...

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
  2011-03-02 22:01         ` Stefan Weil
  2011-03-02 22:27           ` Stefan Weil
@ 2011-03-02 22:40           ` Peter Maydell
  1 sibling, 0 replies; 14+ messages in thread
From: Peter Maydell @ 2011-03-02 22:40 UTC (permalink / raw)
  To: Stefan Weil; +Cc: Anthony Liguori, qemu-devel, Corentin Chary

On 2 March 2011 22:01, Stefan Weil <weil@mail.berlios.de> wrote:
> The part missing in my patch is correct handling of another
> rounding effect:
>
> VNC_DIRTY_WORDS is exact for 32 bit long values (and the
> "old" code which used uint32_t until some weeks ago), where
> VNC_DIRTY_WORDS = 2560/16/32 = 5.
>
> For 64 bit values, VNC_DIRTY_WORDS = 2560/16/64 = 2 (rounded)!

Yes, I noticed that after I posted. Given that we have arrays
like
     unsigned long dirty[VNC_MAX_HEIGHT][VNC_DIRTY_WORDS];
rounding down rather than up looks a bit suspicious...

(Can we just make VNC_MAX_WIDTH a multiple of 64, or is it
baked into the VNC protocol?)

-- PMM

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
  2011-03-02 22:27           ` Stefan Weil
@ 2011-03-03  1:37             ` Wen Congyang
  0 siblings, 0 replies; 14+ messages in thread
From: Wen Congyang @ 2011-03-03  1:37 UTC (permalink / raw)
  To: Stefan Weil; +Cc: Peter Maydell, Anthony Liguori, qemu-devel, Corentin Chary

At 03/03/2011 06:27 AM, Stefan Weil Write:
> Am 02.03.2011 23:01, schrieb Stefan Weil:
>> Am 02.03.2011 19:47, schrieb Peter Maydell:
>>> On 2 March 2011 18:36, Stefan Weil <weil@mail.berlios.de> wrote:
>>>> No. I dont't think that the third parameter of bitmap_clear is
>>>> ok like that. See my patch for the correct value.
>>>
>>> Wen's patch:
>>>
>>> + const size_t width = ds_get_width(vd->ds) / 16;
>>> [...]
>>> -    bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
>>> -    bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
>>> -                 VNC_DIRTY_WORDS * BITS_PER_LONG);
>>> +    bitmap_set(width_mask, 0, width);
>>> +    bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG
>>> - width);
>>>
>>> Your patch:
>>>
>>> bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
>>> - VNC_DIRTY_WORDS * BITS_PER_LONG);
>>> + (VNC_MAX_WIDTH - ds_get_width(vd->ds)) / 16);
>>>
>>> Since ui/vnc.h has:
>>>
>>> #define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))
>>>
>>> the third parameter to bitmap_clear is the same value in
>>> both cases, isn't it? Or is this a rounding bug?
>>>
>>> -- PMM
>>
>> Because of rounding effects, both values can be different.
>>
>> The part missing in my patch is correct handling of another
>> rounding effect:
>>
>> VNC_DIRTY_WORDS is exact for 32 bit long values (and the
>> "old" code which used uint32_t until some weeks ago), where
>> VNC_DIRTY_WORDS = 2560/16/32 = 5.
>>
>> For 64 bit values, VNC_DIRTY_WORDS = 2560/16/64 = 2 (rounded)!
>>
>> Stefan W.
> 
> 
> Is bitmap_clear() really needed here? Meanwhile I think it is not,
> so this might be a new patch variant...

I do not know why we call bitmap_clear() hear. I only know it is
the same as the old code:

-static inline void vnc_set_bits(uint32_t *d, int n, int nb_words)
-{
-    int j;
-
-    j = 0;
-    while (n >= 32) {
-        d[j++] = -1;
-        n -= 32;
-    }
-    if (n > 0)
-        d[j++] = (1 << n) - 1;
-    while (j < nb_words)               <=== bitmap_clear()
-        d[j++] = 0;
-}

-    vnc_set_bits(width_mask, (ds_get_width(vd->ds) / 16), VNC_DIRTY_WORDS);
+    bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
+    bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
+                 VNC_DIRTY_WORDS * BITS_PER_LONG);


> 
> 
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PATCH 3/3] correct VNC_DIRTY_WORDS on 64 bit machine
  2011-03-02  3:46 [Qemu-devel] [PATCH RESEND v2 1/2] fix vnc regression Wen Congyang
  2011-03-02  3:58 ` [Qemu-devel] [PATCH RESEND 2/2] vnc: Fix heap corruption Wen Congyang
  2011-03-02 10:56 ` [Qemu-devel] Re: [PATCH RESEND v2 1/2] fix vnc regression Corentin Chary
@ 2011-03-03  2:44 ` Wen Congyang
  2011-03-03  6:41   ` [Qemu-devel] " Corentin Chary
  2 siblings, 1 reply; 14+ messages in thread
From: Wen Congyang @ 2011-03-03  2:44 UTC (permalink / raw)
  To: qemu-devel, Stefan Weil, Anthony Liguori, Corentin Chary

VNC_DIRTY_WORDS is wrong on 64 bit long machine.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>

---
 ui/vnc.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/ui/vnc.h b/ui/vnc.h
index 8a1e7b9..239a7a7 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -81,7 +81,8 @@ typedef void VncSendHextileTile(VncState *vs,
 
 #define VNC_MAX_WIDTH 2560
 #define VNC_MAX_HEIGHT 2048
-#define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))
+#define divideup(x, y)  (((x) + ((y) - 1)) / (y))
+#define VNC_DIRTY_WORDS (divideup(VNC_MAX_WIDTH, (16 * BITS_PER_LONG)))
 
 #define VNC_STAT_RECT  64
 #define VNC_STAT_COLS (VNC_MAX_WIDTH / VNC_STAT_RECT)
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Qemu-devel] Re: [PATCH 3/3] correct VNC_DIRTY_WORDS on 64 bit machine
  2011-03-03  2:44 ` [Qemu-devel] [PATCH 3/3] correct VNC_DIRTY_WORDS on 64 bit machine Wen Congyang
@ 2011-03-03  6:41   ` Corentin Chary
  2011-03-03  6:42     ` Wen Congyang
  2011-03-03  6:49     ` [Qemu-devel] [PATCH v2 " Wen Congyang
  0 siblings, 2 replies; 14+ messages in thread
From: Corentin Chary @ 2011-03-03  6:41 UTC (permalink / raw)
  To: Wen Congyang; +Cc: Anthony Liguori, qemu-devel

On Thu, Mar 3, 2011 at 3:44 AM, Wen Congyang <wency@cn.fujitsu.com> wrote:
> VNC_DIRTY_WORDS is wrong on 64 bit long machine.
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>
> ---
>  ui/vnc.h |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/ui/vnc.h b/ui/vnc.h
> index 8a1e7b9..239a7a7 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -81,7 +81,8 @@ typedef void VncSendHextileTile(VncState *vs,
>
>  #define VNC_MAX_WIDTH 2560
>  #define VNC_MAX_HEIGHT 2048
> -#define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))
> +#define divideup(x, y)  (((x) + ((y) - 1)) / (y))

osdep.h:#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))


-- 
Corentin Chary
http://xf.iksaif.net

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Qemu-devel] Re: [PATCH 3/3] correct VNC_DIRTY_WORDS on 64 bit machine
  2011-03-03  6:41   ` [Qemu-devel] " Corentin Chary
@ 2011-03-03  6:42     ` Wen Congyang
  2011-03-03  6:49     ` [Qemu-devel] [PATCH v2 " Wen Congyang
  1 sibling, 0 replies; 14+ messages in thread
From: Wen Congyang @ 2011-03-03  6:42 UTC (permalink / raw)
  To: Corentin Chary; +Cc: Anthony Liguori, qemu-devel

At 03/03/2011 02:41 PM, Corentin Chary Write:
> On Thu, Mar 3, 2011 at 3:44 AM, Wen Congyang <wency@cn.fujitsu.com> wrote:
>> VNC_DIRTY_WORDS is wrong on 64 bit long machine.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>
>> ---
>>  ui/vnc.h |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/ui/vnc.h b/ui/vnc.h
>> index 8a1e7b9..239a7a7 100644
>> --- a/ui/vnc.h
>> +++ b/ui/vnc.h
>> @@ -81,7 +81,8 @@ typedef void VncSendHextileTile(VncState *vs,
>>
>>  #define VNC_MAX_WIDTH 2560
>>  #define VNC_MAX_HEIGHT 2048
>> -#define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))
>> +#define divideup(x, y)  (((x) + ((y) - 1)) / (y))
> 
> osdep.h:#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))

Thank you for pointing out it. I will update my patch.

> 
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PATCH v2 3/3] correct VNC_DIRTY_WORDS on 64 bit machine
  2011-03-03  6:41   ` [Qemu-devel] " Corentin Chary
  2011-03-03  6:42     ` Wen Congyang
@ 2011-03-03  6:49     ` Wen Congyang
  1 sibling, 0 replies; 14+ messages in thread
From: Wen Congyang @ 2011-03-03  6:49 UTC (permalink / raw)
  To: Corentin Chary; +Cc: Anthony Liguori, qemu-devel

VNC_DIRTY_WORDS is wrong on 64 bit long machine.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>

---
 ui/vnc.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/ui/vnc.h b/ui/vnc.h
index 8a1e7b9..5fc54e5 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -81,7 +81,7 @@ typedef void VncSendHextileTile(VncState *vs,
 
 #define VNC_MAX_WIDTH 2560
 #define VNC_MAX_HEIGHT 2048
-#define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))
+#define VNC_DIRTY_WORDS (DIV_ROUND_UP(VNC_MAX_WIDTH, (16 * BITS_PER_LONG)))
 
 #define VNC_STAT_RECT  64
 #define VNC_STAT_COLS (VNC_MAX_WIDTH / VNC_STAT_RECT)
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2011-03-03  6:50 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-02  3:46 [Qemu-devel] [PATCH RESEND v2 1/2] fix vnc regression Wen Congyang
2011-03-02  3:58 ` [Qemu-devel] [PATCH RESEND 2/2] vnc: Fix heap corruption Wen Congyang
2011-03-02 10:57   ` [Qemu-devel] " Corentin Chary
2011-03-02 18:36     ` Stefan Weil
2011-03-02 18:47       ` Peter Maydell
2011-03-02 22:01         ` Stefan Weil
2011-03-02 22:27           ` Stefan Weil
2011-03-03  1:37             ` Wen Congyang
2011-03-02 22:40           ` Peter Maydell
2011-03-02 10:56 ` [Qemu-devel] Re: [PATCH RESEND v2 1/2] fix vnc regression Corentin Chary
2011-03-03  2:44 ` [Qemu-devel] [PATCH 3/3] correct VNC_DIRTY_WORDS on 64 bit machine Wen Congyang
2011-03-03  6:41   ` [Qemu-devel] " Corentin Chary
2011-03-03  6:42     ` Wen Congyang
2011-03-03  6:49     ` [Qemu-devel] [PATCH v2 " Wen Congyang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).