Patchwork [v2,01/10] vnc: refactor set_encodings

login
register
mail settings
Submitter Corentin Chary
Date May 18, 2010, 12:49 p.m.
Message ID <1274186986-26878-2-git-send-email-corentincj@iksaif.net>
Download mbox | patch
Permalink /patch/52876/
State New
Headers show

Comments

Corentin Chary - May 18, 2010, 12:49 p.m.
Create a new set_encoding() function to remove
duplicated code in set_encodings().

Signed-off-by: Corentin Chary <corentincj@iksaif.net>
---
 vnc.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)
Alexander Graf - May 18, 2010, 12:54 p.m.
Corentin Chary wrote:
> Create a new set_encoding() function to remove
> duplicated code in set_encodings().
>
> Signed-off-by: Corentin Chary <corentincj@iksaif.net>
>   

Acked-by: Alexander Graf <agraf@suse.de>

Keep in mind that this still means that the last requested compression
algorithm is used, which is reverse according to spec.


Alex
Corentin Chary - May 18, 2010, 6:09 p.m.
On Tue, May 18, 2010 at 2:54 PM, Alexander Graf <agraf@suse.de> wrote:
> Corentin Chary wrote:
>> Create a new set_encoding() function to remove
>> duplicated code in set_encodings().
>>
>> Signed-off-by: Corentin Chary <corentincj@iksaif.net>
>>
>
> Acked-by: Alexander Graf <agraf@suse.de>
>
> Keep in mind that this still means that the last requested compression
> algorithm is used, which is reverse according to spec.
>

I didn't notice that the loop was reversed, and that the right
encoding was selected *before* my patch.
I think the right thing to do is to reverse my previous patch and add
a comment at the begining of the loop.

I can also just make a patch to reverse the loop.
Alexander Graf - May 18, 2010, 6:18 p.m.
On 18.05.2010, at 20:09, Corentin Chary wrote:

> On Tue, May 18, 2010 at 2:54 PM, Alexander Graf <agraf@suse.de> wrote:
>> Corentin Chary wrote:
>>> Create a new set_encoding() function to remove
>>> duplicated code in set_encodings().
>>> 
>>> Signed-off-by: Corentin Chary <corentincj@iksaif.net>
>>> 
>> 
>> Acked-by: Alexander Graf <agraf@suse.de>
>> 
>> Keep in mind that this still means that the last requested compression
>> algorithm is used, which is reverse according to spec.
>> 
> 
> I didn't notice that the loop was reversed, and that the right
> encoding was selected *before* my patch.
> I think the right thing to do is to reverse my previous patch and add
> a comment at the begining of the loop.

I don't see your patch reversing the logic?

Alex
Corentin Chary - May 18, 2010, 7:23 p.m.
> I don't see your patch reversing the logic?
>
> Alex
>
>

Before my patch (not this one, but
http://git.qemu.org/qemu.git/commit/?id=14eb8b6829ad9dee7035de729e083844a425f274
), we looped from the last encoding to the first (for (i = n_encodings
- 1; i >= 0; i--)), so the code was ok.

Commit 14eb8b6829ad9dee7035de729e083844a425f274 was wrong because I
wanted `if (encoding == -1) encoding = enc`, not
`if (encoding != -1) encoding = enc`. With the current code the
encoding will never be set and will always be -1.

And this patch (refactor set_encoding) is also wrong, because it will
only set the first encoding (which is in fact the last, because we
start from the end of the array).

I believe that the right thing to do is to revert
14eb8b6829ad9dee7035de729e083844a425f274 and add some comments to
explain why we loop in reverse order.

Patch

diff --git a/vnc.c b/vnc.c
index 1f7ad73..a91c3a3 100644
--- a/vnc.c
+++ b/vnc.c
@@ -1587,6 +1587,13 @@  static void send_ext_audio_ack(VncState *vs)
     vnc_flush(vs);
 }
 
+static void set_encoding(VncState *vs, int encoding)
+{
+    if (vs->vnc_encoding == -1) {
+        vs->vnc_encoding = encoding;
+    }
+}
+
 static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
 {
     int i;
@@ -1603,24 +1610,18 @@  static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
         enc = encodings[i];
         switch (enc) {
         case VNC_ENCODING_RAW:
-            if (vs->vnc_encoding != -1) {
-                vs->vnc_encoding = enc;
-            }
+            set_encoding(vs, enc);
             break;
         case VNC_ENCODING_COPYRECT:
             vs->features |= VNC_FEATURE_COPYRECT_MASK;
             break;
         case VNC_ENCODING_HEXTILE:
             vs->features |= VNC_FEATURE_HEXTILE_MASK;
-            if (vs->vnc_encoding != -1) {
-                vs->vnc_encoding = enc;
-            }
+            set_encoding(vs, enc);
             break;
         case VNC_ENCODING_ZLIB:
             vs->features |= VNC_FEATURE_ZLIB_MASK;
-            if (vs->vnc_encoding != -1) {
-                vs->vnc_encoding = enc;
-            }
+            set_encoding(vs, enc);
             break;
         case VNC_ENCODING_DESKTOPRESIZE:
             vs->features |= VNC_FEATURE_RESIZE_MASK;