Mir

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

Proposed by Maarten Lankhorst on 2013-07-31
Status: Merged
Approved by: Daniel van Vugt on 2013-08-08
Approved revision: 900
Merged at revision: 942
Proposed branch: lp:~mlankhorst/mir/setsid
Merge into: lp:~mir-team/mir/trunk
Diff against target: 217 lines (+97/-4)
4 files modified
src/server/graphics/gbm/gbm_platform.cpp (+10/-0)
src/server/graphics/gbm/linux_virtual_terminal.cpp (+55/-2)
src/server/graphics/gbm/linux_virtual_terminal.h (+5/-0)
tests/unit-tests/graphics/gbm/test_linux_virtual_terminal.cpp (+27/-2)
To merge this branch: bzr merge lp:~mlankhorst/mir/setsid
Reviewer Review Type Date Requested Status
Daniel van Vugt Approve on 2013-08-08
Chris Halse Rogers Approve on 2013-08-08
Robert Ancell 2013-07-31 Approve on 2013-08-08
Alexandros Frantzis (community) Needs Information on 2013-07-31
Alan Griffiths Abstain on 2013-07-31
PS Jenkins bot (community) continuous-integration Approve on 2013-07-31
Review via email: mp+177800@code.launchpad.net

Commit message

Re-introduce console support, and ignore control characters

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :

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)
Robert Ancell (robert-ancell) wrote :

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

review: Approve
Alan Griffiths (alan-griffiths) wrote :

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

review: Abstain
Alexandros Frantzis (afrantzis) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

> 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) wrote :

> 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) wrote :

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) wrote :

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) wrote :

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) wrote :
Daniel van Vugt (vanvugt) wrote :

Robert,

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

Robert Ancell (robert-ancell) wrote :

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.

review: Approve
Chris Halse Rogers (raof) wrote :

In conjunction with vt-switch-keys, +1

