| Message ID | 20210115102424.1360437-11-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,01/11] ui/gtk: don't try to redefine SI prefixes | expand |
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. 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) >
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) >> >
> This patch breaks QEMU for me. > 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". It is the vnc_colordepth() call. Seems gtk-vnc sends a update request with incremental=0 as response to the VNC_ENCODING_WMVi message. So sending that as response to an incremental=0 update request creates an endless loop ... take care, Gerd diff --git a/ui/vnc.c b/ui/vnc.c index d429bfee5a65..0a3e2f4aa98c 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -2041,7 +2041,6 @@ 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);
On 01/22/21 09:46, Gerd Hoffmann wrote: >> This patch breaks QEMU for me. > >> 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". > > It is the vnc_colordepth() call. Seems gtk-vnc sends a update request > with incremental=0 as response to the VNC_ENCODING_WMVi message. So > sending that as response to an incremental=0 update request creates an > endless loop ... Interesting; I saw that commit 9e1632ad07ca *added* (as opposed to *moving*) the vnc_colordepth() call; I thought it was a relatively insignificant bit... I'll report back on your separately posted patch. Thanks! Laszlo > > take care, > Gerd > > diff --git a/ui/vnc.c b/ui/vnc.c > index d429bfee5a65..0a3e2f4aa98c 100644 > --- a/ui/vnc.c > +++ b/ui/vnc.c > @@ -2041,7 +2041,6 @@ 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); >
On Fri, Jan 22, 2021 at 01:49:38PM +0100, Laszlo Ersek wrote: > On 01/22/21 09:46, Gerd Hoffmann wrote: > >> This patch breaks QEMU for me. > > > >> 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". > > > > It is the vnc_colordepth() call. Seems gtk-vnc sends a update request > > with incremental=0 as response to the VNC_ENCODING_WMVi message. So > > sending that as response to an incremental=0 update request creates an > > endless loop ... > > Interesting; I saw that commit 9e1632ad07ca *added* (as opposed to > *moving*) the vnc_colordepth() call; I thought it was a relatively > insignificant bit... /me too. Some discussions in the resize changes indicated that the idea of a non-incremetal update request is that the server sends the *full* server-side state, so the client can render the screen properly without remembering old state. So I thought ok, lets send the colordepth info too, no big deal ... take care, Gerd
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)