Merge lp:~mlankhorst/mir/setsid into lp:~mir-team/mir/trunk

[Maarten Lankhorst]

Re-introduce console support, and ignore control characters

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:900
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~mlankhorst/mir/setsid/+merge/177800/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/1140/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1498
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1383
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/378
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/378/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/741

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:900
http://jenkins.qa.ubuntu.com/job/mir-ci/1144/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1503
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1388
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/382
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/382/artifact/work/output/*zip*/output.zip
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/745

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

review: Approve (continuous-integration)

[Robert Ancell (robert-ancell)]

Code looks good, haven't tested it for behaviour.

review: Approve

[Alan Griffiths (alan-griffiths)]

No style issues with the code, but I'm unfamiliar with the APIs and the issue being addressed

review: Abstain

[Alexandros Frantzis (afrantzis)]

54 + if (fops->ioctl(vt_fd.fd(), KDSKBMODE, K_OFF) < 0)

Doesn't this turn off input completely? If so, we need to be able to handle VT switching through another mechanism before this lands (handling the keypresses ourselves and changing VT).

review: Needs Information

[Daniel van Vugt (vanvugt)]

Tested it myself. This does indeed break VT switching, though clients get input as they should.

Once you start the server you can't switch away from it, or kill it (without an ssh login).

So needs fixing:
  1. Add support for VT switching out of Mir (maybe Ctrl+Alt+Fn).
  2. Add support for killing the server, since Ctrl+C doesn't any more (maybe use Ctrl+Alt+Backspace)

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Though it's possible that XMir requires neither of those new key combos. So maybe they need to be implemented in examples/demo_server.cpp and examples/demo-shell/*

I'm not sure. Do we want VT switching and emergency exit to be a generic thing implemented in libmirserver? If not then it would go in the examples/ servers.

[Chris Halse Rogers (raof)]

I don't believe that we want VT switching to be a core thing at all; don't we plan to remove VT switching entirely at some point?

[Daniel van Vugt (vanvugt)]

Fair point. But to avoid blocking other developers from doing their thing, VT switching should be reimplemented in demo_server_shell at least. And maybe demo_server.

Similarly, being able to kill the demo server is critical for us. I suggest Ctrl+Alt+Backspace.

[Daniel van Vugt (vanvugt)]

Put it this way... We need to be capable of running a demo server and client(s) on a single machine. Otherwise development is extremely difficult.

This means (1) VT switching support; and (2) the ability to kill the server. Though if (1) is working well enough then you can use that in place of (2).

[Robert Ancell (robert-ancell)]

> Fair point. But to avoid blocking other developers from doing their thing, VT
> switching should be reimplemented in demo_server_shell at least. And maybe
> demo_server.
>
> Similarly, being able to kill the demo server is critical for us. I suggest
> Ctrl+Alt+Backspace.

https://code.launchpad.net/~robert-ancell/mir/alt-ctrl-backspace/+merge/178036

[Robert Ancell (robert-ancell)]

> I don't believe that we want VT switching to be a core thing at all; don't we
> plan to remove VT switching entirely at some point?

Certainly not in the 13.10 timeframe.

[Robert Ancell (robert-ancell)]

I tried to make the demo server do VT switching itself, but it locks up in the same way that the current trunk does (i.e. video stops updating showing last frame - can't switch back).

Code is lp:~robert-ancell/mir/setsid-alt-ctrl-Fn

Given we can't VT switch on trunk I don't think this blocks this MP.

[Robert Ancell (robert-ancell)]

Actually, with further testing I can VT switch from trunk, and not from this branch (even once merged with trunk). So there is an issue in it that stops VT switching.

review: Needs Fixing

[Robert Ancell (robert-ancell)]

With some more work in lp:~robert-ancell/mir/setsid-alt-ctrl-Fn I can now VT switch with this patch. So now we just need:
- This branch to support quitting and/or VT switching in the example servers
- Unity System Compositor to support VT switching

[Robert Ancell (robert-ancell)]

This MP enables VT switching in Unity System Compositor:
https://code.launchpad.net/~robert-ancell/unity-system-compositor/vt-switch/+merge/178072

[Daniel van Vugt (vanvugt)]

Robert,

Can you propose lp:~robert-ancell/mir/setsid-alt-ctrl-Fn for merging into this one, when ready?

[Robert Ancell (robert-ancell)]

I've got alt+ctrl+Fn keys working in https://code.launchpad.net/~robert-ancell/mir/vt-switch-keys/+merge/179067. That can be landed separately from this.

[Robert Ancell (robert-ancell)]

review: Approve

[Chris Halse Rogers (raof)]

In conjunction with vt-switch-keys, +1

review: Approve

[Daniel van Vugt (vanvugt)]

This looks a bit old school... 9600 bps?

65 + cfsetispeed(&tcattr, B9600);
66 + tcattr.c_oflag = 0;
67 + cfsetospeed(&tcattr, B9600);

But anyway, it works and my previous concerns are resolved by the sister branch: lp:~robert-ancell/mir/vt-switch-keys

review: Approve

=== modified file 'src/server/graphics/gbm/gbm_platform.cpp'
--- src/server/graphics/gbm/gbm_platform.cpp	2013-07-30 13:56:22 +0000
+++ src/server/graphics/gbm/gbm_platform.cpp	2013-07-31 10:50:37 +0000
@@ -78,6 +78,16 @@
     {
         return ::ioctl(d, request, p_val);
     }
+
+    int tcsetattr(int d, int acts, const struct termios *tcattr)
+    {
+        return ::tcsetattr(d, acts, tcattr);
+    }
+
+    int tcgetattr(int d, struct termios *tcattr)
+    {
+        return ::tcgetattr(d, tcattr);
+    }
 };
 
 }

=== modified file 'src/server/graphics/gbm/linux_virtual_terminal.cpp'
--- src/server/graphics/gbm/linux_virtual_terminal.cpp	2013-07-30 16:19:14 +0000
+++ src/server/graphics/gbm/linux_virtual_terminal.cpp	2013-07-31 10:50:37 +0000
@@ -45,8 +45,11 @@
       vt_fd{fops, open_vt(vt_number)},
       prev_kd_mode{0},
       prev_vt_mode(),
+      prev_tty_mode(),
+      prev_tcattr(),
       active{true}
 {
+    struct termios tcattr;
     if (fops->ioctl(vt_fd.fd(), KDGETMODE, &prev_kd_mode) < 0)
     {
         BOOST_THROW_EXCEPTION(
@@ -59,14 +62,44 @@
     {
         BOOST_THROW_EXCEPTION(
             boost::enable_error_info(
-                std::runtime_error("Failed to get the current VT")) << boost::errinfo_errno(errno));
-    }
+                std::runtime_error("Failed to get the current VT"))
+                    << boost::errinfo_errno(errno));
+    }
+
+    if (fops->ioctl(vt_fd.fd(), KDGKBMODE, &prev_tty_mode) < 0)
+    {
+        BOOST_THROW_EXCEPTION(
+            boost::enable_error_info(
+                std::runtime_error("Failed to get the current TTY mode"))
+                    << boost::errinfo_errno(errno));
+    }
+    if (fops->ioctl(vt_fd.fd(), KDSKBMODE, K_OFF) < 0)
+    {
+        BOOST_THROW_EXCEPTION(
+            boost::enable_error_info(
+                std::runtime_error("Failed to mute keyboard"))
+                    << boost::errinfo_errno(errno));
+    }
+
+    fops->tcgetattr(vt_fd.fd(), &prev_tcattr);
+    tcattr = prev_tcattr;
+    tcattr.c_iflag = IGNPAR | IGNBRK;
+    cfsetispeed(&tcattr, B9600);
+    tcattr.c_oflag = 0;
+    cfsetospeed(&tcattr, B9600);
+    tcattr.c_cflag = CREAD | CS8;
+    tcattr.c_lflag = 0;
+    tcattr.c_cc[VTIME] = 0;
+    tcattr.c_cc[VMIN] = 1;
+    fops->tcsetattr(vt_fd.fd(), TCSANOW, &tcattr);
 }
 
 mgg::LinuxVirtualTerminal::~LinuxVirtualTerminal() noexcept(true)
 {
     if (vt_fd.fd() > 0)
     {
+        fops->tcsetattr(vt_fd.fd(), TCSANOW, &prev_tcattr);
+        fops->ioctl(vt_fd.fd(), KDSKBMODE, prev_tty_mode);
         fops->ioctl(vt_fd.fd(), KDSETMODE, prev_kd_mode);
         fops->ioctl(vt_fd.fd(), VT_SETMODE, &prev_vt_mode);
     }
@@ -186,6 +219,26 @@
 
     std::string const active_vt_path{vt_path_stream.str()};
 
+    if (activate)
+    {
+        if (getpid() == getpgid(0) && setpgid(0, getpgid(getppid())) < 0)
+        {
+            BOOST_THROW_EXCEPTION(
+                boost::enable_error_info(
+                    std::runtime_error("Failed to stop being a process group"))
+                       << boost::errinfo_errno(errno));
+        }
+
+        /* become process group leader */
+        if (setsid() < 0)
+        {
+            BOOST_THROW_EXCEPTION(
+                boost::enable_error_info(
+                    std::runtime_error("Failed to become session leader"))
+                       << boost::errinfo_errno(errno));
+        }
+    }
+
     auto vt_fd = fops->open(active_vt_path.c_str(), O_RDONLY | O_NDELAY);
 
     if (vt_fd < 0)

=== modified file 'src/server/graphics/gbm/linux_virtual_terminal.h'
--- src/server/graphics/gbm/linux_virtual_terminal.h	2013-06-20 02:16:09 +0000
+++ src/server/graphics/gbm/linux_virtual_terminal.h	2013-07-31 10:50:37 +0000
@@ -24,6 +24,7 @@
 #include <memory>
 
 #include <linux/vt.h>
+#include <termios.h>
 #include <unistd.h>
 
 namespace mir
@@ -45,6 +46,8 @@
     virtual int close(int fd) = 0;
     virtual int ioctl(int d, int request, int val) = 0;
     virtual int ioctl(int d, int request, void* p_val) = 0;
+    virtual int tcsetattr(int d, int acts, const struct termios *tcattr) = 0;
+    virtual int tcgetattr(int d, struct termios *tcattr) = 0;
 
 protected:
     VTFileOperations() = default;
@@ -90,6 +93,8 @@
     FDWrapper const vt_fd;
     int prev_kd_mode;
     struct vt_mode prev_vt_mode;
+    int prev_tty_mode;
+    struct termios prev_tcattr;
     bool active;
 };
 

=== modified file 'tests/unit-tests/graphics/gbm/test_linux_virtual_terminal.cpp'
--- tests/unit-tests/graphics/gbm/test_linux_virtual_terminal.cpp	2013-06-28 15:16:39 +0000
+++ tests/unit-tests/graphics/gbm/test_linux_virtual_terminal.cpp	2013-07-31 10:50:37 +0000
@@ -47,6 +47,8 @@
     MOCK_METHOD1(close, int(int));
     MOCK_METHOD3(ioctl, int(int, int, int));
     MOCK_METHOD3(ioctl, int(int, int, void*));
+    MOCK_METHOD3(tcsetattr, int(int, int, const struct termios*));
+    MOCK_METHOD2(tcgetattr, int(int, struct termios*));
 };
 
 class MockMainLoop : public mir::MainLoop
@@ -73,6 +75,13 @@
     *static_cast<T*>(arg2) = param;
 }
 
+ACTION_TEMPLATE(SetTcAttrPointee,
+                HAS_1_TEMPLATE_PARAMS(typename, T),
+                AND_1_VALUE_PARAMS(param))
+{
+    *static_cast<T*>(arg1) = param;
+}
+
 MATCHER_P(ModeUsesSignal, sig, "")
 {
     auto vtm = static_cast<vt_mode*>(arg);
@@ -90,7 +99,9 @@
     LinuxVirtualTerminalTest()
         : fake_vt_fd{5},
           fake_kd_mode{KD_TEXT},
-          fake_vt_mode{VT_AUTO, 0, 0, 0, 0}
+          fake_vt_mode{VT_AUTO, 0, 0, 0, 0},
+          fake_kb_mode{K_RAW},
+          fake_tc_attr()
     {
     }
 
@@ -142,6 +153,14 @@
             .WillOnce(DoAll(SetIoctlPointee<int>(fake_kd_mode), Return(0)));
         EXPECT_CALL(mock_fops, ioctl(fake_vt_fd, VT_GETMODE, An<void*>()))
             .WillOnce(DoAll(SetIoctlPointee<vt_mode>(fake_vt_mode), Return(0)));
+        EXPECT_CALL(mock_fops, ioctl(fake_vt_fd, KDGKBMODE, An<void*>()))
+            .WillOnce(DoAll(SetIoctlPointee<int>(fake_kb_mode), Return(0)));
+        EXPECT_CALL(mock_fops, ioctl(fake_vt_fd, KDSKBMODE, K_OFF))
+            .WillOnce(Return(0));
+        EXPECT_CALL(mock_fops, tcgetattr(fake_vt_fd, An<struct termios *>()))
+            .WillOnce(DoAll(SetTcAttrPointee<struct termios>(fake_tc_attr), Return(0)));
+        EXPECT_CALL(mock_fops, tcsetattr(fake_vt_fd, TCSANOW, An<const struct termios *>()))
+            .WillOnce(Return(0));
     }
 
     void set_up_expectations_for_switch_handler(int sig)
@@ -158,9 +177,13 @@
     {
         using namespace testing;
 
+        EXPECT_CALL(mock_fops, tcsetattr(fake_vt_fd, TCSANOW, An<const struct termios *>()))
+            .WillOnce(Return(0));
+        EXPECT_CALL(mock_fops, ioctl(fake_vt_fd, KDSKBMODE, fake_kb_mode))
+            .WillOnce(Return(0));
         EXPECT_CALL(mock_fops, ioctl(fake_vt_fd, KDSETMODE, fake_kd_mode))
             .WillOnce(Return(0));
-        EXPECT_CALL(mock_fops, ioctl(fake_vt_fd, VT_SETMODE,An<void*>()))
+        EXPECT_CALL(mock_fops, ioctl(fake_vt_fd, VT_SETMODE, An<void*>()))
             .WillOnce(Return(0));
         EXPECT_CALL(mock_fops, close(fake_vt_fd))
             .WillOnce(Return(0));
@@ -169,6 +192,8 @@
     int const fake_vt_fd;
     int const fake_kd_mode;
     vt_mode fake_vt_mode;
+    int const fake_kb_mode;
+    struct termios fake_tc_attr;
     std::function<void(int)> sig_handler;
     MockVTFileOperations mock_fops;
     MockMainLoop mock_main_loop;