From a3ac01ef9ce92ca2ddd31a8a647937235e294f6d Mon Sep 17 00:00:00 2001 From: Pierre Ossman Date: Mon, 7 Nov 2011 21:13:54 +0000 Subject: [PATCH] Clean up the interface for VNCSConnectionST. Entry points are more apparent and the data flow is now more strictly aimed towards this connection class. git-svn-id: svn://svn.code.sf.net/p/tigervnc/code/trunk@4771 3789f03b-4d11-0410-bbf8-ca57d06f2519 --- common/rfb/SDesktop.h | 7 -- common/rfb/VNCSConnectionST.cxx | 139 +++++++++++++++++++------------- common/rfb/VNCSConnectionST.h | 21 +++-- common/rfb/VNCServer.h | 4 - common/rfb/VNCServerST.cxx | 20 ++--- common/rfb/VNCServerST.h | 1 - win/rfb_win32/SDisplay.cxx | 7 +- win/rfb_win32/SDisplay.h | 1 - 8 files changed, 103 insertions(+), 97 deletions(-) diff --git a/common/rfb/SDesktop.h b/common/rfb/SDesktop.h index 4a53d538..57ceb076 100644 --- a/common/rfb/SDesktop.h +++ b/common/rfb/SDesktop.h @@ -64,13 +64,6 @@ namespace rfb { virtual void stop() {} - // framebufferUpdateRequest() is called to let the desktop know that at - // least one client has become ready for an update. Desktops can check - // whether there are clients ready at any time by calling the VNCServer's - // clientsReadyForUpdate() method. - - virtual void framebufferUpdateRequest() {} - // getFbSize() returns the current dimensions of the framebuffer. // This can be called even while the SDesktop is not start()ed. diff --git a/common/rfb/VNCSConnectionST.cxx b/common/rfb/VNCSConnectionST.cxx index 1478ba5d..84fc2ca1 100644 --- a/common/rfb/VNCSConnectionST.cxx +++ b/common/rfb/VNCSConnectionST.cxx @@ -1,5 +1,5 @@ /* Copyright (C) 2002-2005 RealVNC Ltd. All Rights Reserved. - * Copyright 2009 Pierre Ossman for Cendio AB + * Copyright 2009-2011 Pierre Ossman for Cendio AB * * This is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -34,7 +34,7 @@ static LogWriter vlog("VNCSConnST"); VNCSConnectionST::VNCSConnectionST(VNCServerST* server_, network::Socket *s, bool reverse) - : SConnection(reverse), sock(s), server(server_), + : SConnection(reverse), sock(s), inProcessMessages(false), server(server_), updates(false), image_getter(server->useEconomicTranslate), drawRenderedCursor(false), removeRenderedCursor(false), updateTimer(this), pointerEventTime(0), @@ -111,22 +111,19 @@ void VNCSConnectionST::processMessages() try { // - Now set appropriate socket timeouts and process data setSocketTimeouts(); - bool clientsReadyBefore = server->clientsReadyForUpdate(); + + inProcessMessages = true; while (getInStream()->checkNoWait(1)) { processMsg(); } - // If there were update requests, try to send a framebuffer update. - // We don't send updates immediately on requests as this way, we - // give higher priority to user actions such as keyboard and - // pointer events. - if (!requested.is_empty()) { - writeFramebufferUpdate(); - } + inProcessMessages = false; - if (!clientsReadyBefore && !requested.is_empty()) - server->desktop->framebufferUpdateRequest(); + // If there were anything requiring an update, try to send it here. + // We wait until now with this to aggregate responses and to give + // higher priority to user actions such as keyboard and pointer events. + writeFramebufferUpdate(); } catch (rdr::EndOfStream&) { close("Clean disconnection"); } catch (rdr::Exception &e) { @@ -134,15 +131,6 @@ void VNCSConnectionST::processMessages() } } -void VNCSConnectionST::writeFramebufferUpdateOrClose() -{ - try { - writeFramebufferUpdate(); - } catch(rdr::Exception &e) { - close(e.str()); - } -} - void VNCSConnectionST::pixelBufferChange() { try { @@ -186,27 +174,25 @@ void VNCSConnectionST::pixelBufferChange() updates.add_changed(server->pb->getRect()); vlog.debug("pixel buffer changed - re-initialising image getter"); image_getter.init(server->pb, cp.pf(), writer()); - if (writer()->needFakeUpdate()) - writeFramebufferUpdate(); + writeFramebufferUpdate(); } catch(rdr::Exception &e) { close(e.str()); } } -void VNCSConnectionST::screenLayoutChange(rdr::U16 reason) +void VNCSConnectionST::writeFramebufferUpdateOrClose() { try { - if (!authenticated()) - return; - - cp.screenLayout = server->screenLayout; - if (state() == RFBSTATE_NORMAL) { - writer()->writeExtendedDesktopSize(reason, 0, cp.width, cp.height, - cp.screenLayout); - } + writeFramebufferUpdate(); + } catch(rdr::Exception &e) { + close(e.str()); + } +} - if (writer()->needFakeUpdate()) - writeFramebufferUpdate(); +void VNCSConnectionST::screenLayoutChangeOrClose(rdr::U16 reason) +{ + try { + screenLayoutChange(reason); } catch(rdr::Exception &e) { close(e.str()); } @@ -221,7 +207,7 @@ void VNCSConnectionST::setColourMapEntriesOrClose(int firstColour,int nColours) } } -void VNCSConnectionST::bell() +void VNCSConnectionST::bellOrClose() { try { if (state() == RFBSTATE_NORMAL) writer()->writeBell(); @@ -230,7 +216,7 @@ void VNCSConnectionST::bell() } } -void VNCSConnectionST::serverCutText(const char *str, int len) +void VNCSConnectionST::serverCutTextOrClose(const char *str, int len) { try { if (!(accessRights & AccessCutText)) return; @@ -243,15 +229,10 @@ void VNCSConnectionST::serverCutText(const char *str, int len) } -void VNCSConnectionST::setDesktopName(const char *name) +void VNCSConnectionST::setDesktopNameOrClose(const char *name) { - cp.setName(name); try { - if (state() == RFBSTATE_NORMAL) { - if (!writer()->writeSetDesktopName()) { - fprintf(stderr, "Client does not support desktop rename\n"); - } - } + setDesktopName(name); } catch(rdr::Exception& e) { close(e.str()); } @@ -547,8 +528,7 @@ void VNCSConnectionST::setDesktopSize(int fb_width, int fb_height, if (!layout.validate(fb_width, fb_height)) { writer()->writeExtendedDesktopSize(reasonClient, resultInvalid, fb_width, fb_height, layout); - if (writer()->needFakeUpdate()) - writeFramebufferUpdate(); + writeFramebufferUpdate(); return; } @@ -557,18 +537,19 @@ void VNCSConnectionST::setDesktopSize(int fb_width, int fb_height, // protocol-wise, but unnecessary. result = server->desktop->setScreenLayout(fb_width, fb_height, layout); - // Always send back a reply to the requesting client writer()->writeExtendedDesktopSize(reasonClient, result, fb_width, fb_height, layout); - if (writer()->needFakeUpdate()) - writeFramebufferUpdate(); - // But only notify other clients on success + // Only notify other clients on success if (result == resultSuccess) { if (server->screenLayout != layout) throw Exception("Desktop configured a different screen layout than requested"); server->notifyScreenLayoutChange(this); } + + // but always send back a reply to the requesting client + // (do this last as it might throw an exception on socket errors) + writeFramebufferUpdate(); } void VNCSConnectionST::setInitialColourMap() @@ -627,8 +608,12 @@ void VNCSConnectionST::writeSetCursorCallback() bool VNCSConnectionST::handleTimeout(Timer* t) { - if (t == &updateTimer) - writeFramebufferUpdateOrClose(); + try { + if (t == &updateTimer) + writeFramebufferUpdate(); + } catch (rdr::Exception& e) { + close(e.str()); + } return false; } @@ -647,6 +632,12 @@ void VNCSConnectionST::writeFramebufferUpdate() { updateTimer.stop(); + // We try to aggregate responses, so don't send out anything whilst we + // still have incoming messages. processMessages() will give us another + // chance to run once things are idle. + if (inProcessMessages) + return; + if (state() != RFBSTATE_NORMAL || requested.is_empty()) return; @@ -788,15 +779,33 @@ void VNCSConnectionST::writeRenderedCursorRect() drawRenderedCursor = false; } +void VNCSConnectionST::screenLayoutChange(rdr::U16 reason) +{ + if (!authenticated()) + return; + + cp.screenLayout = server->screenLayout; + + if (state() != RFBSTATE_NORMAL) + return; + + writer()->writeExtendedDesktopSize(reason, 0, cp.width, cp.height, + cp.screenLayout); + writeFramebufferUpdate(); +} + void VNCSConnectionST::setColourMapEntries(int firstColour, int nColours) { - if (!readyForSetColourMapEntries) return; - if (server->pb->getPF().trueColour) return; + if (!readyForSetColourMapEntries) + return; + if (server->pb->getPF().trueColour) + return; image_getter.setColourMapEntries(firstColour, nColours); if (cp.pf().trueColour) { updates.add_changed(server->pb->getRect()); + writeFramebufferUpdate(); } } @@ -807,10 +816,28 @@ void VNCSConnectionST::setColourMapEntries(int firstColour, int nColours) void VNCSConnectionST::setCursor() { - if (state() != RFBSTATE_NORMAL || !cp.supportsLocalCursor) return; + if (state() != RFBSTATE_NORMAL) + return; + if (!cp.supportsLocalCursor) + return; + writer()->cursorChange(this); - if (writer()->needFakeUpdate()) - writeFramebufferUpdate(); + writeFramebufferUpdate(); +} + +void VNCSConnectionST::setDesktopName(const char *name) +{ + cp.setName(name); + + if (state() != RFBSTATE_NORMAL) + return; + + if (!writer()->writeSetDesktopName()) { + fprintf(stderr, "Client does not support desktop rename\n"); + return; + } + + writeFramebufferUpdate(); } void VNCSConnectionST::setSocketTimeouts() diff --git a/common/rfb/VNCSConnectionST.h b/common/rfb/VNCSConnectionST.h index abf43838..5a007ee9 100644 --- a/common/rfb/VNCSConnectionST.h +++ b/common/rfb/VNCSConnectionST.h @@ -1,5 +1,5 @@ /* Copyright (C) 2002-2005 RealVNC Ltd. All Rights Reserved. - * Copyright 2009 Pierre Ossman for Cendio AB + * Copyright 2009-2011 Pierre Ossman for Cendio AB * * This is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -63,14 +63,17 @@ namespace rfb { // Socket if an error occurs, via the close() call. void processMessages(); - void writeFramebufferUpdateOrClose(); + // Called when the underlying pixelbuffer is resized or replaced. void pixelBufferChange(); - void screenLayoutChange(rdr::U16 reason); + + // Wrappers to make these methods "safe" for VNCServerST. + void writeFramebufferUpdateOrClose(); + void screenLayoutChangeOrClose(rdr::U16 reason); void setColourMapEntriesOrClose(int firstColour, int nColours); - void bell(); - void serverCutText(const char *str, int len); - void setDesktopName(const char *name); void setCursorOrClose(); + void bellOrClose(); + void serverCutTextOrClose(const char *str, int len); + void setDesktopNameOrClose(const char *name); // checkIdleTimeout() returns the number of milliseconds left until the // idle timeout expires. If it has expired, the connection is closed and @@ -92,7 +95,6 @@ namespace rfb { bool needRenderedCursor(); network::Socket* getSock() { return sock; } - bool readyForUpdate() { return !requested.is_empty(); } void add_changed(const Region& region) { updates.add_changed(region); } void add_copied(const Region& dest, const Point& delta) { updates.add_copied(dest, delta); @@ -155,12 +157,17 @@ namespace rfb { void writeFramebufferUpdate(); void writeRenderedCursorRect(); + void screenLayoutChange(rdr::U16 reason); void setColourMapEntries(int firstColour, int nColours); void setCursor(); + void setDesktopName(const char *name); void setSocketTimeouts(); network::Socket* sock; CharArray peerEndpoint; + + bool inProcessMessages; + VNCServerST* server; SimpleUpdateTracker updates; TransImageGetter image_getter; diff --git a/common/rfb/VNCServer.h b/common/rfb/VNCServer.h index 7fa44c18..1c8d38e0 100644 --- a/common/rfb/VNCServer.h +++ b/common/rfb/VNCServer.h @@ -58,10 +58,6 @@ namespace rfb { // bell() tells the server that it should make all clients make a bell sound. virtual void bell() = 0; - // clientsReadyForUpdate() returns true if there is at least one client - // waiting for an update, false if no clients are ready. - virtual bool clientsReadyForUpdate() = 0; - // - Close all currently-connected clients, by calling // their close() method with the supplied reason. virtual void closeClients(const char* reason) = 0; diff --git a/common/rfb/VNCServerST.cxx b/common/rfb/VNCServerST.cxx index 32be8625..eea6565d 100644 --- a/common/rfb/VNCServerST.cxx +++ b/common/rfb/VNCServerST.cxx @@ -330,7 +330,7 @@ void VNCServerST::setScreenLayout(const ScreenSet& layout) std::list::iterator ci, ci_next; for (ci=clients.begin();ci!=clients.end();ci=ci_next) { ci_next = ci; ci_next++; - (*ci)->screenLayoutChange(reasonServer); + (*ci)->screenLayoutChangeOrClose(reasonServer); } } @@ -348,7 +348,7 @@ void VNCServerST::bell() std::list::iterator ci, ci_next; for (ci = clients.begin(); ci != clients.end(); ci = ci_next) { ci_next = ci; ci_next++; - (*ci)->bell(); + (*ci)->bellOrClose(); } } @@ -357,7 +357,7 @@ void VNCServerST::serverCutText(const char* str, int len) std::list::iterator ci, ci_next; for (ci = clients.begin(); ci != clients.end(); ci = ci_next) { ci_next = ci; ci_next++; - (*ci)->serverCutText(str, len); + (*ci)->serverCutTextOrClose(str, len); } } @@ -367,7 +367,7 @@ void VNCServerST::setName(const char* name_) std::list::iterator ci, ci_next; for (ci = clients.begin(); ci != clients.end(); ci = ci_next) { ci_next = ci; ci_next++; - (*ci)->setDesktopName(name_); + (*ci)->setDesktopNameOrClose(name_); } } @@ -385,16 +385,6 @@ void VNCServerST::add_copied(const Region& dest, const Point& delta) } } -bool VNCServerST::clientsReadyForUpdate() -{ - std::list::iterator ci; - for (ci = clients.begin(); ci != clients.end(); ci++) { - if ((*ci)->readyForUpdate()) - return true; - } - return false; -} - void VNCServerST::tryUpdate() { std::list::iterator ci, ci_next; @@ -613,6 +603,6 @@ void VNCServerST::notifyScreenLayoutChange(VNCSConnectionST* requester) ci_next = ci; ci_next++; if ((*ci) == requester) continue; - (*ci)->screenLayoutChange(reasonOtherClient); + (*ci)->screenLayoutChangeOrClose(reasonOtherClient); } } diff --git a/common/rfb/VNCServerST.h b/common/rfb/VNCServerST.h index aa9ade0f..c74be903 100644 --- a/common/rfb/VNCServerST.h +++ b/common/rfb/VNCServerST.h @@ -85,7 +85,6 @@ namespace rfb { virtual void serverCutText(const char* str, int len); virtual void add_changed(const Region ®ion); virtual void add_copied(const Region &dest, const Point &delta); - virtual bool clientsReadyForUpdate(); virtual void tryUpdate(); virtual void setCursor(int width, int height, const Point& hotspot, void* cursorData, void* mask); diff --git a/win/rfb_win32/SDisplay.cxx b/win/rfb_win32/SDisplay.cxx index f14b49f1..05ddce34 100644 --- a/win/rfb_win32/SDisplay.cxx +++ b/win/rfb_win32/SDisplay.cxx @@ -316,11 +316,6 @@ void SDisplay::clientCutText(const char* text, int len) { } -void SDisplay::framebufferUpdateRequest() -{ - SetEvent(updateEvent); -} - Point SDisplay::getFbSize() { bool startAndStop = !core; @@ -382,7 +377,7 @@ SDisplay::processEvent(HANDLE event) { inputs->blockInputs(disableLocalInputs); // - Only process updates if the server is ready - if (server && server->clientsReadyForUpdate()) { + if (server) { bool try_update = false; // - Check that the SDesktop doesn't need restarting diff --git a/win/rfb_win32/SDisplay.h b/win/rfb_win32/SDisplay.h index 6dbb50a5..6aac59ae 100644 --- a/win/rfb_win32/SDisplay.h +++ b/win/rfb_win32/SDisplay.h @@ -68,7 +68,6 @@ namespace rfb { virtual void pointerEvent(const Point& pos, int buttonmask); virtual void keyEvent(rdr::U32 key, bool down); virtual void clientCutText(const char* str, int len); - virtual void framebufferUpdateRequest(); virtual Point getFbSize(); // -=- Clipboard -- 2.39.5