Merge lp:~alan-griffiths/mir/fix-1486496 into lp:mir

[Alan Griffiths]

client, frontend: Introduce meaningful versioning into the protobuf protocol and check it.

client, frontend: Introduce meaningful versioning into the protobuf protocol and check it.

Because the server simply disconnects on a connection failure the client doesn't receive any reason for the failure. However, reporting a connection failure is better than hanging.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2872
http://jenkins.qa.ubuntu.com/job/mir-ci/4645/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3629
    FAILURE: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/50/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2540
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3579/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/794/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3579
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3579/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6348/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22753

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4645/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2873
http://jenkins.qa.ubuntu.com/job/mir-ci/4647/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3631
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/52
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2542
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3581/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/796
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/796/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3581
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3581/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6350/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22757

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4647/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2874
http://jenkins.qa.ubuntu.com/job/mir-ci/4649/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3633
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/54
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2544
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3583/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/798
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/798/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3583
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3583/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6352/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22760

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4649/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2875
http://jenkins.qa.ubuntu.com/job/mir-ci/4651/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3635
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/56
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2546
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3585/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/800/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3585
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3585/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6354/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22763

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4651/rebuild

review: Needs Fixing (continuous-integration)

[Chris Halse Rogers (raof)]

126 +// Client libraries older than last ABI break may make calls we can't understand

This seems to be an extremely conservative choice, which will likely prevent many clients which would otherwise work fine from connecting. We remove protocol extremely rarely; the last protocol we removed had been deprecated for 2 years.

I don't think it's unreasonable to manually populate the oldest_compatible_protocol_version().

Otherwise sensible.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> 126 +// Client libraries older than last ABI break may make calls we can't
> understand
>
> This seems to be an extremely conservative choice, which will likely prevent
> many clients which would otherwise work fine from connecting. We remove
> protocol extremely rarely; the last protocol we removed had been deprecated
> for 2 years.

If you investigate the linked bug you'll find that this choice is actually correct as things stand on trunk.

I don't think it is *yet* worth spending the effort to ensure current libmirclient can interwork with old libmirserver versions. (There is no likelihood of Mir servers based on different libmirservers being installed side-by-side on a user system.)

> I don't think it's unreasonable to manually populate the
> oldest_compatible_protocol_version().

I think being conservative is a good default.

Next time we bump the major API version number we can consider if we can be less conservative. I hope after all the "opaquification" and deprecation that landed in 0.14 that this will be a very long time.

> Otherwise sensible.

review: Approve

[Alan Griffiths (alan-griffiths)]

Oops!

review: Abstain

[Alan Griffiths (alan-griffiths)]

After attempting to reproduce locally and chatting with kdub on IRC it looks like the CI failure (hang in glmark) is related to lp:1441553.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2877
http://jenkins.qa.ubuntu.com/job/mir-ci/4661/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3648
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/69
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2559
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3598/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/810
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/810/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3598
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3598/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6367/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22779

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4661/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2878
http://jenkins.qa.ubuntu.com/job/mir-ci/4666/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3656
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/77
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2567
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3606/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/815
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/815/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3606
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3606/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6375/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22792

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4666/rebuild

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

The hang in mir_performance test is because this...

Get:3 http://ports.ubuntu.com/ubuntu-ports/ vivid/universe glmark2-es2-mir armhf 2014.03-0ubuntu3 [298 kB]

... is the libmirclient.so.8 based version from vivid. *Not* the libmirclient.so.9 based rebuild from the "Stable Phone Overlay PPA".

(That is version 2014.03+git20150611.fa71af2d-0ubuntu1.1~overlay1)

=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2015-08-21 11:36:32 +0000
+++ src/client/mir_connection.cpp	2015-08-25 15:45:40 +0000
@@ -165,7 +165,7 @@
 {
     std::lock_guard<decltype(mutex)> lock(mutex);
 
-    if (connect_result && connect_result->has_error())
+    if (error_message.empty() && connect_result)
     {
         return connect_result->error().c_str();
     }

=== modified file 'src/client/rpc/mir_basic_rpc_channel.cpp'
--- src/client/rpc/mir_basic_rpc_channel.cpp	2015-07-29 01:39:52 +0000
+++ src/client/rpc/mir_basic_rpc_channel.cpp	2015-08-25 15:45:40 +0000
@@ -23,12 +23,33 @@
 #include "mir_protobuf_wire.pb.h"
 #include "mir/frontend/client_constants.h"
 #include "mir/variable_length_array.h"
+#include "mir/protobuf/protocol_version.h"
+#include "mir/log.h"
 
 #include <sstream>
 
 namespace mclr = mir::client::rpc;
 namespace mclrd = mir::client::rpc::detail;
 
+namespace
+{
+int get_protocol_version()
+{
+    auto protocol_version = mir::protobuf::current_protocol_version();
+
+    if (auto const protocol_version_override = getenv("MIR_CLIENT_TEST_OVERRRIDE_PROTOCOL_VERSION"))
+    {
+        int const new_protcol_version = strtol(protocol_version_override, nullptr, 0);
+        mir::log(mir::logging::Severity::warning, MIR_LOG_COMPONENT,
+                 "Overriding protocol version 0x%x with 0x%x", protocol_version, new_protcol_version);
+
+        protocol_version = new_protcol_version;
+    }
+
+    return protocol_version;
+}
+}
+
 mclrd::PendingCallCache::PendingCallCache(
     std::shared_ptr<RpcReport> const& rpc_report)
     : rpc_report{rpc_report}
@@ -99,7 +120,8 @@
 
 
 mclr::MirBasicRpcChannel::MirBasicRpcChannel() :
-    next_message_id(0)
+    next_message_id(0),
+    protocol_version{get_protocol_version()}
 {
 }
 
@@ -120,7 +142,7 @@
     invoke.set_id(next_id());
     invoke.set_method_name(method_name);
     invoke.set_parameters(buffer.data(), buffer.size());
-    invoke.set_protocol_version(1);
+    invoke.set_protocol_version(protocol_version);
     invoke.set_side_channel_fds(num_side_channel_fds);
 
     return invoke;

=== modified file 'src/client/rpc/mir_basic_rpc_channel.h'
--- src/client/rpc/mir_basic_rpc_channel.h	2015-07-29 01:39:52 +0000
+++ src/client/rpc/mir_basic_rpc_channel.h	2015-08-25 15:45:40 +0000
@@ -118,6 +118,7 @@
 
 private:
     std::atomic<int> next_message_id;
+    int const protocol_version;
 };
 
 }

=== modified file 'src/include/common/mir/frontend/client_constants.h'
--- src/include/common/mir/frontend/client_constants.h	2015-02-22 07:46:25 +0000
+++ src/include/common/mir/frontend/client_constants.h	2015-08-25 15:45:40 +0000
@@ -27,7 +27,9 @@
 /// Number of buffers the client library will keep.
 
 /// mir::client::ClientBufferDepository and mir::frontend::ClientBufferTracker need to use the same value
-unsigned int const client_buffer_cache_size = 3;
+// TODO this ought to be 3 - but is 4 as a workaround for issues with overallocation (lp:1441553)
+// (this should become moot with "new Buffer Semantics")
+unsigned int const client_buffer_cache_size = 4;
 
 /// Buffers need to be big enough to support messages
 unsigned int const serialization_buffer_size = 2048;

=== added file 'src/include/common/mir/protobuf/protocol_version.h'
--- src/include/common/mir/protobuf/protocol_version.h	1970-01-01 00:00:00 +0000
+++ src/include/common/mir/protobuf/protocol_version.h	2015-08-25 15:45:40 +0000
@@ -0,0 +1,48 @@
+/*
+ * Copyright © 2015 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alan Griffiths <[e-mail]>
+ */
+
+#ifndef MIR_PROTOBUF_PROTOCOL_VERSION_H
+#define MIR_PROTOBUF_PROTOCOL_VERSION_H
+
+#include "mir_toolkit/version.h"
+
+namespace mir
+{
+namespace protobuf
+{
+inline constexpr int protocol_version(int major, int minor)
+{
+    return (major << 10) + minor;
+}
+
+// For the present we use the client API protocol_version as a proxy for the protocol
+//  protocol_version as the protocol is typically updated to support API changes.
+inline constexpr int current_protocol_version() 
+{ 
+    return protocol_version(MIR_CLIENT_MAJOR_VERSION, MIR_CLIENT_MINOR_VERSION); 
+}
+
+// Client libraries older than last ABI break may make calls we can't understand
+inline constexpr int oldest_compatible_protocol_version()
+{
+    return protocol_version(MIR_CLIENT_MAJOR_VERSION, 0);
+}
+}
+}
+
+#endif //MIR_PROTOBUF_PROTOCOL_VERSION_H

=== modified file 'src/server/frontend/socket_connection.cpp'
--- src/server/frontend/socket_connection.cpp	2015-02-22 07:46:25 +0000
+++ src/server/frontend/socket_connection.cpp	2015-08-25 15:45:40 +0000
@@ -21,6 +21,7 @@
 #include "message_receiver.h"
 #include "mir/frontend/message_processor.h"
 #include "mir/frontend/session_credentials.h"
+#include "mir/protobuf/protocol_version.h"
 
 #include "mir_protobuf_wire.pb.h"
 
