[Qemu-devel] [PATCH] vnc: tight: Fix crash after 2GB of output

[Qemu-devel] [PATCH] vnc: tight: Fix crash after 2GB of output

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

If one leaves a VNC session with tight compression running for long
enough, Qemu crashes.  This is because of the computation

    bytes = zstream->total_out - previous_out;

in tight_compress_data, where zstream->total_out is a uLong but
previous_out is an int.  As soon as zstream->total_out gets past
INT_MAX (ie 2GB), previous_out becomes negative and therefore the
result of the subtraction, bytes, becomes a huge positive number that
causes havoc for obvious reasons when passed as a length to
vnc_write().

The fix for this is simple: keep previous_out as a uLong too, which
avoids any problems with sign conversion or truncation.

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 ui/vnc-enc-tight.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
index af45edd..59ec0e3 100644
--- a/ui/vnc-enc-tight.c
+++ b/ui/vnc-enc-tight.c
@@ -829,7 +829,7 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes,
                                int level, int strategy)
 {
     z_streamp zstream = &vs->tight.stream[stream_id];
-    int previous_out;
+    uLong previous_out;
 
     if (bytes < VNC_TIGHT_MIN_TO_COMPRESS) {
         vnc_write(vs, vs->tight.tight.buffer, vs->tight.tight.offset);

Re: [Qemu-devel] [PATCH] vnc: tight: Fix crash after 2GB of output

[Michael Tokarev]

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

04.03.2011 03:57, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
>
> If one leaves a VNC session with tight compression running for long
> enough, Qemu crashes.  This is because of the computation
>
>     bytes = zstream->total_out - previous_out;
>
> in tight_compress_data, where zstream->total_out is a uLong but
> previous_out is an int.  As soon as zstream->total_out gets past
> INT_MAX (ie 2GB), previous_out becomes negative and therefore the
> result of the subtraction, bytes, becomes a huge positive number that
> causes havoc for obvious reasons when passed as a length to
> vnc_write().

Excellent work, Roland!  No doubt I wasn't able to hit this bug
locally - I tried many things, but I never waited long enough
to hit this 2Gb border ;)

> The fix for this is simple: keep previous_out as a uLong too, which
> avoids any problems with sign conversion or truncation.

This looks wrong to me.  On 32bit x86 uLong is 32bits.  Yes
it's unsigned there, but it's still 32bits.  And sure thing
it _will_ overflow again, not at 2Gb but now at 4Gb, and the
next total_out will start from zero again, so that now bytes
will be a large integer again because "next" (new total_out)
will be less than "current" (previous_out).

If the whole total_out is actually needed, that counter should
be kept separate and added to when necessary.  But this
total_out should be reset to avoid letting it large values
in the first place.

total_out isn't used by zlib internally, so if the resulting
"total" counter is not needed in qemu, we can just zero-out
the total_out in this function before calling zlib, and
use the resulting value directly as "bytes", without
saving its previous value in previous_out.  Something like
the attached patch does.

By the way, the same thing is used in ui/vnc-enc-zlib.c too,
and it looks like that place has the same issue as well.

Cc'ng to Corentin Chary since he made lots of work in that
area recently.

Thanks!

> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---
>  ui/vnc-enc-tight.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
> index af45edd..59ec0e3 100644
> --- a/ui/vnc-enc-tight.c
> +++ b/ui/vnc-enc-tight.c
> @@ -829,7 +829,7 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes,
>                                 int level, int strategy)
>  {
>      z_streamp zstream = &vs->tight.stream[stream_id];
> -    int previous_out;
> +    uLong previous_out;
>
>      if (bytes < VNC_TIGHT_MIN_TO_COMPRESS) {
>          vnc_write(vs, vs->tight.tight.buffer, vs->tight.tight.offset);
>



[-- Attachment #2: vnc-tight-2g-overflow.diff --]
[-- Type: text/x-patch, Size: 1653 bytes --]

fix overflow after 2Gb of output in ui/vnc-enc-tight.c

When amount of compressed data is more than 2Gb we will hit
integer overflow (or when it's unsigned, the border is 4Gb).
We don't use zstream->total_out for anything, so instead of
remembering its previois value and compare with hext one,
we can just reset it on entry and use its resulting value
as the amount of bytes to deal with.

diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
index 2522936..e1843cb 100644
--- a/ui/vnc-enc-tight.c
+++ b/ui/vnc-enc-tight.c
@@ -849,7 +849,6 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes,
                                int level, int strategy)
 {
     z_streamp zstream = &vs->tight.stream[stream_id];
-    int previous_out;
 
     if (bytes < VNC_TIGHT_MIN_TO_COMPRESS) {
         vnc_write(vs, vs->tight.tight.buffer, vs->tight.tight.offset);
@@ -869,7 +868,7 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes,
     zstream->next_out = vs->tight.zlib.buffer + vs->tight.zlib.offset;
     zstream->avail_out = vs->tight.zlib.capacity - vs->tight.zlib.offset;
     zstream->data_type = Z_BINARY;
-    previous_out = zstream->total_out;
+    zstream->total_out = 0;
 
     /* start encoding */
     if (deflate(zstream, Z_SYNC_FLUSH) != Z_OK) {
@@ -878,7 +877,7 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes,
     }
 
     vs->tight.zlib.offset = vs->tight.zlib.capacity - zstream->avail_out;
-    bytes = zstream->total_out - previous_out;
+    bytes = zstream->total_out;
 
     tight_send_compact_size(vs, bytes);
     vnc_write(vs, vs->tight.zlib.buffer, bytes);

Re: [Qemu-devel] [PATCH] vnc: tight: Fix crash after 2GB of output

[Michael Tokarev]

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

[And sure thing I forgot to Cc  Corentin Chary.  ENOCOFFEE.
 Please excuse me for the double post]

04.03.2011 03:57, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
>
> If one leaves a VNC session with tight compression running for long
> enough, Qemu crashes.  This is because of the computation
>
>     bytes = zstream->total_out - previous_out;
>
> in tight_compress_data, where zstream->total_out is a uLong but
> previous_out is an int.  As soon as zstream->total_out gets past
> INT_MAX (ie 2GB), previous_out becomes negative and therefore the
> result of the subtraction, bytes, becomes a huge positive number that
> causes havoc for obvious reasons when passed as a length to
> vnc_write().

Excellent work, Roland!  No doubt I wasn't able to hit this bug
locally - I tried many things, but I never waited long enough
to hit this 2Gb border ;)

> The fix for this is simple: keep previous_out as a uLong too, which
> avoids any problems with sign conversion or truncation.

This looks wrong to me.  On 32bit x86 uLong is 32bits.  Yes
it's unsigned there, but it's still 32bits.  And sure thing
it _will_ overflow again, not at 2Gb but now at 4Gb, and the
next total_out will start from zero again, so that now bytes
will be a large integer again because "next" (new total_out)
will be less than "current" (previous_out).

If the whole total_out is actually needed, that counter should
be kept separate and added to when necessary.  But this
total_out should be reset to avoid letting it large values
in the first place.

total_out isn't used by zlib internally, so if the resulting
"total" counter is not needed in qemu, we can just zero-out
the total_out in this function before calling zlib, and
use the resulting value directly as "bytes", without
saving its previous value in previous_out.  Something like
the attached patch does.

By the way, the same thing is used in ui/vnc-enc-zlib.c too,
and it looks like that place has the same issue as well.

Cc'ng to Corentin Chary since he made lots of work in that
area recently.

Thanks!

> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---
>  ui/vnc-enc-tight.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
> index af45edd..59ec0e3 100644
> --- a/ui/vnc-enc-tight.c
> +++ b/ui/vnc-enc-tight.c
> @@ -829,7 +829,7 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes,
>                                 int level, int strategy)
>  {
>      z_streamp zstream = &vs->tight.stream[stream_id];
> -    int previous_out;
> +    uLong previous_out;
>
>      if (bytes < VNC_TIGHT_MIN_TO_COMPRESS) {
>          vnc_write(vs, vs->tight.tight.buffer, vs->tight.tight.offset);
>




[-- Attachment #2: vnc-tight-2g-overflow.diff --]
[-- Type: text/x-patch, Size: 1654 bytes --]

fix overflow after 2Gb of output in ui/vnc-enc-tight.c

When amount of compressed data is more than 2Gb we will hit
integer overflow (or when it's unsigned, the border is 4Gb).
We don't use zstream->total_out for anything, so instead of
remembering its previois value and compare with hext one,
we can just reset it on entry and use its resulting value
as the amount of bytes to deal with.

diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
index 2522936..e1843cb 100644
--- a/ui/vnc-enc-tight.c
+++ b/ui/vnc-enc-tight.c
@@ -849,7 +849,6 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes,
                                int level, int strategy)
 {
     z_streamp zstream = &vs->tight.stream[stream_id];
-    int previous_out;
 
     if (bytes < VNC_TIGHT_MIN_TO_COMPRESS) {
         vnc_write(vs, vs->tight.tight.buffer, vs->tight.tight.offset);
@@ -869,7 +868,7 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes,
     zstream->next_out = vs->tight.zlib.buffer + vs->tight.zlib.offset;
     zstream->avail_out = vs->tight.zlib.capacity - vs->tight.zlib.offset;
     zstream->data_type = Z_BINARY;
-    previous_out = zstream->total_out;
+    zstream->total_out = 0;
 
     /* start encoding */
     if (deflate(zstream, Z_SYNC_FLUSH) != Z_OK) {
@@ -878,7 +877,7 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes,
     }
 
     vs->tight.zlib.offset = vs->tight.zlib.capacity - zstream->avail_out;
-    bytes = zstream->total_out - previous_out;
+    bytes = zstream->total_out;
 
     tight_send_compact_size(vs, bytes);
     vnc_write(vs, vs->tight.zlib.buffer, bytes);

Re: [Qemu-devel] [PATCH] vnc: tight: Fix crash after 2GB of output

[Corentin Chary]

>>
>>     bytes = zstream->total_out - previous_out;

Good catch

> total_out isn't used by zlib internally, so if the resulting
> "total" counter is not needed in qemu, we can just zero-out
> the total_out in this function before calling zlib, and
> use the resulting value directly as "bytes", without
> saving its previous value in previous_out.  Something like
> the attached patch does.

If you're certain that total_out is not used by zlib, could you also
send a patch for zlib encoding please ? (vnc-enc-zlib.c)
Thanks,

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

Re: [Qemu-devel] [PATCH] vnc: tight: Fix crash after 2GB of output

[Michael Tokarev]

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

04.03.2011 11:56, Corentin Chary wrote:
>>>
>>>     bytes = zstream->total_out - previous_out;
> 
> Good catch
> 
>> total_out isn't used by zlib internally, so if the resulting
>> "total" counter is not needed in qemu, we can just zero-out
>> the total_out in this function before calling zlib, and
>> use the resulting value directly as "bytes", without
>> saving its previous value in previous_out.  Something like
>> the attached patch does.
> 
> If you're certain that total_out is not used by zlib, could you also
> send a patch for zlib encoding please ? (vnc-enc-zlib.c)
> Thanks,

Yes, I noticed this too (the same code is in enc-zlib), and mentioned
this in my previous email.

The attached slightly different patch fixes both places and fixes
them for good (hopefully anyway).  Runtime-tested for the tight
case, but honestly, I didn't wait for 2G of output ;)

Thanks!

/mjt

[-- Attachment #2: fix-vnc-zlib-overflow.diff --]
[-- Type: text/x-patch, Size: 2847 bytes --]

fix 2Gb integer overflow in in VNC tight and zlib encodings

As found by Roland Dreier <roland@purestorage.com> (excellent
catch!), when amount of VNC compressed data produced by zlib
and sent to client exceeds 2Gb, integer overflow occurs because
currently, we calculate amount of data produced at each step by
comparing saved total_out with new total_out, and total_out is
something which grows without bounds.  Compare it with previous
avail_out instead of total_out, and leave total_out alone.

The same code is used in vnc-enc-tight.c and vnc-enc-zlib.c,
so fix both cases.

There, there's no actual need to save previous_out value, since
capacity-offset (which is how that value is calculated) stays
the same so it can be recalculated again after call to deflate(),
but whole thing becomes less readable this way.

Reported-by: Roland Dreier <roland@purestorage.com>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>

diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
index 2522936..87fdf35 100644
--- a/ui/vnc-enc-tight.c
+++ b/ui/vnc-enc-tight.c
@@ -868,8 +868,8 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes,
     zstream->avail_in = vs->tight.tight.offset;
     zstream->next_out = vs->tight.zlib.buffer + vs->tight.zlib.offset;
     zstream->avail_out = vs->tight.zlib.capacity - vs->tight.zlib.offset;
+    previous_out = zstream->avail_out;
     zstream->data_type = Z_BINARY;
-    previous_out = zstream->total_out;
 
     /* start encoding */
     if (deflate(zstream, Z_SYNC_FLUSH) != Z_OK) {
@@ -878,7 +878,8 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes,
     }
 
     vs->tight.zlib.offset = vs->tight.zlib.capacity - zstream->avail_out;
-    bytes = zstream->total_out - previous_out;
+    /* ...how much data has actually been produced by deflate() */
+    bytes = previous_out - zstream->avail_out;
 
     tight_send_compact_size(vs, bytes);
     vnc_write(vs, vs->tight.zlib.buffer, bytes);
diff --git a/ui/vnc-enc-zlib.c b/ui/vnc-enc-zlib.c
index 3c6e6ab..e32e4cd 100644
--- a/ui/vnc-enc-zlib.c
+++ b/ui/vnc-enc-zlib.c
@@ -103,8 +103,8 @@ static int vnc_zlib_stop(VncState *vs)
     zstream->avail_in = vs->zlib.zlib.offset;
     zstream->next_out = vs->output.buffer + vs->output.offset;
     zstream->avail_out = vs->output.capacity - vs->output.offset;
+    previous_out = zstream->avail_out;
     zstream->data_type = Z_BINARY;
-    previous_out = zstream->total_out;
 
     // start encoding
     if (deflate(zstream, Z_SYNC_FLUSH) != Z_OK) {
@@ -113,7 +113,7 @@ static int vnc_zlib_stop(VncState *vs)
     }
 
     vs->output.offset = vs->output.capacity - zstream->avail_out;
-    return zstream->total_out - previous_out;
+    return previous_out - zstream->avail_out;
 }
 
 int vnc_zlib_send_framebuffer_update(VncState *vs, int x, int y, int w, int h)

Re: [Qemu-devel] [PATCH] vnc: tight: Fix crash after 2GB of output

[Roland Dreier]

On Thu, Mar 3, 2011 at 11:34 PM, Michael Tokarev <mjt@tls.msk.ru> wrote:
>> The fix for this is simple: keep previous_out as a uLong too, which
>> avoids any problems with sign conversion or truncation.
>
> This looks wrong to me.  On 32bit x86 uLong is 32bits.  Yes
> it's unsigned there, but it's still 32bits.  And sure thing
> it _will_ overflow again, not at 2Gb but now at 4Gb, and the
> next total_out will start from zero again, so that now bytes
> will be a large integer again because "next" (new total_out)
> will be less than "current" (previous_out).

Actually there is no problem with overflow of unsigned long.
The C standard says that unsigned arithmetic is simply done
modulo the size of the integer, so when total_out reaches
4GB, things will just wrap around (and the difference
between "nearby" values will still be the correct, small
value).  For example, if previous were (4GB - 5) and
then total_out had 1000 added to it, total_out would
end up as 995, and total_out - previous would be 1000.

In other words I'm pretty sure my fix works on 32 bits too.

However your fix using avail_out is probably better anyway,
and your latest patch fixes both instances of the bug, so
we might as well go with that.

Thanks,
 - R.

Re: [Qemu-devel] [PATCH] vnc: tight: Fix crash after 2GB of output

[Roland Dreier]

On Fri, Mar 4, 2011 at 8:59 AM, Roland Dreier <roland@kernel.org> wrote:
> Actually there is no problem with overflow of unsigned long.
> The C standard says that unsigned arithmetic is simply done
> modulo the size of the integer, so when total_out reaches
> 4GB, things will just wrap around (and the difference
> between "nearby" values will still be the correct, small
> value).  For example, if previous were (4GB - 5) and
> then total_out had 1000 added to it, total_out would
> end up as 995, and total_out - previous would be 1000.

Additionally, thinking about this further, I realize that
amusingly enough, the old code also works on 32-bit:
the bug occurred because when we put a value above
2GB in a (32-bit) int, it became a signed quantity,
which then became a gigantic value when promoted
back to an unsigned (64-bit) long, which causes the
subtraction to get the wrong value.  On 32-bit, the
promotion from signed 32-bit to unsigned 32-bit
doesn't lead to the wrong difference.

 - R.

Re: [Qemu-devel] [PATCH] vnc: tight: Fix crash after 2GB of output

[Corentin Chary]

On Fri, Mar 4, 2011 at 12:46 PM, Michael Tokarev <mjt@tls.msk.ru> wrote:
> 04.03.2011 11:56, Corentin Chary wrote:
>>>>
>>>>     bytes = zstream->total_out - previous_out;
>>
>> Good catch
>>
>>> total_out isn't used by zlib internally, so if the resulting
>>> "total" counter is not needed in qemu, we can just zero-out
>>> the total_out in this function before calling zlib, and
>>> use the resulting value directly as "bytes", without
>>> saving its previous value in previous_out.  Something like
>>> the attached patch does.
>>
>> If you're certain that total_out is not used by zlib, could you also
>> send a patch for zlib encoding please ? (vnc-enc-zlib.c)
>> Thanks,
>
> Yes, I noticed this too (the same code is in enc-zlib), and mentioned
> this in my previous email.
>
> The attached slightly different patch fixes both places and fixes
> them for good (hopefully anyway).  Runtime-tested for the tight
> case, but honestly, I didn't wait for 2G of output ;)
>
> Thanks!
>
> /mjt
>

Could you re-send it inline (not as an attachment), and CC Anthony ?
Thanks,

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

[Qemu-devel] [PATCH][STABLE-0.14] vnc: tight: Fix crash after 2GB of output

[Michael Tokarev]

05.03.2011 00:08, Corentin Chary wrote:
> On Fri, Mar 4, 2011 at 12:46 PM, Michael Tokarev <mjt@tls.msk.ru> wrote:
[]
>> The attached slightly different patch fixes both places and fixes
>> them for good (hopefully anyway).  Runtime-tested for the tight
>> case, but honestly, I didn't wait for 2G of output ;)
>>
> Could you re-send it inline (not as an attachment), and CC Anthony ?

What's wrong with using an attachment?  The whole email can be
fed into patch(1) (or git-am, whatever) and either will do the
job.  But here it goes, anyway, with one possible caveat - I'm
not sure anymore it will apply, since now I used cut-n-paste.

/mjt

----

fix 2Gb integer overflow in in VNC tight and zlib encodings

As found by Roland Dreier <roland@purestorage.com> (excellent
catch!), when amount of VNC compressed data produced by zlib
and sent to client exceeds 2Gb, integer overflow occurs because
currently, we calculate amount of data produced at each step by
comparing saved total_out with new total_out, and total_out is
something which grows without bounds.  Compare it with previous
avail_out instead of total_out, and leave total_out alone.

The same code is used in vnc-enc-tight.c and vnc-enc-zlib.c,
so fix both cases.

There, there's no actual need to save previous_out value, since
capacity-offset (which is how that value is calculated) stays
the same so it can be recalculated again after call to deflate(),
but whole thing becomes less readable this way.

Reported-by: Roland Dreier <roland@purestorage.com>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>

diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
index 2522936..87fdf35 100644
--- a/ui/vnc-enc-tight.c
+++ b/ui/vnc-enc-tight.c
@@ -868,8 +868,8 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes,
     zstream->avail_in = vs->tight.tight.offset;
     zstream->next_out = vs->tight.zlib.buffer + vs->tight.zlib.offset;
     zstream->avail_out = vs->tight.zlib.capacity - vs->tight.zlib.offset;
+    previous_out = zstream->avail_out;
     zstream->data_type = Z_BINARY;
-    previous_out = zstream->total_out;

     /* start encoding */
     if (deflate(zstream, Z_SYNC_FLUSH) != Z_OK) {
@@ -878,7 +878,8 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes,
     }

     vs->tight.zlib.offset = vs->tight.zlib.capacity - zstream->avail_out;
-    bytes = zstream->total_out - previous_out;
+    /* ...how much data has actually been produced by deflate() */
+    bytes = previous_out - zstream->avail_out;

     tight_send_compact_size(vs, bytes);
     vnc_write(vs, vs->tight.zlib.buffer, bytes);
diff --git a/ui/vnc-enc-zlib.c b/ui/vnc-enc-zlib.c
index 3c6e6ab..e32e4cd 100644
--- a/ui/vnc-enc-zlib.c
+++ b/ui/vnc-enc-zlib.c
@@ -103,8 +103,8 @@ static int vnc_zlib_stop(VncState *vs)
     zstream->avail_in = vs->zlib.zlib.offset;
     zstream->next_out = vs->output.buffer + vs->output.offset;
     zstream->avail_out = vs->output.capacity - vs->output.offset;
+    previous_out = zstream->avail_out;
     zstream->data_type = Z_BINARY;
-    previous_out = zstream->total_out;

     // start encoding
     if (deflate(zstream, Z_SYNC_FLUSH) != Z_OK) {
@@ -113,7 +113,7 @@ static int vnc_zlib_stop(VncState *vs)
     }

     vs->output.offset = vs->output.capacity - zstream->avail_out;
-    return zstream->total_out - previous_out;
+    return previous_out - zstream->avail_out;
 }

 int vnc_zlib_send_framebuffer_update(VncState *vs, int x, int y, int w, int h)

[Qemu-devel] Re: [PATCH][STABLE-0.14] vnc: tight: Fix crash after 2GB of output

[Corentin Chary]

On Sat, Mar 5, 2011 at 6:56 AM, Michael Tokarev <mjt@tls.msk.ru> wrote:
> 05.03.2011 00:08, Corentin Chary wrote:
>> On Fri, Mar 4, 2011 at 12:46 PM, Michael Tokarev <mjt@tls.msk.ru> wrote:
> []
>>> The attached slightly different patch fixes both places and fixes
>>> them for good (hopefully anyway).  Runtime-tested for the tight
>>> case, but honestly, I didn't wait for 2G of output ;)
>>>
>> Could you re-send it inline (not as an attachment), and CC Anthony ?
>
> What's wrong with using an attachment?  The whole email can be
> fed into patch(1) (or git-am, whatever) and either will do the
> job.  But here it goes, anyway, with one possible caveat - I'm
> not sure anymore it will apply, since now I used cut-n-paste.

It's easier for reviewer because it allow people to view and comment
the code directly in a mail client.
The prefered way to send patch is `git format-patch` and `git
send-email`. If you use these two tools, you can be sure that all will
be ok :).

Re: [Qemu-devel] Re: [PATCH][STABLE-0.14] vnc: tight: Fix crash after 2GB of output

[Michael Tokarev]

05.03.2011 11:33, Corentin Chary wrote:
> On Sat, Mar 5, 2011 at 6:56 AM, Michael Tokarev <mjt@tls.msk.ru> wrote:
>> 05.03.2011 00:08, Corentin Chary wrote:

>> What's wrong with using an attachment?  The whole email can be
>> fed into patch(1) (or git-am, whatever) and either will do the
>> job.  But here it goes, anyway, with one possible caveat - I'm
>> not sure anymore it will apply, since now I used cut-n-paste.

> It's easier for reviewer because it allow people to view and comment
> the code directly in a mail client.

I can usually easily review patches sent as text/plain attachments
directly in my client.  But granted, I tried to reply to my own
email with attachment and Thunderbird does not show the patch text, --
it usually does.  I'll investigate it.  But the patch is shown
inline as the rest of my email, so it's still easy to review it
(but not comment on individual parts).

In any way, this patch is too simple to comment on it.

> The prefered way to send patch is `git format-patch` and `git
> send-email`. If you use these two tools, you can be sure that all will
> be ok :).

Now please show me how I can use these tools when replying to
a discussion and keeping other comments and thread flow.  Yes
sure this can be done, I can cut-n-paste all sorts of original
message into the editor executed by git send-email, or even into
command-line of git format-patch, but this is just too much work
compared with regular reply in usual mail client and attaching
a single file.  The latter is much more productive and as easy
to review and apply, the only difference is - as it turns out -
in-line commenting, but I'll check this separately.

What I miss in Thunderbird is a way to _insert_ content of a
file into the message, -- a combination of cut-n-paste and
attachment.  With sufficient care I can keep tabs after
cut-n-paste, but this is fragile.  If I can solve inline-
comments in attached patch that'll be ideal variant.

And I think that the reasons you stated above are not
sufficient to warrant re-sending this very patch as
you asked me to do... ;)

Thanks!

/mjt

Re: [Qemu-devel] Re: [PATCH][STABLE-0.14] vnc: tight: Fix crash after 2GB of output

[Corentin Chary]

> Now please show me how I can use these tools when replying to
> a discussion and keeping other comments and thread flow.  Yes
> sure this can be done, I can cut-n-paste all sorts of original
> message into the editor executed by git send-email, or even into
> command-line of git format-patch, but this is just too much work
> compared with regular reply in usual mail client and attaching
> a single file.  The latter is much more productive and as easy
> to review and apply, the only difference is - as it turns out -
> in-line commenting, but I'll check this separately.

Hi,

You can use "git send-email --in-reply-to=<messageid>" to reply to a thread.

> And I think that the reasons you stated above are not
> sufficient to warrant re-sending this very patch as
> you asked me to do... ;)

Do not see any thing personal here, I'm just quoting
http://wiki.qemu.org/Contribute/SubmitAPatch:
"Send patches inline so they are easy to reply to with review
comments. Do not put patches in attachments".

I mainly asked yo to re-send it  so Anthony has less chance to miss it
if he is cc and if the patch is inline :), that's all.

Thanks,

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