Re: [PULL 10/11] vnc: move initialization to framebuffer_update_request

[Laszlo Ersek]

On 01/21/21 22:22, Laszlo Ersek wrote:
> On 01/15/21 11:24, Gerd Hoffmann wrote:
>> qemu sends various state info like current cursor shape to newly connected
>> clients in response to a set_encoding message.  This is not correct according
>> to the rfb spec.  Send that information in response to a full (incremental=0)
>> framebuffer update request instead.  Also send the resize information
>> unconditionally, not only in case of an actual server-side change.
>>
>> This makes the qemu vnc server conform to the spec and allows clients to
>> request the complete vnc server state without reconnect.
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>> Message-id: 20210112134120.2031837-3-kraxel@redhat.com
>> ---
>>  ui/vnc.c | 11 ++++-------
>>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> This patch breaks QEMU for me.
>
> Bisection log below.
>
> (I started the bisection from commit facf7c60ee60 ("vl: initialize
> displays _after_ exiting preconfiguration", 2021-01-02), which is the
> fix for the previous display regression. I noticed the regression after
> pulling master today, at fef80ea073c4.)
>
>> git bisect start
>> # good: [facf7c60ee60aab7d73b204ee8c86b90fbc6b3db] vl: initialize displays 
>> _after_ exiting preconfiguration
>> git bisect good facf7c60ee60aab7d73b204ee8c86b90fbc6b3db
>> # bad: [fef80ea073c4862bc9eaddb6ddb0ed970b8ad7c4] Merge remote-tracking 
>> branch 'remotes/ericb/tags/pull-nbd-2021-01-20' into staging
>> git bisect bad fef80ea073c4862bc9eaddb6ddb0ed970b8ad7c4
>> # good: [88d4005b098427638d7551aa04ebde4fdd06835b] tcg: Use 
>> tcg_constant_{i32,i64,vec} with gvec expanders
>> git bisect good 88d4005b098427638d7551aa04ebde4fdd06835b
>> # bad: [1eaada8ae15f10f7a7f1e2505bd77dbb11a8be85] hw/riscv: sifive_u: Use 
>> SIFIVE_U_CPU for mc->default_cpu_type
>> git bisect bad 1eaada8ae15f10f7a7f1e2505bd77dbb11a8be85
>> # good: [c7a9ef75173f090616328d6870f71e8da2b6bd50] target/mips: Introduce 
>> decode tree bindings for MSA ASE
>> git bisect good c7a9ef75173f090616328d6870f71e8da2b6bd50
>> # bad: [7cb6b97300f0405b4c6856c49bdc33fa3265852f] Merge remote-tracking 
>> branch 'remotes/kraxel/tags/ui-20210115-pull-request' into staging
>> git bisect bad 7cb6b97300f0405b4c6856c49bdc33fa3265852f
>> # good: [eaca85763bcd94ddac3fa11f8cc20e974dc11102] target/mips: Remove 
>> vendor specific CPU definitions
>> git bisect good eaca85763bcd94ddac3fa11f8cc20e974dc11102
>> # good: [5f8679fe46d78acfa5fc43a3fd6b3fe95525d9bd] vnc: Fix a memleak in 
>> vnc_display_connect()
>> git bisect good 5f8679fe46d78acfa5fc43a3fd6b3fe95525d9bd
>> # good: [a968a38005bf2568605cac7f86b9fba7fc089726] Merge remote-tracking 
>> branch 'remotes/gkurz-gitlab/tags/9p-next-2021-01-15' into staging
>> git bisect good a968a38005bf2568605cac7f86b9fba7fc089726
>> # bad: [9e1632ad07ca49de99da4bb231e9e2f22f2d8df5] vnc: move initialization 
>> to framebuffer_update_request
>> git bisect bad 9e1632ad07ca49de99da4bb231e9e2f22f2d8df5
>> # good: [b3c2de9cd5bc0023901e7a4d568dfc5152b6cc4a] vnc: move check into 
>> vnc_cursor_define
>> git bisect good b3c2de9cd5bc0023901e7a4d568dfc5152b6cc4a
>> # first bad commit: [9e1632ad07ca49de99da4bb231e9e2f22f2d8df5] vnc: move 
>> initialization to framebuffer_update_request
>
> The symptom is the following: in virt-manager, the display remains dead
> (black), when I start an OVMF guest. At the same time, unusually high
> CPU load can be seen on the host; it makes me think that virt-manager is
> trying, in a busy loop, to complete the VNC handshake, or some such.
> Ultimately, although the guest boots up fine, virt-manager gives up, and
> the display window says "Viewer was disconnected".
>
> Versions (apart from qemu):
>
> - gtk-vnc2-0.7.0-3.el7.x86_64
> - gvnc-0.7.0-3.el7.x86_64
> - libvirt-daemon-6.6.0-8 (rebuilt from RHEL8 to RHEL7)
> - spice-gtk3-0.35-5.el7_9.1.x86_64 (in case it matters...)
> - virt-manager-1.5.0-7.el7.noarch
>
> The domain config is:
>
>>     <graphics type='vnc' port='-1' autoport='yes'>
>>       <listen type='address'/>
>>     </graphics>
>>     <video>
>>       <model type='qxl' ram='65536' vram='65536' vgamem='16384' heads='1' 
>> primary='yes'/>
>>       <address type='pci' domain='0x0000' bus='0x00' slot='0x03' 
>> function='0x0'/>
>>     </video>
>
> Reverting
>
> - 763deea7e906 ("vnc: add support for extended desktop resize",
>   2021-01-15), and
>
> - 9e1632ad07ca ("vnc: move initialization to
>   framebuffer_update_request", 2021-01-15),
>
> in this order, returns QEMU to working state.

The patch also breaks QEMU for the "examples/gvncviewer" utility, built
from the gtk-vnc project at commit c7942ba5499a ("src: remove use of
obsolete thread APIs", 2021-01-07). (This commit is the last one that I
can easily build on RHEL7, as the next one would be f79a54e8b692
("build: bump min versions of dependancies to RHEL8 vintage",
2021-01-07).)

The symptom is that gncviewer @ c7942ba5499a displays an initial image,
but no updates -- instead, it is spinning:

> Connected to server
> Remote desktop size changed to 640x480
> Connection initialized
> [here]

and then after a while, it prints:

> Error: Failed to flush data
> Disconnected from server

If I revert the above-mentioned two QEMU patches (the one after this
patch first, for a clean context, and then this one), then the same
gncviewer binary (built at c7942ba5499a) works perfectly with QEMU.

(The vncviewer utility from tigervnc-1.8.0-22.el7.x86_64 works fine both
with this QEMU patch in place, and with it reverted.)

So I think that this QEMU change exercises a part of the RFB spec that
gtk-vnc used to be incompatible with, and that incompatibility must have
been fixed only recently, somewhere in the commit range
c7942ba5499a..6046a8b4bfd9 (the latter being the current HEAD of the
master branch):

 1  f79a54e8b692 build: bump min versions of dependancies to RHEL8 vintage
 2  ddff6d5b1b8f build: enable glib/gtk API version usage checking
 3  80b73802cf96 build: change with-vala and introspection options into features
 4  8b79e4023777 build: disable introspection in mingw builds
 5  27e73b8b0f6c src: add minimal support for extended desktop resize
 6  faacea8b016e src: block non-incremental updates after extended desktop 
resize
 7  b884d72881db src: add API for requesting a desktop resize
 8  4821aeb05e1b src: add APIs to control whether desktop resizing is allowed
 9  8e86bc76bca7 src: implement dynamic resize of remote desktop on widget 
resize
10  6046a8b4bfd9 examples: add menu option to enable desktop resizing

I can't perform a reverse bisection on this range (for finding the fix
in gtk-vnc) because commit#1 prevents building gtk-vnc on RHEL7 (bumps
the minimum libgcrypt version from 1.5.0 to 1.8.0).

Thanks
Laszlo

>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index 0f01481cac57..b4e98cf647f5 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -661,10 +661,6 @@ static void vnc_desktop_resize(VncState *vs)
>>      if (vs->ioc == NULL || !vnc_has_feature(vs, VNC_FEATURE_RESIZE)) {
>>          return;
>>      }
>> -    if (vs->client_width == pixman_image_get_width(vs->vd->server) &&
>> -        vs->client_height == pixman_image_get_height(vs->vd->server)) {
>> -        return;
>> -    }
>>
>>      assert(pixman_image_get_width(vs->vd->server) < 65536 &&
>>             pixman_image_get_width(vs->vd->server) >= 0);
>> @@ -2014,6 +2010,10 @@ static void framebuffer_update_request(VncState *vs, 
>> int incremental,
>>      } else {
>>          vs->update = VNC_STATE_UPDATE_FORCE;
>>          vnc_set_area_dirty(vs->dirty, vs->vd, x, y, w, h);
>> +        vnc_colordepth(vs);
>> +        vnc_desktop_resize(vs);
>> +        vnc_led_state_change(vs);
>> +        vnc_cursor_define(vs);
>>      }
>>  }
>>
>> @@ -2154,10 +2154,7 @@ static void set_encodings(VncState *vs, int32_t 
>> *encodings, size_t n_encodings)
>>              break;
>>          }
>>      }
>> -    vnc_desktop_resize(vs);
>>      check_pointer_type_change(&vs->mouse_mode_notifier, NULL);
>> -    vnc_led_state_change(vs);
>> -    vnc_cursor_define(vs);
>>  }
>>
>>  static void set_pixel_conversion(VncState *vs)
>>
>