[Qemu-devel] [PULL for-2.1 0/2] vnc: fix two vnc update issues.

[Qemu-devel] [PULL for-2.1 0/2] vnc: fix two vnc update issues.

[Gerd Hoffmann]

  Hi,

Here are two vnc update bugfixes, found by code review while hunting
down a issue with vnc updates not being sent to the client.  The
original issue has not been root-caused yet, it is also not clear
whenever qemu is at fault at all or whenever the vnc updates are
stuck somewhere else (kernel network stack).  The bugs found are
for real nevertheless, and here are the fixes.

please pull,
  Gerd

The following changes since commit f368c33d5ab09dd5656924185cd975b11838cd25:

  Update version for v2.1.0-rc3 release (2014-07-22 18:17:03 +0100)

are available in the git repository at:

  git://git.kraxel.org/qemu tags/pull-vnc-20140724-1

for you to fetch changes up to 832932a6f17983a3167ae9da6fe54a245a30758e:

  vnc update fix (2014-07-24 10:14:34 +0200)

----------------------------------------------------------------
vnc: fix two vnc update issues.

----------------------------------------------------------------
Gerd Hoffmann (1):
      vnc update fix

Stephan Kulow (1):
      fix full frame updates for VNC clients

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

[Qemu-devel] [PULL for-2.1 1/2] fix full frame updates for VNC clients

[Gerd Hoffmann]

From: Stephan Kulow <coolo@suse.de>

If the client asks for !incremental frame updates, it has lost its content
so dirty doesn't matter - it has to see the full frame, so setting force_update

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Peter Lieven <pl@kamp.de>
---
 ui/vnc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ui/vnc.c b/ui/vnc.c
index 548588a..06d6ca0 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1878,6 +1878,7 @@ static void framebuffer_update_request(VncState *vs, int incremental,
         return;
     }
 
+    vs->force_update = 1;
     vnc_set_area_dirty(vs->dirty, width, height, x, y, w, h);
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PULL for-2.1 1/2] fix full frame updates for VNC clients

[Andreas Färber]

Stephan,

Am 24.07.2014 10:28, schrieb Gerd Hoffmann:
> From: Stephan Kulow <coolo@suse.de>
> 
> If the client asks for !incremental frame updates, it has lost its content
> so dirty doesn't matter - it has to see the full frame, so setting force_update
> 

Can you please sign off your (trivial) patch via reply?

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Peter Lieven <pl@kamp.de>
> ---

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PULL for-2.1 1/2] fix full frame updates for VNC clients

[Stephan Kulow]

On 24.07.2014 14:30, Andreas Färber wrote:
> Stephan,
> 
> Am 24.07.2014 10:28, schrieb Gerd Hoffmann:
>> From: Stephan Kulow <coolo@suse.de>
>>
>> If the client asks for !incremental frame updates, it has lost its content
>> so dirty doesn't matter - it has to see the full frame, so setting force_update
>>
> 
> Can you please sign off your (trivial) patch via reply?

You mean confirming it was my patch? Yes, it was.

Greetings, Stephan

Re: [Qemu-devel] [PULL for-2.1 1/2] fix full frame updates for VNC clients

[Andreas Färber]

Am 24.07.2014 15:45, schrieb Stephan Kulow:
> On 24.07.2014 14:30, Andreas Färber wrote:
>> Am 24.07.2014 10:28, schrieb Gerd Hoffmann:
>>> From: Stephan Kulow <coolo@suse.de>
>>>
>>> If the client asks for !incremental frame updates, it has lost its content
>>> so dirty doesn't matter - it has to see the full frame, so setting force_update
>>>
>>
>> Can you please sign off your (trivial) patch via reply?
> 
> You mean confirming it was my patch? Yes, it was.

Yes, in the sense of written-by-you - that's expected to be indicated
the kernel way through a "Signed-off-by: Your name <your@email>" line,
usually automated via git commit -s.

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PULL for-2.1 2/2] vnc update fix

[Gerd Hoffmann]

