| Message ID | 1274186986-26878-2-git-send-email-corentincj@iksaif.net |
|---|---|
| State | New |
| Headers | show |
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
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.
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
> 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.
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;
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(-)