Merge lp:~attente/mir/1634508 into lp:mir

Proposed by William Hua on 2016-10-18
Status: Merged
Approved by: Alan Griffiths on 2016-10-19
Approved revision: 3762
Merged at revision: 3773
Proposed branch: lp:~attente/mir/1634508
Merge into: lp:mir
Diff against target: 12 lines (+1/-1)
1 file modified
src/client/mir_connection_api.cpp (+1/-1)
To merge this branch: bzr merge lp:~attente/mir/1634508
Reviewer Review Type Date Requested Status
Mir CI Bot continuous-integration Approve on 2016-10-19
Alan Griffiths Approve on 2016-10-19
Chris Halse Rogers Abstain on 2016-10-19
Cemil Azizoglu (community) 2016-10-18 Approve on 2016-10-18
Review via email: mp+308730@code.launchpad.net

Commit message

Use the default socket location if MIR_SOCKET is "".
(LP: #1634508)

Description of the change

Use the default socket location if MIR_SOCKET is "".

To post a comment you must log in.
Cemil Azizoglu (cemil-azizoglu) wrote :

Makes sense

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

*Needs Discussion*

I'd prefer adding the endpoint name to the "Failed to connect" error message. That will help with other stupid mistakes specifying the endpoint.

Amusingly, mir_demo_client_basic seems to be the only place we do that!

review: Needs Information
Chris Halse Rogers (raof) wrote :

I'm +1 for making the error message more useful; I think this would be more than just the endpoint name we tried to connect to, but also the source of that name (default, environment variable, command-line parameter).

I'm +0 on this; option validation is not unsensible, but the only *correct* validation is provided by the result of connect().

review: Abstain
William Hua (attente) wrote :

The unity8 upstart script will never be able to sanitize the environment properly because dbus-update-activation-environment is unable to actually unset MIR_SOCKET after it has been set once (which I guess is why it sets it to "" causing the bug in the first place). You could improve the error message, but there is no other way to prevent this particular problem other than to make the mir client library more robust in its interpretation of MIR_SOCKET.

Alan Griffiths (alan-griffiths) wrote :

OK as a workaround

review: Approve
Chris Halse Rogers (raof) wrote :

Is there a reason why we don't just fix unity8's upstart script?
Neither X11 nor Wayland handle having DISPLAY="" or WAYLAND_DISPLAY="".

Iain Lane (laney) wrote :

DBus UpdateActivationEnvironment doesn't let you unset variables completely, and Unity8's script messes with MIR_SOCKET, so it can only set it to an empty variable.

If there were a case in hand where this broke for X11 or Wayland I think those would want to be fixed too.

Or DBus could be, but it would have to be a new API since UAE takes an a{ss} (Variable → Value) so a longer term thing.

Mir CI Bot (mir-ci-bot) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/client/mir_connection_api.cpp'
2--- src/client/mir_connection_api.cpp 2016-10-12 06:03:15 +0000
3+++ src/client/mir_connection_api.cpp 2016-10-18 13:40:08 +0000
4@@ -64,7 +64,7 @@
5 else
6 {
7 auto socket_env = getenv("MIR_SOCKET");
8- if (socket_env)
9+ if (socket_env && socket_env[0])
10 sock = socket_env;
11 else
12 sock = mir::default_server_socket;


People subscribed via source and target branches