review: Approve
Daniel van Vugt (vanvugt) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/server/graphics/gbm/gbm_platform.cpp'
2--- src/server/graphics/gbm/gbm_platform.cpp 2013-07-30 13:56:22 +0000
3+++ src/server/graphics/gbm/gbm_platform.cpp 2013-07-31 10:50:37 +0000
4@@ -78,6 +78,16 @@
5 {
6 return ::ioctl(d, request, p_val);
7 }
8+
9+ int tcsetattr(int d, int acts, const struct termios *tcattr)
10+ {
11+ return ::tcsetattr(d, acts, tcattr);
12+ }
13+
14+ int tcgetattr(int d, struct termios *tcattr)
15+ {
16+ return ::tcgetattr(d, tcattr);
17+ }
18 };
19
20 }
21
22=== modified file 'src/server/graphics/gbm/linux_virtual_terminal.cpp'
23--- src/server/graphics/gbm/linux_virtual_terminal.cpp 2013-07-30 16:19:14 +0000
24+++ src/server/graphics/gbm/linux_virtual_terminal.cpp 2013-07-31 10:50:37 +0000
25@@ -45,8 +45,11 @@
26 vt_fd{fops, open_vt(vt_number)},
27 prev_kd_mode{0},
28 prev_vt_mode(),
29+ prev_tty_mode(),
30+ prev_tcattr(),
31 active{true}
32 {
33+ struct termios tcattr;
34 if (fops->ioctl(vt_fd.fd(), KDGETMODE, &prev_kd_mode) < 0)
35 {
36 BOOST_THROW_EXCEPTION(
37@@ -59,14 +62,44 @@
38 {
39 BOOST_THROW_EXCEPTION(
40 boost::enable_error_info(
41- std::runtime_error("Failed to get the current VT")) << boost::errinfo_errno(errno));
42- }
43+ std::runtime_error("Failed to get the current VT"))
44+ << boost::errinfo_errno(errno));
45+ }
46+
47+ if (fops->ioctl(vt_fd.fd(), KDGKBMODE, &prev_tty_mode) < 0)
48+ {
49+ BOOST_THROW_EXCEPTION(
50+ boost::enable_error_info(
51+ std::runtime_error("Failed to get the current TTY mode"))
52+ << boost::errinfo_errno(errno));
53+ }
54+ if (fops->ioctl(vt_fd.fd(), KDSKBMODE, K_OFF) < 0)
55+ {
56+ BOOST_THROW_EXCEPTION(
57+ boost::enable_error_info(
58+ std::runtime_error("Failed to mute keyboard"))
59+ << boost::errinfo_errno(errno));
60+ }
61+
62+ fops->tcgetattr(vt_fd.fd(), &prev_tcattr);
63+ tcattr = prev_tcattr;
64+ tcattr.c_iflag = IGNPAR | IGNBRK;
65+ cfsetispeed(&tcattr, B9600);
66+ tcattr.c_oflag = 0;
67+ cfsetospeed(&tcattr, B9600);
68+ tcattr.c_cflag = CREAD | CS8;
69+ tcattr.c_lflag = 0;
70+ tcattr.c_cc[VTIME] = 0;
71+ tcattr.c_cc[VMIN] = 1;
72+ fops->tcsetattr(vt_fd.fd(), TCSANOW, &tcattr);
73 }
74
75 mgg::LinuxVirtualTerminal::~LinuxVirtualTerminal() noexcept(true)
76 {
77 if (vt_fd.fd() > 0)
78 {
79+ fops->tcsetattr(vt_fd.fd(), TCSANOW, &prev_tcattr);
80+ fops->ioctl(vt_fd.fd(), KDSKBMODE, prev_tty_mode);
81 fops->ioctl(vt_fd.fd(), KDSETMODE, prev_kd_mode);
82 fops->ioctl(vt_fd.fd(), VT_SETMODE, &prev_vt_mode);
83 }
84@@ -186,6 +219,26 @@
85
86 std::string const active_vt_path{vt_path_stream.str()};
87
88+ if (activate)
89+ {
90+ if (getpid() == getpgid(0) && setpgid(0, getpgid(getppid())) < 0)
91+ {
92+ BOOST_THROW_EXCEPTION(
93+ boost::enable_error_info(
94+ std::runtime_error("Failed to stop being a process group"))
95+ << boost::errinfo_errno(errno));
96+ }
97+
98+ /* become process group leader */
99+ if (setsid() < 0)
100+ {
101+ BOOST_THROW_EXCEPTION(
102+ boost::enable_error_info(
103+ std::runtime_error("Failed to become session leader"))
104+ << boost::errinfo_errno(errno));
105+ }
106+ }
107+
108 auto vt_fd = fops->open(active_vt_path.c_str(), O_RDONLY | O_NDELAY);
109
110 if (vt_fd < 0)
111
112=== modified file 'src/server/graphics/gbm/linux_virtual_terminal.h'
113--- src/server/graphics/gbm/linux_virtual_terminal.h 2013-06-20 02:16:09 +0000
114+++ src/server/graphics/gbm/linux_virtual_terminal.h 2013-07-31 10:50:37 +0000
115@@ -24,6 +24,7 @@
116 #include <memory>
117
118 #include <linux/vt.h>
119+#include <termios.h>
120 #include <unistd.h>
121
122 namespace mir
123@@ -45,6 +46,8 @@
124 virtual int close(int fd) = 0;
125 virtual int ioctl(int d, int request, int val) = 0;
126 virtual int ioctl(int d, int request, void* p_val) = 0;
127+ virtual int tcsetattr(int d, int acts, const struct termios *tcattr) = 0;
128+ virtual int tcgetattr(int d, struct termios *tcattr) = 0;
129
130 protected:
131 VTFileOperations() = default;
132@@ -90,6 +93,8 @@
133 FDWrapper const vt_fd;
134 int prev_kd_mode;
135 struct vt_mode prev_vt_mode;
136+ int prev_tty_mode;
137+ struct termios prev_tcattr;
138 bool active;
139 };
140
141
142=== modified file 'tests/unit-tests/graphics/gbm/test_linux_virtual_terminal.cpp'
143--- tests/unit-tests/graphics/gbm/test_linux_virtual_terminal.cpp 2013-06-28 15:16:39 +0000
144+++ tests/unit-tests/graphics/gbm/test_linux_virtual_terminal.cpp 2013-07-31 10:50:37 +0000
145@@ -47,6 +47,8 @@
146 MOCK_METHOD1(close, int(int));
147 MOCK_METHOD3(ioctl, int(int, int, int));
148 MOCK_METHOD3(ioctl, int(int, int, void*));
149+ MOCK_METHOD3(tcsetattr, int(int, int, const struct termios*));
150+ MOCK_METHOD2(tcgetattr, int(int, struct termios*));
151 };
152
153 class MockMainLoop : public mir::MainLoop
154@@ -73,6 +75,13 @@
155 *static_cast<T*>(arg2) = param;
156 }
157
158+ACTION_TEMPLATE(SetTcAttrPointee,
159+ HAS_1_TEMPLATE_PARAMS(typename, T),
160+ AND_1_VALUE_PARAMS(param))
161+{
162+ *static_cast<T*>(arg1) = param;
163+}
164+
165 MATCHER_P(ModeUsesSignal, sig, "")
166 {
167 auto vtm = static_cast<vt_mode*>(arg);
168@@ -90,7 +99,9 @@
169 LinuxVirtualTerminalTest()
170 : fake_vt_fd{5},
171 fake_kd_mode{KD_TEXT},
172- fake_vt_mode{VT_AUTO, 0, 0, 0, 0}
173+ fake_vt_mode{VT_AUTO, 0, 0, 0, 0},
174+ fake_kb_mode{K_RAW},
175+ fake_tc_attr()
176 {
177 }
178
179@@ -142,6 +153,14 @@
180 .WillOnce(DoAll(SetIoctlPointee<int>(fake_kd_mode), Return(0)));
181 EXPECT_CALL(mock_fops, ioctl(fake_vt_fd, VT_GETMODE, An<void*>()))
182 .WillOnce(DoAll(SetIoctlPointee<vt_mode>(fake_vt_mode), Return(0)));
183+ EXPECT_CALL(mock_fops, ioctl(fake_vt_fd, KDGKBMODE, An<void*>()))
184+ .WillOnce(DoAll(SetIoctlPointee<int>(fake_kb_mode), Return(0)));
185+ EXPECT_CALL(mock_fops, ioctl(fake_vt_fd, KDSKBMODE, K_OFF))
186+ .WillOnce(Return(0));
187+ EXPECT_CALL(mock_fops, tcgetattr(fake_vt_fd, An<struct termios *>()))
188+ .WillOnce(DoAll(SetTcAttrPointee<struct termios>(fake_tc_attr), Return(0)));
189+ EXPECT_CALL(mock_fops, tcsetattr(fake_vt_fd, TCSANOW, An<const struct termios *>()))
190+ .WillOnce(Return(0));
191 }
192
193 void set_up_expectations_for_switch_handler(int sig)
194@@ -158,9 +177,13 @@
195 {
196 using namespace testing;
197
198+ EXPECT_CALL(mock_fops, tcsetattr(fake_vt_fd, TCSANOW, An<const struct termios *>()))
199+ .WillOnce(Return(0));
200+ EXPECT_CALL(mock_fops, ioctl(fake_vt_fd, KDSKBMODE, fake_kb_mode))
201+ .WillOnce(Return(0));
202 EXPECT_CALL(mock_fops, ioctl(fake_vt_fd, KDSETMODE, fake_kd_mode))
203 .WillOnce(Return(0));
204- EXPECT_CALL(mock_fops, ioctl(fake_vt_fd, VT_SETMODE,An<void*>()))
205+ EXPECT_CALL(mock_fops, ioctl(fake_vt_fd, VT_SETMODE, An<void*>()))
206 .WillOnce(Return(0));
207 EXPECT_CALL(mock_fops, close(fake_vt_fd))
208 .WillOnce(Return(0));
209@@ -169,6 +192,8 @@
210 int const fake_vt_fd;
211 int const fake_kd_mode;
212 vt_mode fake_vt_mode;
213+ int const fake_kb_mode;
214+ struct termios fake_tc_attr;
215 std::function<void(int)> sig_handler;
216 MockVTFileOperations mock_fops;
217 MockMainLoop mock_main_loop;

Subscribers

People subscribed via source and target branches