Merge lp:~andreas-pokorny/mir/fix-null-cmdline-crash into lp:mir

[Andreas Pokorny]

merged from 0.14.0: fix to avoid crash within qtmir test suite when accessing missing program parameter

qmir test suite passes an empty (argc == 0) command line to mir. Due to a change in configuration handling we access argv[0] on server construction.

This is one of the small changes that were necessary to release 0.14.0. This change allows providing argc=0 to Server. qtmir does that in a couple of tests.

[Daniel van Vugt (vanvugt)]

review: Approve

[Alan Griffiths (alan-griffiths)]

I realize it is low cost, but should we support this usecase? (Or fix qtmir?)

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2783
http://jenkins.qa.ubuntu.com/job/mir-ci/4419/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3315
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/168
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/840
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3263
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/568
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/568/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3263
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3263/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6059
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22140

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

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

setting argc == 0 seems like a user error to me; just because it is well-known how argc/argv works.

I don't mind fixing though so that argc/argv work like a normal array, but while we're fixing zany user errors, we could also fix if someone does:
server.set_command_line(-112, nullptr);

[Andreas Pokorny (andreas-pokorny)]

.. the necessary change in qtmir would be small too. I made the change in mir because I believe that qtmirs test setup worked with previous versions of mir/

[Alan Griffiths (alan-griffiths)]

Given the choice between adding logic to production code just to support test code and fixing the test code not to need it...I recommend fixing the test code.

review: Disapprove

[Cemil Azizoglu (cemil-azizoglu)]

> Given the choice between adding logic to production code just to support test
> code and fixing the test code not to need it...I recommend fixing the test
> code.

+1

Unmerged revisions

2783. By Andreas Pokorny on 2015-07-27

merged from 0.14.0: fix to avoid crash within qtmir test suite

qmir test suite passes an empty (argc == 0) command line to mir. Due to a change in configuration handling we access argv[0] on server construction.

=== modified file 'src/platform/options/default_configuration.cpp'
--- src/platform/options/default_configuration.cpp	2015-07-23 02:39:20 +0000
+++ src/platform/options/default_configuration.cpp	2015-07-27 08:22:32 +0000
@@ -131,7 +131,9 @@
     argv(argv),
     unparsed_arguments_handler{handler},
     program_options(std::make_shared<boost::program_options::options_description>(
-        description_text(argv[0], config_file)))
+        description_text(
+            (argc && argv)?argv[0]:"mir-server",
+            config_file)))
 {
     using namespace options;
     namespace po = boost::program_options;

=== modified file 'tests/acceptance-tests/server_configuration_options.cpp'
--- tests/acceptance-tests/server_configuration_options.cpp	2014-11-06 13:26:52 +0000
+++ tests/acceptance-tests/server_configuration_options.cpp	2015-07-27 08:22:32 +0000
@@ -123,6 +123,18 @@
     server.apply_settings();
 }
 
+TEST_F(ServerConfigurationOptions, empty_command_line_is_allowed)
+{
+    const int argc = 0;
+    char const** argv = 0;
+
+    server.set_command_line(argc, argv);
+
+    EXPECT_CALL(*this, command_line_handler(_)).Times(0);
+
+    server.apply_settings();
+}
+
 TEST_F(ServerConfigurationOptions, are_read_from_xdg_config_home)
 {
     create_config_file_in(fake_xdg_config_home);