We need to remember has_updates for each vnc client.  Otherwise it might
happen that vnc_update_client(has_dirty=1) takes the first exit due to
output buffers not being flushed yet and subsequent calls with
has_dirty=0 take the second exit, wrongly assuming there is nothing to
do because the work defered in the first call is ignored.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Peter Lieven <pl@kamp.de>
---
 ui/vnc.c | 4 +++-
 ui/vnc.h | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 06d6ca0..f8d9b7d 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -887,6 +887,7 @@ static int find_and_clear_dirty_height(struct VncState *vs,
 
 static int vnc_update_client(VncState *vs, int has_dirty, bool sync)
 {
+    vs->has_dirty += has_dirty;
     if (vs->need_update && vs->csock != -1) {
         VncDisplay *vd = vs->vd;
         VncJob *job;
@@ -898,7 +899,7 @@ static int vnc_update_client(VncState *vs, int has_dirty, bool sync)
             /* kernel send buffers are full -> drop frames to throttle */
             return 0;
 
-        if (!has_dirty && !vs->audio_cap && !vs->force_update)
+        if (!vs->has_dirty && !vs->audio_cap && !vs->force_update)
             return 0;
 
         /*
@@ -941,6 +942,7 @@ static int vnc_update_client(VncState *vs, int has_dirty, bool sync)
             vnc_jobs_join(vs);
         }
         vs->force_update = 0;
+        vs->has_dirty = 0;
         return n;
     }
 
diff --git a/ui/vnc.h b/ui/vnc.h
index 8f582fd..334de9d 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -263,6 +263,7 @@ struct VncState
     VncDisplay *vd;
     int need_update;
     int force_update;
+    int has_dirty;
     uint32_t features;
     int absolute;
     int last_x;
-- 
1.8.3.1

Re: [Qemu-devel] [PULL for-2.1 0/2] vnc: fix two vnc update issues.

[Peter Maydell]

On 24 July 2014 09:28, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
> Here are two vnc update bugfixes, found by code review while hunting
> down a issue with vnc updates not being sent to the client.  The
> original issue has not been root-caused yet, it is also not clear
> whenever qemu is at fault at all or whenever the vnc updates are
> stuck somewhere else (kernel network stack).  The bugs found are
> for real nevertheless, and here are the fixes.
>
> please pull,
>   Gerd
>
> The following changes since commit f368c33d5ab09dd5656924185cd975b11838cd25:
>
>   Update version for v2.1.0-rc3 release (2014-07-22 18:17:03 +0100)
>
> are available in the git repository at:
>
>   git://git.kraxel.org/qemu tags/pull-vnc-20140724-1
>
> for you to fetch changes up to 832932a6f17983a3167ae9da6fe54a245a30758e:
>
>   vnc update fix (2014-07-24 10:14:34 +0200)
>
> ----------------------------------------------------------------
> vnc: fix two vnc update issues.
>
> ----------------------------------------------------------------
> Gerd Hoffmann (1):
>       vnc update fix
>
> Stephan Kulow (1):
>       fix full frame updates for VNC clients
>
>  ui/vnc.c | 5 ++++-
>  ui/vnc.h | 1 +
>  2 files changed, 5 insertions(+), 1 deletion(-)

So are these *really* release critical bugs, if they've been
only found in code review? We're really close to release now
and so my preference is not to include changes unless they're
really necessary...

thanks
-- PMM

Re: [Qemu-devel] [PULL for-2.1 0/2] vnc: fix two vnc update issues.

[Andreas Färber]

Am 24.07.2014 13:48, schrieb Peter Maydell:
> On 24 July 2014 09:28, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>   Hi,
>>
>> Here are two vnc update bugfixes, found by code review while hunting
>> down a issue with vnc updates not being sent to the client.  The
>> original issue has not been root-caused yet, it is also not clear
>> whenever qemu is at fault at all or whenever the vnc updates are
>> stuck somewhere else (kernel network stack).  The bugs found are
>> for real nevertheless, and here are the fixes.
>>
>> please pull,
>>   Gerd
>>
>> The following changes since commit f368c33d5ab09dd5656924185cd975b11838cd25:
>>
>>   Update version for v2.1.0-rc3 release (2014-07-22 18:17:03 +0100)
>>
>> are available in the git repository at:
>>
>>   git://git.kraxel.org/qemu tags/pull-vnc-20140724-1
>>
>> for you to fetch changes up to 832932a6f17983a3167ae9da6fe54a245a30758e:
>>
>>   vnc update fix (2014-07-24 10:14:34 +0200)
>>
>> ----------------------------------------------------------------
>> vnc: fix two vnc update issues.
>>
>> ----------------------------------------------------------------
>> Gerd Hoffmann (1):
>>       vnc update fix
>>
>> Stephan Kulow (1):
>>       fix full frame updates for VNC clients
>>
>>  ui/vnc.c | 5 ++++-
>>  ui/vnc.h | 1 +
>>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> So are these *really* release critical bugs, if they've been
> only found in code review? We're really close to release now
> and so my preference is not to include changes unless they're
> really necessary...

These are fixing openQA breakage (os-autoinst),

https://bugzilla.novell.com/show_bug.cgi?id=888142

so +1 to include them if Gerd is confident they don't regress otherwise.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PULL for-2.1 0/2] vnc: fix two vnc update issues.

[Gerd Hoffmann]

  Hi,

> > So are these *really* release critical bugs, if they've been
> > only found in code review? We're really close to release now
> > and so my preference is not to include changes unless they're
> > really necessary...
> 
> These are fixing openQA breakage (os-autoinst),

In more detail:

Stephan's patch fixes a rather serve violation of the vnc protocol.  If
vnc clients ask for a complete framebuffer update they may not get it.
The complete display is tagged dirty, but nothing is sent out until
something changes on the screen, thereby triggering processing of all
dirty display regions.  Which may be never.

The other patch fixes a simliar situation.  Screen updates might become
stuck in case vnc delays processing due to output buffers being filled.
Again other screen updates will trigger dirty processing and un-stuck
the updates.

That kind of bug you usually don't notice as normal user.  You'll wiggle
the mouse and the mouse pointer update will make vnc flush things.  Or,
if in text mode, the friendly blinking cursor causes regular vnc screen
update activity.  Thats why it didn't trip up normal users.

Scrips are much more likely to hit it as they don't do random mouse
activity.  But even openQA doesn't hit it on every run, but only now and
then.

Also asking for a complete framebuffer update isn't something desktop
vnc clients usually do (after initial connect).  openQA does it though.

cheers,
  Gerd

Re: [Qemu-devel] [PULL for-2.1 0/2] vnc: fix two vnc update issues.

[Peter Maydell]

On 24 July 2014 15:10, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
>> > So are these *really* release critical bugs, if they've been
>> > only found in code review? We're really close to release now
>> > and so my preference is not to include changes unless they're
>> > really necessary...
>>
>> These are fixing openQA breakage (os-autoinst),
>
> In more detail:
> [snip]

Thanks for the clarification; these do seem worth putting in 2.1.
I'll need to get you to respin with the missing signed-off-by
that Andreas pointed out, though.

-- PMM

Re: [Qemu-devel] [PULL for-2.1 0/2] vnc: fix two vnc update issues.

[Gerd Hoffmann]

On Do, 2014-07-24 at 15:22 +0100, Peter Maydell wrote:
> On 24 July 2014 15:10, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >   Hi,
> >
> >> > So are these *really* release critical bugs, if they've been
> >> > only found in code review? We're really close to release now
> >> > and so my preference is not to include changes unless they're
> >> > really necessary...
> >>
> >> These are fixing openQA breakage (os-autoinst),
> >
> > In more detail:
> > [snip]
> 
> Thanks for the clarification; these do seem worth putting in 2.1.
> I'll need to get you to respin with the missing signed-off-by
> that Andreas pointed out, though.

Done: git://git.kraxel.org/qemu tags/pull-vnc-20140725-1

cheers,
  Gerd

Re: [Qemu-devel] [PULL for-2.1 0/2] vnc: fix two vnc update issues.

[Peter Maydell]

On 25 July 2014 08:45, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Do, 2014-07-24 at 15:22 +0100, Peter Maydell wrote:
>> On 24 July 2014 15:10, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> >   Hi,
>> >
>> >> > So are these *really* release critical bugs, if they've been
>> >> > only found in code review? We're really close to release now
>> >> > and so my preference is not to include changes unless they're
>> >> > really necessary...
>> >>
>> >> These are fixing openQA breakage (os-autoinst),
>> >
>> > In more detail:
>> > [snip]
>>
>> Thanks for the clarification; these do seem worth putting in 2.1.
>> I'll need to get you to respin with the missing signed-off-by
>> that Andreas pointed out, though.
>
> Done: git://git.kraxel.org/qemu tags/pull-vnc-20140725-1

Applied, thanks.

-- PMM