Mir

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

fix-1543049
Merge into development-branch

Proposed by Alan Griffiths on 2016-03-01

Status:

Merged

Approved by:

Alan Griffiths on 2016-03-04

Approved revision:

no longer in the source branch.

Merged at revision:

3363

Proposed branch:

lp:~alan-griffiths/mir/fix-1543049

Merge into:

lp:mir

Diff against target:

140 lines (+54/-36)

3 files modified

src/server/input/input_probe.cpp (+35/-36)
tests/unit-tests/CMakeLists.txt (+1/-0)
tests/unit-tests/input/test_input_platform_probing.cpp (+18/-0)

To merge this branch:

bzr merge lp:~alan-griffiths/mir/fix-1543049

Low

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Mir CI Bot	continuous-integration		Approve on 2016-03-04
Chris Halse Rogers			Approve on 2016-03-04
Daniel van Vugt			Abstain on 2016-03-03
Andreas Pokorny (community)			Approve on 2016-03-02
Cemil Azizoglu (community)		2016-03-01	Approve on 2016-03-01
Review via email: mp+287644@code.launchpad.net

Commit message

input: update mi::probe_input_platforms() to select the newest of duplicate input modules (LP: #1543049)

Description of the change

input: update mi::probe_input_platforms() to select the newest of duplicate input modules

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-03-01:

I'm unsure whether I should add the following to the selection logic around line 60

                // Ignore modules from different major versions of Mir
                if (MIR_VERSION_MAJOR != desc->major_version)
                    return Selection::persist;

                // Ignore modules from more recent minor versions of Mir
                if (MIR_VERSION_MINOR < desc->minor_version)
                    return Selection::persist;

Does anyone have an opinion either way?

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-01:

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/460/rebuild

review: Approve (continuous-integration)

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2016-03-01:

Ok.

Nit : s/huristic/heuristic
62 + // This is a huristic that assumes we're always looking for the most up-to-date driver,

review: Approve

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-01:

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/462/rebuild

review: Approve (continuous-integration)

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2016-03-01:

yeah just played the scenario of bumping one ABI.. this works.

review: Approve

Revision history for this message

Chris Halse Rogers (raof) wrote on 2016-03-01:

Oooh! You know what we should do? We should *first* dlopen(NULL, $STUFF) and see if we can resolve the relevant symbols from there!

If we can, then that means we've already loaded a module that provides the symbol, and we should use it. That'd resolve:
+ // TODO find a way to coordinate the selection of mesa-x11 and input platforms

Does that work to fix the bug?

review: Needs Information

Revision history for this message

Chris Halse Rogers (raof) wrote on 2016-03-01:

(That'd be mir::SharedLibrary us{nullptr};)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-03-01:

On Tuesday, 1 March 2016 23:13:53 GMT, Chris Halse Rogers
<email address hidden> wrote:
> Review: Needs Information
>
> Oooh! You know what we should do? We should *first*
> dlopen(NULL, $STUFF) and see if we can resolve the relevant
> symbols from there!
>
> If we can, then that means we've already loaded a module that
> provides the symbol, and we should use it. That'd resolve:
> + // TODO find a way to coordinate the selection
> of mesa-x11 and input platforms
>
> Does that work to fix the bug?

Seems somewhat orthogonal to the bug, and only addresses the comment if
both the graphics module is guaranteed to load first and we only want one
input module. (We return a collection for some reason.)

--
Alan Griffiths. +44 (0)798 9938 758
Octopull Limited. http://www.octopull.co.uk/

Revision history for this message

Chris Halse Rogers (raof) wrote on 2016-03-02:

My understanding of the bug is that we have a problem when we load mesa-x11 as the graphics module and then load a *different* mesa-x11 as the input module. Is that not correct?

Having both the input probe and graphics probe first check if the relevant symbol is already by an already-loaded module would fix this.

(Sorry for the direct mail, there. Stupid not-reply-all).

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-03-02:

Although it's clearly possible to set up a machine to make the test case in question fail, most peoples' machines are not failing any tests even while the bug exists. So can we add a new unit test for the issue to cover that gap?

review: Needs Fixing

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-03-02:

> Although it's clearly possible to set up a machine to make the test case in
> question fail, most peoples' machines are not failing any tests even while the
> bug exists.

All you need is a working copy in which you've built Mir. Then, as indicated in the bug, bump MIR_SERVER_GRAPHICS_PLATFORM_ABI and rebuild without cleaning lib/server-modules/.

This can happen in normal workflow either when you, or a branch you merge updates the graphics platform ABI. As noted in the bug deleting the old lib works around the problem.

AFAIK we've never yet replicated this problem with a release, but as Chris indicates, it could also impact some upgrade scenarios.

> So can we add a new unit test for the issue to cover that gap?

Hmm. Because the logic is hard coded to the real filesystem there's no convenient "seam" for testing, but I'll have a think about the best way to address this.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-03-02:

> My understanding of the bug is that we have a problem when we load mesa-x11 as
> the graphics module and then load a *different* mesa-x11 as the input module.
> Is that not correct?

Strictly, that's not the scenario described in the linked bug. The bug is that we try to load multiple mesa-x11 input modules.

What you describe is indeed a potential issue as mentioned in the TODO comment.

> Having both the input probe and graphics probe first check if the relevant
> symbol is already by an already-loaded module would fix this.

For reasons I don't yet understand the input probing can (and does) select multiple input modules. The logic to match already-loaded modules so as to reject related candidates but not reject unrelated candidates is not as simple as just looking for a symbol. I'd rather leave that for a future MP (hence the TODO).

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-02:

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/470/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-03-02:

> For reasons I don't yet understand the input probing can (and does) select
> multiple input modules. The logic to match already-loaded modules so as to
> reject related candidates but not reject unrelated candidates is not as simple
> as just looking for a symbol. I'd rather leave that for a future MP (hence the
> TODO).

I've discussed this with Andreas and we don't need multiple input modules.

I'd like to land and build on this MP. Two changes will follow: refactoring to using a single input module; and then, as you suggest, checking to see if we've already loaded a satisfactory module.

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2016-03-02:

Yeah I agree to the plan..

review: Approve

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-03-02:

> Oooh! You know what we should do? We should *first* dlopen(NULL, $STUFF) and
> see if we can resolve the relevant symbols from there!

One of us is missing something. Surely that can't work modules (correctly) loaded with RTLD_LOCAL?

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-03-03:

I understand the "seam" problem. I'm dealing with a similar one myself...

P.S. The single input module approach is probably going to bite us:
https://code.launchpad.net/~alan-griffiths/mir/fix-1528110/+merge/287800/comments/734991

review: Abstain

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-03-03:

> I'm unsure whether I should add the following to the selection logic around
> line 60
>
> // Ignore modules from different major versions of Mir
> if (MIR_VERSION_MAJOR != desc->major_version)
> return Selection::persist;
>
> // Ignore modules from more recent minor versions of Mir
> if (MIR_VERSION_MINOR < desc->minor_version)
> return Selection::persist;
>
> Does anyone have an opinion either way?

Answering my own question - that only works for plugins we supply ourselves (with MIR_VERSION based versioning).

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-03-03:

> Oooh! You know what we should do? We should *first* dlopen(NULL, $STUFF) and
> see if we can resolve the relevant symbols from there!
>
> If we can, then that means we've already loaded a module that provides the
> symbol, and we should use it. That'd resolve:
> + // TODO find a way to coordinate the selection of mesa-x11
> and input platforms

Give or take the detail that dlopen(NULL, $STUFF) won't work (modules are loaded with RTLD_LOCAL) and different trick is needed that's the approach taken in a grandchild of this MP:

lp:~alan-griffiths/mir/spike-input-from-the-existing-graphics-module/+merge/287964

So you're probably happy with this part of it now?

Revision history for this message

Chris Halse Rogers (raof) wrote on 2016-03-04:

Yup, I'm happy with this now.

review: Approve

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-04:

review: Needs Fixing (continuous-integration)

Revision history for this message

Mir CI Bot (mir-ci-bot) on 2016-03-04:

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Alan Griffiths

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === modified file 'src/server/input/input_probe.cpp'
 --- src/server/input/input_probe.cpp	2015-10-14 15:22:39 +0000
 +++ src/server/input/input_probe.cpp	2016-03-02 10:44:16 +0000
@@ -34,22 +34,6 @@
  namespace
+ {
--auto probe_input_platform(mir::SharedLibrary const& lib, mir::options::Option const& options)
--{
--    auto probe = lib.load_function<mi::ProbePlatform>("probe_input_platform", MIR_SERVER_INPUT_PLATFORM_VERSION);
--
--    return probe(options);
--}
--
--void describe_input_platform(mir::SharedLibrary const& lib)
--{
--    auto describe =
--        lib.load_function<mi::DescribeModule>("describe_input_module", MIR_SERVER_INPUT_PLATFORM_VERSION);
--    auto desc = describe();
--    mir::log_info("Selected input driver: %s (version: %d.%d.%d)", desc->name, desc->major_version, desc->minor_version,
--                  desc->micro_version);
--}
--
  mir::UniqueModulePtr<mi::Platform> create_input_platform(
      mir::SharedLibrary const& lib, mir::options::Option const& options,
      std::shared_ptr<mir::EmergencyCleanupRegistry> const& cleanup_registry,
@@ -67,37 +51,52 @@
      std::shared_ptr<mi::InputDeviceRegistry> const& device_registry, std::shared_ptr<mi::InputReport> const& input_report,
      mir::SharedLibraryProberReport& prober_report)
+ {
++    auto reject_platform_priority = mi::PlatformPriority::dummy;
++
      std::vector<UniqueModulePtr<Platform>> platforms;
--
--    if (options.is_set(mo::platform_input_lib))
--    {
--        mir::SharedLibrary lib(options.get<std::string>(mo::platform_input_lib));
--
--        platforms.emplace_back(create_input_platform(lib, options, emergency_cleanup, device_registry, input_report));
--
--        describe_input_platform(lib);
--    }
--    else
--    {
--        auto const& path = options.get<std::string>(mo::platform_path);
--        auto platforms_libs = mir::libraries_for_path(path, prober_report);
--
--        for (auto const& platform_lib : platforms_libs)
++    std::vector<std::string> module_names;
++
++    auto const module_selector = [&](std::shared_ptr<mir::SharedLibrary> const& module)
+         {
              try
+             {
--                if (probe_input_platform(*platform_lib, options) > mi::PlatformPriority::dummy)
++                auto const probe = module->load_function<mi::ProbePlatform>(
++                    "probe_input_platform", MIR_SERVER_INPUT_PLATFORM_VERSION);
++                auto const desc = module->load_function<mi::DescribeModule>(
++                    "describe_input_module", MIR_SERVER_INPUT_PLATFORM_VERSION)();
++
++                // We only take the first found of duplicate modules, as that will be the most recent.
++                // This is a heuristic that assumes we're always looking for the most up-to-date driver,
++                // TODO find a way to coordinate the selection of mesa-x11 and input platforms
++                auto const duplicate = find(begin(module_names), end(module_names), desc->name) != end(module_names);
++
++                if (probe(options) > reject_platform_priority && !duplicate)
+                 {
                      platforms.emplace_back(
--                        create_input_platform(*platform_lib, options, emergency_cleanup, device_registry, input_report));
--
--                    describe_input_platform(*platform_lib);
++                        create_input_platform(*module, options, emergency_cleanup, device_registry, input_report));
++
++                    module_names.push_back(desc->name);
++
++                    mir::log_info("Selected input driver: %s (version: %d.%d.%d)",
++                        desc->name, desc->major_version, desc->minor_version, desc->micro_version);
+                 }
+             }
              catch (std::runtime_error const&)
+             {
++                // Assume we were handed a SharedLibrary that's not an input module of the correct vintage.
+             }
--        }
++
++            return Selection::persist;
++        };
++
++    if (options.is_set(mo::platform_input_lib))
++    {
++        reject_platform_priority = PlatformPriority::unsupported;
++        module_selector(std::make_shared<mir::SharedLibrary>(options.get<std::string>(mo::platform_input_lib)));
++    }
++    else
++    {
++        select_libraries_for_path(options.get<std::string>(mo::platform_path), module_selector, prober_report);
+     }
      return platforms;
 === modified file 'tests/unit-tests/CMakeLists.txt'
 --- tests/unit-tests/CMakeLists.txt	2016-02-24 03:55:11 +0000
 +++ tests/unit-tests/CMakeLists.txt	2016-03-02 10:44:16 +0000
@@ -2,6 +2,7 @@
  add_definitions(
    -DMIR_CLIENT_PLATFORM_VERSION="${MIR_CLIENT_PLATFORM_VERSION}"
++  -DMIR_SERVER_GRAPHICS_PLATFORM_ABI_STRING="${MIR_SERVER_GRAPHICS_PLATFORM_ABI}"
+ )
  if (MIR_BUILD_PLATFORM_ANDROID)
 === modified file 'tests/unit-tests/input/test_input_platform_probing.cpp'
 --- tests/unit-tests/input/test_input_platform_probing.cpp	2015-12-02 13:22:51 +0000
 +++ tests/unit-tests/input/test_input_platform_probing.cpp	2016-03-02 10:44:16 +0000
@@ -149,6 +149,24 @@
      EXPECT_THAT(platforms, UnorderedElementsAre(OfPtrType<mi::evdev::Platform>(), OfPtrType<mi::X::XInputPlatform>()));
+ }
++TEST_F(InputPlatformProbe, when_multiple_x11_platforms_are_eligible_only_one_is_selected)
++{
++    auto const real_lib = mtf::server_platform_path() + "server-mesa-x11.so." MIR_SERVER_GRAPHICS_PLATFORM_ABI_STRING;
++    auto const fake_lib = mtf::server_platform_path() + "server-mesa-x11.so.0";
++
++    ASSERT_THAT(real_lib, Ne(fake_lib));
++    remove(fake_lib.c_str());
++    ASSERT_THAT(link(real_lib.c_str(), fake_lib.c_str()), Eq(0));
++
++    auto platforms =
++        mi::probe_input_platforms(mock_options, mt::fake_shared(stub_emergency), mt::fake_shared(mock_registry),
++                                  mr::null_input_report(), *stub_prober_report);
++
++    EXPECT_THAT(platforms, UnorderedElementsAre(OfPtrType<mi::evdev::Platform>(), OfPtrType<mi::X::XInputPlatform>()));
++
++    remove(fake_lib.c_str());
++}
++
  TEST_F(InputPlatformProbe, x11_input_platform_not_used_when_vt_specified)
+ {
      ON_CALL(mock_options, is_set(StrEq(vt))).WillByDefault(Return(true));