loolwsd/LOOLProtocol.hpp | 6 +++++- loolwsd/test/helpers.hpp | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-)
New commits: commit 53b718844fd2c0b6366055cf6fc4d018cf6c339b Author: Tor Lillqvist <t...@collabora.com> Date: Tue Oct 18 16:23:33 2016 +0300 Add a FIXME for some sillyness One thing works mainly by accident, another is inefficient. diff --git a/loolwsd/test/helpers.hpp b/loolwsd/test/helpers.hpp index 9e3756e..ab2b1ba 100644 --- a/loolwsd/test/helpers.hpp +++ b/loolwsd/test/helpers.hpp @@ -188,6 +188,22 @@ std::vector<char> getResponseMessage(Poco::Net::WebSocket& ws, const std::string auto message = LOOLProtocol::getAbbreviatedMessage(response); if (bytes > 0 && (flags & Poco::Net::WebSocket::FRAME_OP_BITMASK) != Poco::Net::WebSocket::FRAME_OP_CLOSE) { + // FIXME: This is wrong in two ways: + + // 1) The message variable is the return value from getAbbreviatedMessage(), + // That is a string that is intended for human-readable logging only. It is not + // intended to be used for actually decoding the protocol. Sure, at the moment + // it happens to work, but is still wrong. + + // 2) Using std::string::find() like this is silly. If message does not start + // with prefix, the find() function will search through the whole buffer. That + // is a waste of cycles. Use the LOOLProtocol functions to manipulate and + // inspect LOOL protocol frames, that is what they are there + // for. getFirstToken() should be quite efficient, and it doesn't incur the + // (potential) overhead of setting up a StringTokenizer. Or, if this is actually + // used to look for not just a first token, then introduce a startsWith() + // function. + if (message.find(prefix) == 0) { std::cerr << name << "Got " << bytes << " bytes: " << message << std::endl; commit b0c870cbeb77b69c5bf1046646b5253b411a546a Author: Tor Lillqvist <t...@collabora.com> Date: Tue Oct 18 15:51:29 2016 +0300 Expand documentation for getAbbreviatedMessage() It has always been the intent that getAbbreviatedMessage() is for producing human-readbale nice output for logging purposes only. But this has not been mentioned, so (naturally) the function has been misused. Sigh. diff --git a/loolwsd/LOOLProtocol.hpp b/loolwsd/LOOLProtocol.hpp index 6120e0c..3857a4e 100644 --- a/loolwsd/LOOLProtocol.hpp +++ b/loolwsd/LOOLProtocol.hpp @@ -133,7 +133,11 @@ namespace LOOLProtocol return getFirstLine(message.data(), message.size()); } - /// Returns an abbereviation of the message (the first line, indicating truncation). + /// Returns an abbereviation of the message (the first line, indicating truncation). We assume + /// that it adhers to the LOOL protocol, i.e. that there is always a first (or only) line that + /// is in printable UTF-8. I.e. no encoding of binary bytes is done. The format of the result is + /// not guaranteed to be stable. It is to be used for logging purposes only, not for decoding + /// protocol frames. inline std::string getAbbreviatedMessage(const char *message, const int length) { _______________________________________________ Libreoffice-commits mailing list libreoffice-comm...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits