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

[Alan Griffiths]

client, frontend: Introduce meaningful versioning into the protobuf protocol and check it.
(LP: #1486496)

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)]

PASSED: Continuous integration, rev:2880
http://jenkins.qa.ubuntu.com/job/mir-ci/4675/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3669
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/90
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2580
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3619
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/824
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/824/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3619
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3619/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6391
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22820

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

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

lgtm

review: Approve

[Alexandros Frantzis (afrantzis)]

What if we break the protocol without an API change?

review: Needs Information

[Alan Griffiths (alan-griffiths)]

> What if we break the protocol without an API change?

Then the soname doesn't change, clients links to the new libmirclient and everything works.

But I see your point - calling it the protocol version is misleading. Suggestions for a better name please?

[Chris Halse Rogers (raof)]

There's actually no guarantee that the client code will link with the new libmirclient. It could be statically linked, or provided by the .snap rather than the system (Snappy core doesn't have mirclient in the base image)

-----Original Message-----
From: "Alan Griffiths" <email address hidden>
Sent: ‎27/‎08/‎2015 18:28
To: "Alexandros Frantzis" <email address hidden>
Subject: Re: [Merge] lp:~alan-griffiths/mir/fix-1486496 into lp:mir

> What if we break the protocol without an API change?

Then the soname doesn't change, clients links to the new libmirclient and everything works.

But I see your point - calling it the protocol version is misleading. Suggestions for a better name please?
--
https://code.launchpad.net/~alan-griffiths/mir/fix-1486496/+merge/269087
You are requested to review the proposed merge of lp:~alan-griffiths/mir/fix-1486496 into lp:mir.

[Alan Griffiths (alan-griffiths)]

> There's actually no guarantee that the client code will link with the new
> libmirclient. It could be statically linked, or provided by the .snap rather
> than the system (Snappy core doesn't have mirclient in the base image)

https://bugs.launchpad.net/mir/+bug/1486496/comments/21

[Alexandros Frantzis (afrantzis)]

> > What if we break the protocol without an API change?
>
> Then the soname doesn't change, clients links to the new libmirclient and
> everything works.
>
> But I see your point - calling it the protocol version is misleading.
> Suggestions for a better name please?

How about using protocol(epoch, major, minor), where epoch is a value we can increase manually as needed:

current client API -> supported protocol versions

1:3.0 -> 1:3.0
1:3.1 -> 1:3.0, 1:3.1
<protocol break without API break> 2:3.1 -> (2:3.0 which doesn't exist) and 2:3.1

[Alexandros Frantzis (afrantzis)]

> current client API -> supported protocol versions

epoch:current_client_API -> supported protocol versions

[Alan Griffiths (alan-griffiths)]

> epoch:current_client_API -> supported protocol versions

OK, added epoch

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2882
http://jenkins.qa.ubuntu.com/job/mir-ci/4694/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3699
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/120
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2610
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3649
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/843
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/843/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3649
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3649/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6414
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22864

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

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

still lgtm

[Daniel van Vugt (vanvugt)]

Tested: Client built from this branch, server built from lp:mir/0.14.

Indeed the hang is now replaced with an inexplicable failure to connect. Even mir_connection_get_error_message() just gives "Connect failed". In a way this is kind of worse, because the user will be even more confused than before.

We need a custom error message so clients that bother to call mir_connection_get_error_message() will know why they are failing and there really is a server at the other end that's just rejecting us.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> Tested: Client built from this branch, server built from lp:mir/0.14.
>
> Indeed the hang is now replaced with an inexplicable failure to connect. Even
> mir_connection_get_error_message() just gives "Connect failed". In a way this
> is kind of worse, because the user will be even more confused than before.
>
> We need a custom error message so clients that bother to call
> mir_connection_get_error_message() will know why they are failing and there
> really is a server at the other end that's just rejecting us.

All that the client sees is the connection being dropped. Even if we were to change that handshake in current servers it wouldn't help with 0.14 servers which just close the socket.

There are other failure modes, e.g.

"Failed to connect to server `/tmp/mir_socket': Failed to connect to server socket: Permission denied"

So I've updated the message to:

"Failed to connect to server `/tmp/mir_socket': Failed to connect: not accepted by server"

Which is a little less opaque but not specific beyond what the client really knows. (E.g. it could be the SessionAuthorizer that forces the disconnect.)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2884
http://jenkins.qa.ubuntu.com/job/mir-ci/4715/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3725
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/146
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2636
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3675
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/864
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/864/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3675
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3675/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6434
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22897

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

OK, that will work.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/1827/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3785/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/207
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2696
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3735
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-autolanding/380
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-autolanding/380/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3735
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3735/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6470
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22954

review: Needs Fixing (continuous-integration)

=== 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-28 13:15:15 +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();
     }
@@ -259,7 +259,7 @@
         std::lock_guard<decltype(mutex)> lock(mutex);
 
         if (!connect_result->has_platform() || !connect_result->has_display_configuration())
-            set_error_message("Connect failed");
+            set_error_message("Failed to connect: not accepted by server");
 
         connect_done = true;
 

=== 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-28 13:15:15 +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-28 13:15:15 +0000
@@ -118,6 +118,7 @@
 
 private:
     std::atomic<int> next_message_id;
+    int const protocol_version;
 };
 
 }

=== 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-28 13:15:15 +0000
@@ -0,0 +1,49 @@
+/*
+ * 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
+{
+// For the present we use the client API protocol_version as a proxy for the protocol
+// version as the protocol is typically updated to support API changes.
+// If we need to break protocol without a corresponding ABI break then we need to bump "epoch"
+inline constexpr int protocol_version(int major, int minor, int epoch = 0)
+{
+    return MIR_VERSION_NUMBER(epoch, major, minor);
+}
+
+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-28 13:15:15 +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-28 13:15:15 +0000
@@ -135,7 +135,11 @@
     }
     
     mtf::UsingStubClientPlatform using_stub_client_platform;
+
+    static auto constexpr current_protocol_epoch = 0;
 };
+
+auto const* const protocol_version_override = "MIR_CLIENT_TEST_OVERRRIDE_PROTOCOL_VERSION";
 }
 
 using namespace testing;
@@ -164,6 +168,67 @@
     mir_connection_release(connection);
 }
 
+TEST_F(ClientLibrary, connects_when_protobuf_protocol_oldest_supported)
+{
+    std::ostringstream buffer;
+    buffer << MIR_VERSION_NUMBER(current_protocol_epoch, MIR_CLIENT_MAJOR_VERSION, 0);
+
+    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_VERSION_NUMBER(current_protocol_epoch, MIR_CLIENT_MAJOR_VERSION-1, 0);
+    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), HasSubstr("not accepted by server"));
+
+    mir_connection_release(connection);
+}
+
+TEST_F(ClientLibrary, reports_error_when_protobuf_protocol_too_new)
+{
+    std::ostringstream buffer;
+    buffer << MIR_VERSION_NUMBER(current_protocol_epoch, MIR_CLIENT_MAJOR_VERSION, 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), HasSubstr("not accepted by server"));
+
+    mir_connection_release(connection);
+}
+
+TEST_F(ClientLibrary, reports_error_when_protobuf_protocol_epoch_too_new)
+{
+    std::ostringstream buffer;
+    buffer << MIR_VERSION_NUMBER(current_protocol_epoch+1, MIR_CLIENT_MAJOR_VERSION, MIR_CLIENT_MINOR_VERSION);
+    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), HasSubstr("not accepted by server"));
+
+    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-28 13:15:15 +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("Failed to connect"));
 
     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-28 13:15:15 +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;