@@ -90,6 +91,10 @@
 void mfd::SocketConnection::on_new_message(const boost::system::error_code& error)
 try
 {
+    // No support for newer client libraries
+    static auto const forward_compatibility_limit = mir::protobuf::current_protocol_version();
+    static auto const backward_compatibility_limit = mir::protobuf::oldest_compatible_protocol_version();
+
     if (error)
     {
         BOOST_THROW_EXCEPTION(std::runtime_error(error.message()));
@@ -98,7 +103,9 @@
     mir::protobuf::wire::Invocation invocation;
     invocation.ParseFromArray(body.data(), body.size());
 
-    if (!invocation.has_protocol_version() || invocation.protocol_version() != 1)
+    if (!invocation.has_protocol_version() ||
+        invocation.protocol_version() > forward_compatibility_limit ||
+        invocation.protocol_version() < backward_compatibility_limit)
         BOOST_THROW_EXCEPTION(std::runtime_error("Unsupported protocol version"));
 
     std::vector<mir::Fd> fds;

=== modified file 'tests/acceptance-tests/test_client_library.cpp'
--- tests/acceptance-tests/test_client_library.cpp	2015-07-27 07:24:50 +0000
+++ tests/acceptance-tests/test_client_library.cpp	2015-08-25 15:45:40 +0000
@@ -136,6 +136,8 @@
     
     mtf::UsingStubClientPlatform using_stub_client_platform;
 };
+
+auto const* const protocol_version_override = "MIR_CLIENT_TEST_OVERRRIDE_PROTOCOL_VERSION";
 }
 
 using namespace testing;
@@ -164,6 +166,52 @@
     mir_connection_release(connection);
 }
 
+TEST_F(ClientLibrary, connects_when_protobuf_protocol_oldest_supported)
+{
+    std::ostringstream buffer;
+    buffer << ((MIR_CLIENT_MAJOR_VERSION) << 10);
+
+    add_to_environment(protocol_version_override, buffer.str().c_str());
+
+    connection = mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__);
+
+    EXPECT_THAT(connection, NotNull());
+    EXPECT_TRUE(mir_connection_is_valid(connection));
+    EXPECT_THAT(mir_connection_get_error_message(connection), StrEq(""));
+
+    mir_connection_release(connection);
+}
+
+TEST_F(ClientLibrary, reports_error_when_protobuf_protocol_obsolete)
+{
+    std::ostringstream buffer;
+    buffer << ((MIR_CLIENT_MAJOR_VERSION-1) << 10);
+    add_to_environment(protocol_version_override, buffer.str().c_str());
+
+    connection = mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__);
+
+    EXPECT_THAT(connection, NotNull());
+    EXPECT_FALSE(mir_connection_is_valid(connection));
+    EXPECT_THAT(mir_connection_get_error_message(connection), StrEq("Connect failed"));
+
+    mir_connection_release(connection);
+}
+
+TEST_F(ClientLibrary, reports_error_when_protobuf_protocol_too_new)
+{
+    std::ostringstream buffer;
+    buffer << ((MIR_CLIENT_MAJOR_VERSION) << 10) + MIR_CLIENT_MINOR_VERSION+1;
+    add_to_environment(protocol_version_override, buffer.str().c_str());
+
+    connection = mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__);
+
+    EXPECT_THAT(connection, NotNull());
+    EXPECT_FALSE(mir_connection_is_valid(connection));
+    EXPECT_THAT(mir_connection_get_error_message(connection), StrEq("Connect failed"));
+
+    mir_connection_release(connection);
+}
+
 TEST_F(ClientLibrary, creates_surface)
 {
     mir_wait_for(mir_connect(new_connection().c_str(), __PRETTY_FUNCTION__, connection_callback, this));

=== modified file 'tests/integration-tests/test_error_reporting.cpp'
--- tests/integration-tests/test_error_reporting.cpp	2015-07-29 01:18:29 +0000
+++ tests/integration-tests/test_error_reporting.cpp	2015-08-25 15:45:40 +0000
@@ -100,7 +100,7 @@
 
     ASSERT_TRUE(connection != NULL);
     EXPECT_FALSE(mir_connection_is_valid(connection));
-    EXPECT_THAT(mir_connection_get_error_message(connection), testing::HasSubstr(test_exception_text));
+    EXPECT_THAT(mir_connection_get_error_message(connection), testing::HasSubstr("Connect failed"));
 
     mir_connection_release(connection);
 }

=== modified file 'tests/unit-tests/frontend/test_socket_connection.cpp'
--- tests/unit-tests/frontend/test_socket_connection.cpp	2015-06-25 03:00:08 +0000
+++ tests/unit-tests/frontend/test_socket_connection.cpp	2015-08-25 15:45:40 +0000
@@ -22,6 +22,7 @@
 #include "mir/frontend/message_processor.h"
 #include "mir/frontend/session_credentials.h"
 #include "mir/fd.h"
+#include "mir/protobuf/protocol_version.h"
 
 #include "mir/test/fake_shared.h"
 
@@ -145,7 +146,7 @@
         invocation.set_id(1);
         invocation.set_method_name("");
         invocation.set_parameters(buffer, 0);
-        invocation.set_protocol_version(1);
+        invocation.set_protocol_version(mir::protobuf::current_protocol_version());
         invocation.set_side_channel_fds(2);
         auto const body_size = invocation.ByteSize();
         buffer[0] = body_size / 0x100;