Re: [systemd-devel] [PATCH 2/3] logind: session: set_controller should fail if prepare_vt fails

2014-08-11 Thread David Herrmann 
Hi

On Fri, Aug 8, 2014 at 8:45 PM, Olivier Brunel j...@jjacky.com wrote:
 If controllers can expect logind to have prepared the VT (e.g. set it to
 graphics mode, etc) then TakeControl() should fail if said preparation
 failed (and session_restore_vt() was called).
 ---
  src/login/logind-session.c | 15 +--
  src/login/logind-session.h |  2 +-
  2 files changed, 10 insertions(+), 7 deletions(-)

 diff --git a/src/login/logind-session.c b/src/login/logind-session.c
 index 905e73f..3f4e177 100644
 --- a/src/login/logind-session.c
 +++ b/src/login/logind-session.c
 @@ -1003,14 +1003,14 @@ static int session_vt_fn(sd_event_source *source, 
 const struct signalfd_siginfo
  return 0;
  }

 -void session_prepare_vt(Session *s) {
 +int session_prepare_vt(Session *s) {
  int vt, r;
  struct vt_mode mode = { 0 };
  sigset_t mask;

  vt = session_open_vt(s);
  if (vt  0)
 -return;
 +return vt;

This doesn't work. session_open_vt() returns -1 if the session has no
VT attached. You need to return 0 in that case, though:

if (s-vtnr  1)
return 0;
vt = session_open_vt(s);
if (vt  0)
return vt;

Thanks
David


  r = fchown(vt, s-user-uid, -1);
  if (r  0)
 @@ -1042,11 +1042,12 @@ void session_prepare_vt(Session *s) {
  if (r  0)
  goto error;

 -return;
 +return 0;

  error:
  log_error(cannot mute VT %u for session %s (%d/%d), s-vtnr, 
 s-id, r, errno);
  session_restore_vt(s);
 +return r;
  }

  void session_restore_vt(Session *s) {
 @@ -1123,8 +1124,6 @@ int session_set_controller(Session *s, const char 
 *sender, bool force) {
  return r;
  }

 -session_swap_controller(s, t);
 -
  /* When setting a session controller, we forcibly mute the VT and set
   * it into graphics-mode. Applications can override that by changing
   * VT state after calling TakeControl(). However, this serves as a 
 good
 @@ -1133,7 +1132,11 @@ int session_set_controller(Session *s, const char 
 *sender, bool force) {
   * exits.
   * If logind crashes/restarts, we restore the controller during 
 restart
   * or reset the VT in case it crashed/exited, too. */
 -session_prepare_vt(s);
 +r = session_prepare_vt(s);
 +if (r  0)
 +return r;
 +
 +session_swap_controller(s, t);

  return 0;
  }
 diff --git a/src/login/logind-session.h b/src/login/logind-session.h
 index e62b76d..2ab3182 100644
 --- a/src/login/logind-session.h
 +++ b/src/login/logind-session.h
 @@ -172,7 +172,7 @@ SessionClass session_class_from_string(const char *s) 
 _pure_;
  const char *kill_who_to_string(KillWho k) _const_;
  KillWho kill_who_from_string(const char *s) _pure_;

 -void session_prepare_vt(Session *s);
 +int session_prepare_vt(Session *s);
  void session_restore_vt(Session *s);

  bool session_is_controller(Session *s, const char *sender);
 --
 2.0.4

 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/3] logind: session: set_controller should fail if prepare_vt fails

2014-08-11 Thread Lennart Poettering 
On Mon, 11.08.14 17:17, David Herrmann (dh.herrm...@gmail.com) wrote:

 
 Hi
 
 On Fri, Aug 8, 2014 at 8:45 PM, Olivier Brunel j...@jjacky.com wrote:
  If controllers can expect logind to have prepared the VT (e.g. set it to
  graphics mode, etc) then TakeControl() should fail if said preparation
  failed (and session_restore_vt() was called).
  ---
   src/login/logind-session.c | 15 +--
   src/login/logind-session.h |  2 +-
   2 files changed, 10 insertions(+), 7 deletions(-)
 
  diff --git a/src/login/logind-session.c b/src/login/logind-session.c
  index 905e73f..3f4e177 100644
  --- a/src/login/logind-session.c
  +++ b/src/login/logind-session.c
  @@ -1003,14 +1003,14 @@ static int session_vt_fn(sd_event_source *source, 
  const struct signalfd_siginfo
   return 0;
   }
 
  -void session_prepare_vt(Session *s) {
  +int session_prepare_vt(Session *s) {
   int vt, r;
   struct vt_mode mode = { 0 };
   sigset_t mask;
 
   vt = session_open_vt(s);
   if (vt  0)
  -return;
  +return vt;
 
 This doesn't work. session_open_vt() returns -1 if the session has no

Humm. David, can we please stick to returning errors as negative errno
values, and not make up -1 as error in some cases? Please uniformly
only return negative errno codes...

So, please session_open_vt() should be fixed to return -EBUSY or -EINVAL
or something, but not -1...

And in that case Olivier's patch looks sane to me...

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/3] logind: session: set_controller should fail if prepare_vt fails

2014-08-11 Thread David Herrmann 
Hi

On Mon, Aug 11, 2014 at 6:13 PM, Lennart Poettering
lenn...@poettering.net wrote:
 On Mon, 11.08.14 17:17, David Herrmann (dh.herrm...@gmail.com) wrote:


 Hi

 On Fri, Aug 8, 2014 at 8:45 PM, Olivier Brunel j...@jjacky.com wrote:
  If controllers can expect logind to have prepared the VT (e.g. set it to
  graphics mode, etc) then TakeControl() should fail if said preparation
  failed (and session_restore_vt() was called).
  ---
   src/login/logind-session.c | 15 +--
   src/login/logind-session.h |  2 +-
   2 files changed, 10 insertions(+), 7 deletions(-)
 
  diff --git a/src/login/logind-session.c b/src/login/logind-session.c
  index 905e73f..3f4e177 100644
  --- a/src/login/logind-session.c
  +++ b/src/login/logind-session.c
  @@ -1003,14 +1003,14 @@ static int session_vt_fn(sd_event_source *source, 
  const struct signalfd_siginfo
   return 0;
   }
 
  -void session_prepare_vt(Session *s) {
  +int session_prepare_vt(Session *s) {
   int vt, r;
   struct vt_mode mode = { 0 };
   sigset_t mask;
 
   vt = session_open_vt(s);
   if (vt  0)
  -return;
  +return vt;

 This doesn't work. session_open_vt() returns -1 if the session has no

 Humm. David, can we please stick to returning errors as negative errno
 values, and not make up -1 as error in some cases? Please uniformly
 only return negative errno codes...

 So, please session_open_vt() should be fixed to return -EBUSY or -EINVAL
 or something, but not -1...

Old legacy.. I fixed it up, changed the !CONFIG_VT case and applied the patch.

Thanks
David
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH 2/3] logind: session: set_controller should fail if prepare_vt fails

2014-08-08 Thread Olivier Brunel 
If controllers can expect logind to have prepared the VT (e.g. set it to
graphics mode, etc) then TakeControl() should fail if said preparation
failed (and session_restore_vt() was called).
---
 src/login/logind-session.c | 15 +--
 src/login/logind-session.h |  2 +-
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/src/login/logind-session.c b/src/login/logind-session.c
index 905e73f..3f4e177 100644
--- a/src/login/logind-session.c
+++ b/src/login/logind-session.c
@@ -1003,14 +1003,14 @@ static int session_vt_fn(sd_event_source *source, const 
struct signalfd_siginfo
 return 0;
 }
 
-void session_prepare_vt(Session *s) {
+int session_prepare_vt(Session *s) {
 int vt, r;
 struct vt_mode mode = { 0 };
 sigset_t mask;
 
 vt = session_open_vt(s);
 if (vt  0)
-return;
+return vt;
 
 r = fchown(vt, s-user-uid, -1);
 if (r  0)
@@ -1042,11 +1042,12 @@ void session_prepare_vt(Session *s) {
 if (r  0)
 goto error;
 
-return;
+return 0;
 
 error:
 log_error(cannot mute VT %u for session %s (%d/%d), s-vtnr, s-id, 
r, errno);
 session_restore_vt(s);
+return r;
 }
 
 void session_restore_vt(Session *s) {
@@ -1123,8 +1124,6 @@ int session_set_controller(Session *s, const char 
*sender, bool force) {
 return r;
 }
 
-session_swap_controller(s, t);
-
 /* When setting a session controller, we forcibly mute the VT and set
  * it into graphics-mode. Applications can override that by changing
  * VT state after calling TakeControl(). However, this serves as a good
@@ -1133,7 +1132,11 @@ int session_set_controller(Session *s, const char 
*sender, bool force) {
  * exits.
  * If logind crashes/restarts, we restore the controller during restart
  * or reset the VT in case it crashed/exited, too. */
-session_prepare_vt(s);
+r = session_prepare_vt(s);
+if (r  0)
+return r;
+
+session_swap_controller(s, t);
 
 return 0;
 }
diff --git a/src/login/logind-session.h b/src/login/logind-session.h
index e62b76d..2ab3182 100644
--- a/src/login/logind-session.h
+++ b/src/login/logind-session.h
@@ -172,7 +172,7 @@ SessionClass session_class_from_string(const char *s) 
_pure_;
 const char *kill_who_to_string(KillWho k) _const_;
 KillWho kill_who_from_string(const char *s) _pure_;
 
-void session_prepare_vt(Session *s);
+int session_prepare_vt(Session *s);
 void session_restore_vt(Session *s);
 
 bool session_is_controller(Session *s, const char *sender);
-- 
2.0.4

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel