Re: [systemd-devel] bpfilter blocks root unmount during shutdown

2018-09-24 Thread Olivier Brunel 
On Mon, 24 Sep 2018 15:20:47 +0200
Lennart Poettering  wrote:

> On So, 23.09.18 10:38, Andrei Borzenkov (arvidj...@gmail.com) wrote:
> 
> > Dracut /shutdown script first tries to kill all processes still
> > running off old root. Unfortunately this fails for special user
> > process that runs bpfilter because it does not include reference
> > to /oldroot in places where dracut looks for in
> > kilall_proc_mountpoint()  
> 
> Hmm, when we invoke the /shutdown executable we already executed our
> process killing spree as part of systemd-shutdown. How come your
> processes even survive that long? What am I missing?

I believe it's because the bpfilter helper process is identified as a
kernel thread - since it has an empty command line - and therefore not
killed.

I personally feel this is a bug (in the kernel), but apparently
this whole bpfilter thing isn't quite ready yet and shouldn't be
used for the moment -- so hopefully it'll improve/be fixed in the mean
time.
You can see this thread[1] about the issue.

Cheers,



[1] https://www.spinics.net/lists/netdev/msg520030.html
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] Fail to reset-failed as user

2015-02-14 Thread Olivier Brunel 
On 02/11/15 21:13, Lennart Poettering wrote:
 On Thu, 05.02.15 19:20, Olivier Brunel (j...@jjacky.com) wrote:
 
 On 02/03/15 22:17, Lennart Poettering wrote:
 On Fri, 12.12.14 16:06, Olivier Brunel (j...@jjacky.com) wrote:

 Sorry for resurrecting this old thread this late. Is this still an
 issue? Does this work on current git?

 Still an issue w/ 218 yes, haven't actually had time to try with current
 git. I'll try to do that over the weekend.

 Today I had one unit in failed state, and after taking care of things I
 wanted to simply reset its state (to inactive) w/out having to start it.

 Looking up the man page, I see there's a command reset-failed for this
 exact purpose, awesome. So I go:

 % systemctl reset-failed backups2.service
 Failed to reset failed state of unit backups2.service: No such device or
 address

 Hmm, did you issue this from some weird environment (su/sudo context,
 from a system service context or so?)

 If this is still an issue, could you try to reproduce this after
 issuing systemd-analyze set-log-level debug? Then please attach the
 log output this generates!

 Meanwhile, this is what I get today: http://ix.io/gaR
 This is not from some weird environment no (or, not that I'm aware of),
 but an (almost) up-to-date Arch Linux x64, systemd 218.
 
 Puzzled. Don't see how this can happen. Also, works fine here...
 
 If you can reproduce this on git, it would be good to gdb this. For
 that:
 
 a) start gdb, type attach 1, to attach to PID 1
 
 b) add a breakpoint on method_reset_failed_unit, by issuing b
method_reset_failed_unit
 
 c) Continue execution of PID 1, by typing in the line c
 
 d) trigger the issue, gdb should break at that instant. 
 
 e) now, check which call fails by stepping through the function with
n. As soon as the function is left, use c to continue
execution. Not that the function will be executed twice, one after
the other. The first invocation will be without PolicyKit privs,
the second one with PolicyKit privs. The second invocation is the
interesting one. Check why it exits non-zero, and whether
unit_reset_failed() is invoked at all (which actually does the
inetersting work).
 
 f) post your findings here

Alright so I did some testing, here's what I found:

- on that second invocation, method_reset_failed_unit() fails from its
call to bus_unit_method_reset_failed(), and that comes down to
bus_message_enter_struct() returning -ENXIO.

- I don't know how this whole thing is supposed to work, but what I
noticed is that bus_message_enter_struct() is called twice from
method_reset_failed_unit(), once from bus_verify_manage_unit_async() and
then from bus_unit_method_reset_failed(). Details as follow:

First, when bus_verify_manage_unit_async() is called:

#0  bus_message_enter_struct (m=0x7f5fb0cb88b0, c=0x7f5fb0cb8aa0,
contents=0x7f5faef0d152 bba{ss}, item_size=0x7fffcebd4928,
offsets=0x7fffcebd4918,
n_offsets=0x7fffcebd4920) at src/libsystemd/sd-bus/bus-message.c:3865
#1  0x7f5faee80136 in sd_bus_message_enter_container
(m=0x7f5fb0cb88b0, type=114 'r',
contents=0x7f5faef0d152 bba{ss}) at
src/libsystemd/sd-bus/bus-message.c:4012
#2  0x7f5faee8e00d in bus_verify_polkit_async (call=0x7f5fb0ca59a0,
capability=21,
action=0x7f5faeef05f8 org.freedesktop.systemd1.manage-units,
interactive=false,
registry=0x7f5fb0c0a890, error=0x7fffcebd4ad0) at
src/libsystemd/sd-bus/bus-util.c:374
#3  0x7f5faee0aa00 in bus_verify_manage_unit_async
(m=0x7f5fb0c0a460, call=0x7f5fb0ca59a0,
error=0x7fffcebd4ad0) at src/core/dbus.c:1196
#4  0x7f5faee0c801 in method_reset_failed_unit (bus=0x7f5fb0ca32f0,
message=0x7f5fb0ca59a0,
userdata=0x7f5fb0c0a460, error=0x7fffcebd4ad0) at
src/core/dbus-manager.c:574

(gdb) p *c
$38 = {enclosing = 0 '\000', need_offsets = false, index = 0,
saved_index = 0,
  signature = 0x7f5fb0c09110 (bba{ss}), before = 0, begin = 0, end =
133, array_size = 0x0,
  offsets = 0x0, n_offsets = 0, offsets_allocated = 0, offset_index = 0,
item_size = 133,
  peeked_signature = 0x0}
(gdb) p contents
$39 = 0x7f5faef0d152 bba{ss}

It eventually returns 1.

Then it gets to called from bus_unit_method_reset_failed():

#0  bus_message_enter_struct (m=0x7f5fb0cb88b0, c=0x7f5fb0cb8250,
contents=0x7f5faef0d152 bba{ss}, item_size=0x7fffcebd48e8,
offsets=0x7fffcebd48d8,
n_offsets=0x7fffcebd48e0) at src/libsystemd/sd-bus/bus-message.c:3865
#1  0x7f5faee80136 in sd_bus_message_enter_container
(m=0x7f5fb0cb88b0, type=114 'r',
contents=0x7f5faef0d152 bba{ss}) at
src/libsystemd/sd-bus/bus-message.c:4012
#2  0x7f5faee8e00d in bus_verify_polkit_async (call=0x7f5fb0ca59a0,
capability=21,
action=0x7f5faeef05f8 org.freedesktop.systemd1.manage-units,
interactive=false,
registry=0x7f5fb0c0a890, error=0x7fffcebd4ad0) at
src/libsystemd/sd-bus/bus-util.c:374
#3  0x7f5faee0aa00 in bus_verify_manage_unit_async
(m=0x7f5fb0c0a460, call=0x7f5fb0ca59a0,
error=0x7fffcebd4ad0) at src/core/dbus.c

Re: [systemd-devel] Fail to reset-failed as user

2015-02-05 Thread Olivier Brunel 
On 02/03/15 22:17, Lennart Poettering wrote:
 On Fri, 12.12.14 16:06, Olivier Brunel (j...@jjacky.com) wrote:
 
 Sorry for resurrecting this old thread this late. Is this still an
 issue? Does this work on current git?

Still an issue w/ 218 yes, haven't actually had time to try with current
git. I'll try to do that over the weekend.

 Today I had one unit in failed state, and after taking care of things I
 wanted to simply reset its state (to inactive) w/out having to start it.

 Looking up the man page, I see there's a command reset-failed for this
 exact purpose, awesome. So I go:

 % systemctl reset-failed backups2.service
 Failed to reset failed state of unit backups2.service: No such device or
 address
 
 Hmm, did you issue this from some weird environment (su/sudo context,
 from a system service context or so?)
 
 If this is still an issue, could you try to reproduce this after
 issuing systemd-analyze set-log-level debug? Then please attach the
 log output this generates!

Meanwhile, this is what I get today: http://ix.io/gaR
This is not from some weird environment no (or, not that I'm aware of),
but an (almost) up-to-date Arch Linux x64, systemd 218.

-j

 Thanks,
 
 Lennart
 

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

[systemd-devel] Fail to reset-failed as user

2014-12-12 Thread Olivier Brunel 
Hi,

Today I had one unit in failed state, and after taking care of things I
wanted to simply reset its state (to inactive) w/out having to start it.

Looking up the man page, I see there's a command reset-failed for this
exact purpose, awesome. So I go:

% systemctl reset-failed backups2.service
Failed to reset failed state of unit backups2.service: No such device or
address

I was nicely asked to authenticate, but then it failed stating the unit
doesn't exist or something (not sure what the error message refers to)?
Now of course said unit does exist:

% systemctl is-failed backups2.service
failed

And I could eventually do it, as root:

% sudo systemctl reset-failed backups2.service

This worked fine and is probably what I would have done had my fingers
not slipped (sc instead of ssc, aliases for `systemctl` and `sudo
systemctl` resp.), but I'm not sure what I missed: since I was properly
authenticated, shouldn't the systemctl call also have worked?

FYI here's what shows up in the journal, confirming the auth:
Dec 12 15:40:00 arch.local polkitd[670]: Operator of unix-session:c3
successfully authenticated as unix-user:jjacky to gain TEMPORARY
authorization for action org.freedesktop.systemd1.manage-units for
system-bus-name::1.259 [systemctl reset-failed backups2.service] (owned
by unix-user:jjacky)

What am I missing/misunderstanding? (or is this a bug?)
Thanks,
-j
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH] journal: Fix navigating backwards missing entries

2014-12-05 Thread Olivier Brunel 
With DIRECTION_UP (i.e. navigating backwards) in generic_array_bisect() when the
needle was found as the last item in the array, it wasn't actually processed as
match, resulting in entries being missed.

https://bugs.freedesktop.org/show_bug.cgi?id=86855
---
This was a good excuse for me to dive in and learn about journal's internals,
but someone with actual knowledge of said internals should review this, in case
I missed something.

 src/journal/journal-file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/journal/journal-file.c b/src/journal/journal-file.c
index 7858435..c5d2d19 100644
--- a/src/journal/journal-file.c
+++ b/src/journal/journal-file.c
@@ -1657,7 +1657,7 @@ static int generic_array_bisect(
 }
 }
 
-if (k  n) {
+if (k = n) {
 if (direction == DIRECTION_UP) {
 i = n;
 subtract_one = true;
-- 
2.1.3

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

Re: [systemd-devel] [PATCH 1/3] logind: session: don't set /dev/ttyX owner to root on restore_vt

2014-08-11 Thread Olivier Brunel 
On 08/11/14 16:25, Lennart Poettering wrote:
 On Fri, 08.08.14 20:45, Olivier Brunel (j...@jjacky.com) wrote:
 
 In session_prepare_vt() we set owner of /dev/ttyX to the user, as that is
 needed for things to work. However, we shouldn't reset it to root on
 session_restore_vt() since it could have in fact already been set to
 the user.
 
 I don't follow here, can't parse this. Could you please elaborate? 

I meant, before the call to session_prepare_vt() the owner of /dev/ttyX
might not be root, but already set to the user. In which case setting it
back to root might not be expected/best.

E.g. if a log in on tty3 right now, /dev/tty3 will be owned by my user,
then if a process try a TakeControl() and it failed (or after it's done)
the ownership would be set to root, even though it wasn't actually root
to begin with.

 
 ---
  src/login/logind-session.c | 2 --
  1 file changed, 2 deletions(-)

 diff --git a/src/login/logind-session.c b/src/login/logind-session.c
 index fdeacb1..905e73f 100644
 --- a/src/login/logind-session.c
 +++ b/src/login/logind-session.c
 @@ -1070,8 +1070,6 @@ void session_restore_vt(Session *s) {
  mode.mode = VT_AUTO;
  ioctl(vt, VT_SETMODE, mode);
  
 -fchown(vt, 0, -1);
 -
  s-vtfd = safe_close(s-vtfd);
  }
  
 
 
 Lennart
 

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

Re: [systemd-devel] [PATCH 3/3] logind: session: Fix not allowing more than one controller

2014-08-11 Thread Olivier Brunel 
On 08/11/14 16:34, Lennart Poettering wrote:
 On Fri, 08.08.14 20:45, Olivier Brunel (j...@jjacky.com) wrote:
 
 While a session can only ever have one controller, there can be more than
 one session with a controller at a time. However, because of the handling
 of SIGUSR1 for handling VT switch, trying to set a controller on a session
 while another session had a controller would fail.
 
 I really don't feel comfortable with using SIGUSR1 for this, anyway I
 must say... SIGUSR1 and SIGUSR2 I think should be left for admins to
 communicate with the daemon for some simple operations, but using this
 internally sounds wrong. Now, the VT is so stupid to require a signa
 handler here, but I think using SIGRTMIN+1 or so might be the better
 choice here?
 
 Now, what makes we wonder here, shouldn't we just install a single
 signal event handler for this when logind initializes, and leaving it on
 until the very end?

Just to note: the problem is that when the signal is called, there's no
way of telling which VT it is about. I think it was only intended for
one process to handle one VT, so there was no question. But if logind
wants to handle more than one VT, to know on which VT to operate then
I'm not sure it would actually be doable unless using a different signal
per VT...

 
 David, could you comment on this, please?
 
 Lennart
 

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

Re: [systemd-devel] [PATCH 1/3] logind: session: don't set /dev/ttyX owner to root on restore_vt

2014-08-11 Thread Olivier Brunel 
On 08/11/14 16:54, Lennart Poettering wrote:
 On Mon, 11.08.14 16:39, Olivier Brunel (j...@jjacky.com) wrote:
 

 On 08/11/14 16:25, Lennart Poettering wrote:
 On Fri, 08.08.14 20:45, Olivier Brunel (j...@jjacky.com) wrote:

 In session_prepare_vt() we set owner of /dev/ttyX to the user, as that is
 needed for things to work. However, we shouldn't reset it to root on
 session_restore_vt() since it could have in fact already been set to
 the user.

 I don't follow here, can't parse this. Could you please elaborate? 

 I meant, before the call to session_prepare_vt() the owner of /dev/ttyX
 might not be root, but already set to the user. In which case setting it
 back to root might not be expected/best.
 
 But that sounds more as if session_restore_vt() should not be used as-is
 as cleanup path for session_prepare_vt(), no?
 
 E.g. if a log in on tty3 right now, /dev/tty3 will be owned by my user,
 then if a process try a TakeControl() and it failed (or after it's done)
 the ownership would be set to root, even though it wasn't actually root
 to begin with.
 
 Isn't this very theoretic? I mean, when does TakeControl() actually
 really fail for you IRL?

Right, this was noticed when trying to start a second rootless X, since
with systemd-215 things currently fail, and the ownership of /dev/ttyX
would then be switched/changed to root.

session_prepare_vt() could also check the owner first, and note what to
reset to on session_restore_vt(), to effectively restore things as they
were.


 
 Lennart
 

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

Re: [systemd-devel] [PATCH 1/3] logind: session: don't set /dev/ttyX owner to root on restore_vt

2014-08-11 Thread Olivier Brunel 
On 08/11/14 17:12, David Herrmann wrote:
 Hi
 
 On Mon, Aug 11, 2014 at 5:05 PM, Olivier Brunel j...@jjacky.com wrote:
 On 08/11/14 16:54, Lennart Poettering wrote:
 On Mon, 11.08.14 16:39, Olivier Brunel (j...@jjacky.com) wrote:


 On 08/11/14 16:25, Lennart Poettering wrote:
 On Fri, 08.08.14 20:45, Olivier Brunel (j...@jjacky.com) wrote:

 In session_prepare_vt() we set owner of /dev/ttyX to the user, as that is
 needed for things to work. However, we shouldn't reset it to root on
 session_restore_vt() since it could have in fact already been set to
 the user.

 I don't follow here, can't parse this. Could you please elaborate?

 I meant, before the call to session_prepare_vt() the owner of /dev/ttyX
 might not be root, but already set to the user. In which case setting it
 back to root might not be expected/best.

 But that sounds more as if session_restore_vt() should not be used as-is
 as cleanup path for session_prepare_vt(), no?

 E.g. if a log in on tty3 right now, /dev/tty3 will be owned by my user,
 then if a process try a TakeControl() and it failed (or after it's done)
 the ownership would be set to root, even though it wasn't actually root
 to begin with.

 Isn't this very theoretic? I mean, when does TakeControl() actually
 really fail for you IRL?

 Right, this was noticed when trying to start a second rootless X, since
 with systemd-215 things currently fail, and the ownership of /dev/ttyX
 would then be switched/changed to root.
 
 Wait, what? Can you please elaborate. Currently, only one process can

Sorry, I meant e.g. having one rootless X on tt1 and starting another
one on tty2. Currently this fails (see other patch/mail), and is how
this was observed.

 be controller at a time, and session_prepare_vt() is called *after*
 the controller is set. Therefore, it is not called when you try
 starting a second controller.
 
 The only race I see is this:
  * start legacy Xserver (which doesn't use TakeControl()) which sets
 user-id on the TTY
  * start new Xserver which uses TakeControl() (and thus calls
 session_prepare_vt())
  * stop new Xserver (thus drop Control and reset the TTY)
 
 In this case, the new xserver will try to start up, but fail as the
 old one is still running. Therefore, it *might* call ReleaseControl()
 and thus reset the TTY. However, you're not supposed to mix both and
 this is not a legitimate use-case. I mean, the old server is root and
 modifies the TTY by itself (without using systemd). Obviously, this is
 racy.
 
 From a systemd-logind perspective, this is the same as if you run
 sudo chown xyz /dev/tty manually.
 
 session_prepare_vt() could also check the owner first, and note what to
 reset to on session_restore_vt(), to effectively restore things as they
 were.
 
 Even though I don't see any problem here, I'd be fine with such a
 patch. Care to send one? Note that you probably need to store that
 information in the session-file (session_save() / session_load()) too.

If you think the current behavior (setting to root on
session_restore_vt()) is fine, I'm fine leaving it as is. Else, I could
work on a patch as described.

 
 Thanks
 David
 

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

Re: [systemd-devel] [PATCH] login: share VT-signal handler between sessions

2014-08-11 Thread Olivier Brunel 
On 08/11/14 18:21, David Herrmann wrote:
 sd-event does not allow multiple handlers for a single signal. However,
 logind sets up signal handlers for each session with VT_PROCESS set (that
 is, it has an active controller). Therefore, registering multiple such
 controllers will fail.
 
 Lets make the VT-handler global, as it's mostly trivial, anyway. This way,
 the sessions don't have to take care of that and we can simply acknowledge
 all VT-switch requests as we always did.
 ---
 Hi Olivier
 
 Can you give this a try? It should solve your issues.

Yes, just tried it -- it does solve the issue.

Thanks guys,
-j

BTW there's a bug report opened about this issue:
https://bugs.freedesktop.org/show_bug.cgi?id=81932

 
 Thanks
 David
 
  src/login/logind-session.c | 26 ++-
  src/login/logind-session.h |  1 -
  src/login/logind.c | 62 
 ++
  3 files changed, 64 insertions(+), 25 deletions(-)
 
 diff --git a/src/login/logind-session.c b/src/login/logind-session.c
 index fdeacb1..26b6a90 100644
 --- a/src/login/logind-session.c
 +++ b/src/login/logind-session.c
 @@ -153,8 +153,6 @@ void session_free(Session *s) {
  
  hashmap_remove(s-manager-sessions, s-id);
  
 -s-vt_source = sd_event_source_unref(s-vt_source);
 -
  free(s-state_file);
  free(s);
  }
 @@ -994,19 +992,9 @@ static int session_open_vt(Session *s) {
  return s-vtfd;
  }
  
 -static int session_vt_fn(sd_event_source *source, const struct 
 signalfd_siginfo *si, void *data) {
 -Session *s = data;
 -
 -if (s-vtfd = 0)
 -ioctl(s-vtfd, VT_RELDISP, 1);
 -
 -return 0;
 -}
 -
  void session_prepare_vt(Session *s) {
  int vt, r;
  struct vt_mode mode = { 0 };
 -sigset_t mask;
  
  vt = session_open_vt(s);
  if (vt  0)
 @@ -1024,20 +1012,12 @@ void session_prepare_vt(Session *s) {
  if (r  0)
  goto error;
  
 -sigemptyset(mask);
 -sigaddset(mask, SIGUSR1);
 -sigprocmask(SIG_BLOCK, mask, NULL);
 -
 -r = sd_event_add_signal(s-manager-event, s-vt_source, SIGUSR1, 
 session_vt_fn, s);
 -if (r  0)
 -goto error;
 -
  /* Oh, thanks to the VT layer, VT_AUTO does not work with 
 KD_GRAPHICS.
   * So we need a dummy handler here which just acknowledges *all* VT
   * switch requests. */
  mode.mode = VT_PROCESS;
 -mode.relsig = SIGUSR1;
 -mode.acqsig = SIGUSR1;
 +mode.relsig = SIGRTMIN;
 +mode.acqsig = SIGRTMIN + 1;
  r = ioctl(vt, VT_SETMODE, mode);
  if (r  0)
  goto error;
 @@ -1058,8 +1038,6 @@ void session_restore_vt(Session *s) {
  if (vt  0)
  return;
  
 -s-vt_source = sd_event_source_unref(s-vt_source);
 -
  ioctl(vt, KDSETMODE, KD_TEXT);
  
  if (read_one_line_file(/sys/module/vt/parameters/default_utf8, 
 utf8) = 0  *utf8 == '1')
 diff --git a/src/login/logind-session.h b/src/login/logind-session.h
 index e62b76d..562332c 100644
 --- a/src/login/logind-session.h
 +++ b/src/login/logind-session.h
 @@ -98,7 +98,6 @@ struct Session {
  Seat *seat;
  unsigned int vtnr;
  int vtfd;
 -sd_event_source *vt_source;
  
  pid_t leader;
  uint32_t audit_id;
 diff --git a/src/login/logind.c b/src/login/logind.c
 index ec5529d..b470916 100644
 --- a/src/login/logind.c
 +++ b/src/login/logind.c
 @@ -720,6 +720,46 @@ static int manager_connect_bus(Manager *m) {
  return 0;
  }
  
 +static int manager_vt_switch(sd_event_source *src, const struct 
 signalfd_siginfo *si, void *data) {
 +Manager *m = data;
 +Session *active, *iter;
 +
 +/*
 + * We got a VT-switch signal and we have to acknowledge it 
 immediately.
 + * Preferably, we'd just use m-seat0-active-vtfd, but 
 unfortunately,
 + * old user-space might run multiple sessions on a single VT, *sigh*.
 + * Therefore, we have to iterate all sessions and find one with a 
 vtfd
 + * on the requested VT.
 + * As only VTs with active controllers have VT_PROCESS set, our 
 current
 + * notion of the active VT might be wrong (for instance if the switch
 + * happens while we setup VT_PROCESS). Therefore, read the current VT
 + * first and then use s-active-vtnr as reference. Note that this is
 + * not racy, as no further VT-switch can happen as long as we're in
 + * synchronous VT_PROCESS mode.
 + */
 +
 +seat_read_active_vt(m-seat0);
 +
 +active = m-seat0-active;
 +if (!active || active-vtnr  1) {
 +log_warning(Received VT_PROCESS signal without a registered 
 session on that VT.);
 +return 0;
 +}
 +
 +if (active-vtfd = 0) {
 +ioctl(active-vtfd, VT_RELDISP, 1);
 +

[systemd-devel] [PATCH 1/3] logind: session: don't set /dev/ttyX owner to root on restore_vt

2014-08-08 Thread Olivier Brunel 
In session_prepare_vt() we set owner of /dev/ttyX to the user, as that is
needed for things to work. However, we shouldn't reset it to root on
session_restore_vt() since it could have in fact already been set to the user.
---
 src/login/logind-session.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/login/logind-session.c b/src/login/logind-session.c
index fdeacb1..905e73f 100644
--- a/src/login/logind-session.c
+++ b/src/login/logind-session.c
@@ -1070,8 +1070,6 @@ void session_restore_vt(Session *s) {
 mode.mode = VT_AUTO;
 ioctl(vt, VT_SETMODE, mode);
 
-fchown(vt, 0, -1);
-
 s-vtfd = safe_close(s-vtfd);
 }
 
-- 
2.0.4

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

[systemd-devel] [PATCH 3/3] logind: session: Fix not allowing more than one controller

2014-08-08 Thread Olivier Brunel 
While a session can only ever have one controller, there can be more than
one session with a controller at a time. However, because of the handling
of SIGUSR1 for handling VT switch, trying to set a controller on a session
while another session had a controller would fail.

E.g. trying to start a rootless X while there's already one running on
another session would fail, as its request to TakeControl() would fail,
because we couldn't set a signal handler for SIGUSR1 (since we've already
one set up for the other session).

When a session goes inactive, we remove the handler (since it cannot be
deactivated anymore), thus allowing to set one up for another session if needed.
Of course, when the session is activated again, we reset the handler.

(Should be noted that our handler might also never be called, if e.g. the
controller did a VT_SETMODE as well, thus handling/receiving the SIGUSR1)

Alas, if there can be two sessions active at the same time (in different
seats) then the second TakeControl() request will fail, for the same
reason. This could probably be fixed by making sd-event support multiple signal
handlers per signal.

https://bugs.freedesktop.org/show_bug.cgi?id=81932
---
Note that when I say trying to start another rootless X would have its
TakeControl() to fail, that's only true with the previous patch. Today it
actually succeeds, but then X fails with a permission denied error when it tries
to access /dev/ttyX, as session_restore_vt() was called and, amongst other
things, the owner was reset to root.


 src/login/logind-seat.c|  9 +
 src/login/logind-session.c | 39 ++-
 src/login/logind-session.h |  2 ++
 3 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/src/login/logind-seat.c b/src/login/logind-seat.c
index 9992195..1fb0d68 100644
--- a/src/login/logind-seat.c
+++ b/src/login/logind-seat.c
@@ -239,6 +239,8 @@ int seat_set_active(Seat *s, Session *session) {
 s-active = session;
 
 if (old_active) {
+if (old_active-controller)
+session_vt_deactivate(old_active);
 session_device_pause_all(old_active);
 session_send_changed(old_active, Active, NULL);
 }
@@ -246,6 +248,13 @@ int seat_set_active(Seat *s, Session *session) {
 seat_apply_acls(s, old_active);
 
 if (session  session-started) {
+if (session-controller) {
+int r;
+
+r = session_vt_activate(session);
+if (r  0)
+log_error(failed to activate VT %u for 
session %s (%d/%d), session-vtnr, session-id, r, errno);
+}
 session_send_changed(session, Active, NULL);
 session_device_resume_all(session);
 }
diff --git a/src/login/logind-session.c b/src/login/logind-session.c
index 3f4e177..7428e6c 100644
--- a/src/login/logind-session.c
+++ b/src/login/logind-session.c
@@ -1003,10 +1003,35 @@ static int session_vt_fn(sd_event_source *source, const 
struct signalfd_siginfo
 return 0;
 }
 
+int session_vt_activate(Session *s) {
+int r;
+sigset_t mask;
+
+sigemptyset(mask);
+sigaddset(mask, SIGUSR1);
+sigprocmask(SIG_BLOCK, mask, NULL);
+
+r = sd_event_add_signal(s-manager-event, s-vt_source, SIGUSR1, 
session_vt_fn, s);
+if (r  0)
+return r;
+
+return 0;
+}
+
+void session_vt_deactivate(Session *s) {
+assert(s);
+
+/* This is called once the session is inactive, and therefore the
+ * handler isn't needed anymore, and will be reset when the session is
+ * activated again.
+ * Meanwhile, it allows to set up another handler for another session 
if
+ * needed. */
+s-vt_source = sd_event_source_unref(s-vt_source);
+}
+
 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)
@@ -1024,17 +1049,13 @@ int session_prepare_vt(Session *s) {
 if (r  0)
 goto error;
 
-sigemptyset(mask);
-sigaddset(mask, SIGUSR1);
-sigprocmask(SIG_BLOCK, mask, NULL);
-
-r = sd_event_add_signal(s-manager-event, s-vt_source, SIGUSR1, 
session_vt_fn, s);
-if (r  0)
-goto error;
-
 /* Oh, thanks to the VT layer, VT_AUTO does not work with KD_GRAPHICS.
  * So we need a dummy handler here which just acknowledges *all* VT
  * switch requests. */
+r = session_vt_activate(s);
+if (r  0)
+goto error;
+
 mode.mode = VT_PROCESS;
 mode.relsig = SIGUSR1;
 mode.acqsig = SIGUSR1;
diff --git a/src/login/logind-session.h b/src/login/logind-session.h
index 2ab3182..5ad2f26 100644
--- a/src/login/logind-session.h
+++ b/src/login/logind-session.h

[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

Re: [systemd-devel] [PATCH 0/3] Fix issues re: visibility of status messages

2013-11-14 Thread Olivier Brunel 
On 11/14/13 05:40, Zbigniew Jędrzejewski-Szmek wrote:
 On Fri, Sep 20, 2013 at 10:18:27PM +0200, Olivier Brunel wrote:
 Hi,

 I'm running Arch Linux, have been using systemd-204, and recently tried the 
 new
 207 release, and I have been having some issues with it. One was that status
 messages would just stop at some point near the end of the boot process, and
 also that I wouldn't get any during a shutdown/reboot.

 It might be useful to note that I don't start a getty on tty1, which is why I
 expect to see all status messages until default target is reached, even after
 the getty/login has been started (which happens on tty2).

 After looking into it, I came up with the following patches to fix the issue.
 The reason status messages would stop was that the getty was started, and
 systemd then stopped using the console to avoid collisions w/ gettys.

 However, as I said I don't have a getty started on tty1 so for me that is a 
 bug,
 as there's no reason not to keep printing status messages on tty1.

 The lack of messages on shutdown/reboot was also linked to this, because if
 no_console_output was set to true during boot, it'd stay there and prevent
 messages to show up on shutdown.

 To fix this (in the event it was set to true on boot) a patch simply resets 
 it
 to false on job_shutdown_magic(), but I'm not exactly sure if that's the 
 right
 way to do this.
 All 3 patches applied. I *think* they are all correct, but this code
 has so many corner cases that it's hard to be sure. I made some
 tweaks, please check that it still works. Sorry for the delay. In the
 future, if you don't get an answer within a week or two, please holler :)
 Patches do sometimes slip through, especially when there are a lot
 of changes like recently, and a ping to the ml will help to bring the
 thread to the bottom. 

Noted, thanks. Tried the latest git, it all works as expected.

 
 FYI I should add that in a similar setup as the one I described, this will 
 not
 be enough to keep messages on tty1, since fsck's units are now 
 RemainAfterExit
 (see https://bugs.freedesktop.org/show_bug.cgi?id=66784), which means they're
 seen by systemd as owning the console (as far as outputing messages there 
 is
 concerned I mean), and it will therefore stop printing status messages.

 I'm not sure you want to fix this, as it might be only a cosmetic issue 
 for a
 small usecase hence not worth the trouble, so I've simply undone it using a
 .conf file on my end, figured I should mention it though.
 Hm, we could detect this case by looking at services in the SERVICE_EXITED
 substate. It might actually be worth fixing, since almost everything now
 is RemainAfterExit=true.

Alright, I've looked into this a bit, I'll send a patch that should
handle it as well.

-j

 
 Zbyszek
 

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

[systemd-devel] [PATCH] Fix RemainAfterExit services keeping a hold on console

2013-11-14 Thread Olivier Brunel 
When a service exits succesfully and has RemainAfterExit set, its hold
on the console (in m-n_on_console) wasn't released since the unit state
didn't change.
---
 TODO   |  2 --
 src/core/service.c | 16 
 src/core/unit.c|  3 +++
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/TODO b/TODO
index 57e1122..efc7e2a 100644
--- a/TODO
+++ b/TODO
@@ -21,8 +21,6 @@ Bugfixes:
 
   Cannot add dependency job for unit display-manager.service, ignoring: Unit 
display-manager.service failed to load: No such file or directory. See system 
logs and 'systemctl status display-manager.service' for details.
 
-* Substract units in SERVICE_EXITED substate from n_on_console.
-
 Fedora 20:
 
 * external: ps should gain colums for slice and machine
diff --git a/src/core/service.c b/src/core/service.c
index 3da32a1..c0ee114 100644
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -1519,6 +1519,22 @@ static void service_set_state(Service *s, ServiceState 
state) {
 if (state == SERVICE_EXITED  UNIT(s)-manager-n_reloading = 0)
 unit_destroy_cgroup(UNIT(s));
 
+/* For remain_after_exit services, let's see if we can release the
+ * hold on the console, since unit_notify() only does that in case of
+ * change of state */
+if (state == SERVICE_EXITED  s-remain_after_exit 
+UNIT(s)-manager-n_on_console  0) {
+ExecContext *ec = unit_get_exec_context(UNIT(s));
+if (ec  exec_context_may_touch_console(ec)) {
+Manager *m = UNIT(s)-manager;
+
+m-n_on_console --;
+if (m-n_on_console == 0)
+/* unset no_console_output flag, since the 
console is free */
+m-no_console_output = false;
+}
+}
+
 if (old_state != state)
 log_debug_unit(UNIT(s)-id,
%s changed %s - %s, UNIT(s)-id,
diff --git a/src/core/unit.c b/src/core/unit.c
index 15e0a82..d41bc90 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -1484,6 +1484,9 @@ void unit_notify(Unit *u, UnitActiveState os, 
UnitActiveState ns, bool reload_su
 if (UNIT_IS_INACTIVE_OR_FAILED(ns))
 unit_destroy_cgroup(u);
 
+/* Note that this doesn't apply to RemainAfterExit services exiting
+ * sucessfully, since there's no change of state in that case. Which is
+ * why it is handled in service_set_state() */
 if (UNIT_IS_INACTIVE_OR_FAILED(os) != UNIT_IS_INACTIVE_OR_FAILED(ns)) {
 ExecContext *ec = unit_get_exec_context(u);
 if (ec  exec_context_may_touch_console(ec)) {
-- 
1.8.4.2

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

Re: [systemd-devel] [PATCH] bus: fix duplicate comparisons

2013-10-10 Thread Olivier Brunel 
On 10/10/13 12:38, Carlos Silva wrote:
 On Thu, Oct 10, 2013 at 5:14 AM, Tero Roponen tero.ropo...@gmail.com
  wrote:
 
 Testing for y  x is the same as testing for x  y.

 
 
 -if (y  x)
 +if (x  y)

 snip
 
 I thing you forgot to change the signs ;)

No, I believe that was the point of the patch. The two tests were the
same, first testing (x  y), and then (y  x). Now it then properly
tests for (x  y)

-j

 
 
 ___
 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] Fix timeout when stopping Type=notify service

2013-10-01 Thread Olivier Brunel 
On 10/01/13 05:08, Lennart Poettering wrote:
 On Fri, 20.09.13 22:53, Olivier Brunel (j...@jjacky.com) wrote:
 
 Since 41efeaec a call to service_unwatch_main_pid() is done from
 service_set_main_pid(), which is called upon receiving message MAINPID=

 This had the side effect of not watching pid anymore, and would result in a
 useless timeout when stopping the service, as the unit wouldn't be identified
 from the pid, so not marked stopped which would result in systemd thinking 
 this
 was a timeout.
 
 I commited a different fix now in
 7400b9d2e99938d17b281d7df43680eade18666e, but it's untested. Could you
 check if this makes things work?

Yes, fixes the issue.

Thanks,
-j

 
 ---
 I'm not exactly familiar with systemd's internals, so this might not be the
 correct way to fix this, please correct me if it isn't.

  src/core/service.c | 8 
  1 file changed, 8 insertions(+)

 diff --git a/src/core/service.c b/src/core/service.c
 index 246a86e..1dccdff 100644
 --- a/src/core/service.c
 +++ b/src/core/service.c
 @@ -3388,9 +3388,17 @@ static void service_notify_message(Unit *u, pid_t 
 pid, char **tags) {
  log_warning_unit(u-id,
   Failed to parse notification 
 message %s, e);
  else {
 +int r;
 +
  log_debug_unit(u-id,
 %s: got %s, u-id, e);
  service_set_main_pid(s, pid);
 +r = unit_watch_pid(u, pid);
 +if (r  0)
 +/* FIXME: we need to do something here */
 +log_warning_unit(u-id,
 + Failed to watch PID %lu 
 from service %s,
 + (unsigned long) pid, 
 u-id);
  }
  }
  
 
 
 Lennart
 

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

[systemd-devel] [PATCH 1/3] Resolve /dev/console to the active tty instead of just tty0

2013-09-20 Thread Olivier Brunel 
When resolving /dev/console one would often get tty0 meaning the active VT.
Resolving to the actual tty (e.g. tty1) will notably help on boot when
determining whether or not PID1 can output to the console.
---
 src/shared/util.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/src/shared/util.c b/src/shared/util.c
index 9a075fa..391bc1d 100644
--- a/src/shared/util.c
+++ b/src/shared/util.c
@@ -3621,6 +3621,16 @@ char *resolve_dev_console(char **active) {
 else
 tty = *active;
 
+if (streq(tty, tty0)) {
+free(*active);
+
+/* Get the active VC (e.g. tty1) */
+if (read_one_line_file(/sys/class/tty/tty0/active, active)  
0)
+*active = strdup(tty0);
+
+tty = *active;
+}
+
 return tty;
 }
 
-- 
1.8.4

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

[systemd-devel] [PATCH] Fix timeout when stopping Type=notify service

2013-09-20 Thread Olivier Brunel 
Since 41efeaec a call to service_unwatch_main_pid() is done from
service_set_main_pid(), which is called upon receiving message MAINPID=

This had the side effect of not watching pid anymore, and would result in a
useless timeout when stopping the service, as the unit wouldn't be identified
from the pid, so not marked stopped which would result in systemd thinking this
was a timeout.
---
I'm not exactly familiar with systemd's internals, so this might not be the
correct way to fix this, please correct me if it isn't.

 src/core/service.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/core/service.c b/src/core/service.c
index 246a86e..1dccdff 100644
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -3388,9 +3388,17 @@ static void service_notify_message(Unit *u, pid_t pid, 
char **tags) {
 log_warning_unit(u-id,
  Failed to parse notification message 
%s, e);
 else {
+int r;
+
 log_debug_unit(u-id,
%s: got %s, u-id, e);
 service_set_main_pid(s, pid);
+r = unit_watch_pid(u, pid);
+if (r  0)
+/* FIXME: we need to do something here */
+log_warning_unit(u-id,
+ Failed to watch PID %lu from 
service %s,
+ (unsigned long) pid, u-id);
 }
 }
 
-- 
1.8.4

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

[systemd-devel] [PATCH 0/3] Fix issues re: visibility of status messages

2013-09-20 Thread Olivier Brunel 
Hi,

I'm running Arch Linux, have been using systemd-204, and recently tried the new
207 release, and I have been having some issues with it. One was that status
messages would just stop at some point near the end of the boot process, and
also that I wouldn't get any during a shutdown/reboot.

It might be useful to note that I don't start a getty on tty1, which is why I
expect to see all status messages until default target is reached, even after
the getty/login has been started (which happens on tty2).

After looking into it, I came up with the following patches to fix the issue.
The reason status messages would stop was that the getty was started, and
systemd then stopped using the console to avoid collisions w/ gettys.

However, as I said I don't have a getty started on tty1 so for me that is a bug,
as there's no reason not to keep printing status messages on tty1.

The lack of messages on shutdown/reboot was also linked to this, because if
no_console_output was set to true during boot, it'd stay there and prevent
messages to show up on shutdown.

To fix this (in the event it was set to true on boot) a patch simply resets it
to false on job_shutdown_magic(), but I'm not exactly sure if that's the right
way to do this.


FYI I should add that in a similar setup as the one I described, this will not
be enough to keep messages on tty1, since fsck's units are now RemainAfterExit
(see https://bugs.freedesktop.org/show_bug.cgi?id=66784), which means they're
seen by systemd as owning the console (as far as outputing messages there is
concerned I mean), and it will therefore stop printing status messages.

I'm not sure you want to fix this, as it might be only a cosmetic issue for a
small usecase hence not worth the trouble, so I've simply undone it using a
.conf file on my end, figured I should mention it though.

-jjacky

Olivier Brunel (3):
  Resolve /dev/console to the active tty instead of just tty0
  Only disable output on console during boot if needed
  Fix possible lack of status messages on shutdown/reboot

 src/core/job.c |  3 +++
 src/core/manager.c |  2 +-
 src/shared/util.c  | 10 ++
 3 files changed, 14 insertions(+), 1 deletion(-)

-- 
1.8.4

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

[systemd-devel] [PATCH 2/3] Only disable output on console during boot if needed

2013-09-20 Thread Olivier Brunel 
If there are no more jobs on console, no need/we shouldn't disable output.
---
 src/core/manager.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/core/manager.c b/src/core/manager.c
index 669af15..a48d711 100644
--- a/src/core/manager.c
+++ b/src/core/manager.c
@@ -1757,7 +1757,7 @@ static int process_event(Manager *m, struct epoll_event 
*ev) {
 }
 
 case WATCH_IDLE_PIPE: {
-m-no_console_output = true;
+m-no_console_output = m-n_on_console  0;
 
 manager_unwatch_idle_pipe(m);
 close_idle_pipe(m);
-- 
1.8.4

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

[systemd-devel] [PATCH 3/3] Fix possible lack of status messages on shutdown/reboot

2013-09-20 Thread Olivier Brunel 
Since 31a7eb86 the output on console can be disabled to avoid colliding with
gettys. However, it could also lead to a lack of messages during
shutdown/reboot.
---
 src/core/job.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/core/job.c b/src/core/job.c
index 85f77e8..fd22184 100644
--- a/src/core/job.c
+++ b/src/core/job.c
@@ -1100,6 +1100,9 @@ void job_shutdown_magic(Job *j) {
 if (detect_container(NULL)  0)
 return;
 
+/* In case messages on console has been disabled on boot */
+j-unit-manager-no_console_output = false;
+
 asynchronous_sync();
 }
 
-- 
1.8.4

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

[systemd-devel] [PATCH] journald: Log error when failed to get machine-id on start

2013-09-12 Thread Olivier Brunel 
Can help since the journal requires /etc/machine-id to exists in order to start,
and will simply silently exit when it does not.
---
Not sure if the behavior is known/expected or a bug, but when e.g. booting a
system with a read-only rootfs where /etc/machine-id doesn't exist, the journal
would just silently fail (over  over) with no indication of why (even at debug
log_level), and regardless of the Storage option (i.e. even with Storage=none).

Again, this might be expected, so this just adds a log message to clue you in on
why it doesn't start. (Might also be a good idea to mention this requirement in
systemd-journald(8) ?)

 src/journal/journald-server.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/journal/journald-server.c b/src/journal/journald-server.c
index 9daeb6e..ba211b3 100644
--- a/src/journal/journald-server.c
+++ b/src/journal/journald-server.c
@@ -897,8 +897,10 @@ static int system_journal_open(Server *s) {
 char ids[33];
 
 r = sd_id128_get_machine(machine);
-if (r  0)
+if (r  0) {
+log_error(Failed to get machine id: %s, strerror(-r));
 return r;
+}
 
 sd_id128_to_string(machine, ids);
 
@@ -1000,10 +1002,8 @@ int server_flush_to_var(Server *s) {
 log_debug(Flushing to /var...);
 
 r = sd_id128_get_machine(machine);
-if (r  0) {
-log_error(Failed to get machine id: %s, strerror(-r));
+if (r  0)
 return r;
-}
 
 r = sd_journal_open(j, SD_JOURNAL_RUNTIME_ONLY);
 if (r  0) {
-- 
1.8.4

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

[systemd-devel] [PATCH] Highlight ordering cycle deletions

2012-11-04 Thread Olivier Brunel 
Having unit(s) removed/not started, even if it solved the issue and allowed
to boot successfully, should still be considered an error, as something
clearly isn't right.

This patch elevates the log message from warning to error, and adds a status
message to make things more obvious.

Signed-off-by: Olivier Brunel i.am.jack.m...@gmail.com
---
 src/core/transaction.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/core/transaction.c b/src/core/transaction.c
index 4bce942..ee6992a 100644
--- a/src/core/transaction.c
+++ b/src/core/transaction.c
@@ -374,7 +374,8 @@ static int transaction_verify_order_one(Transaction *tr, 
Job *j, Job *from, unsi
 
 
 if (delete) {
-log_warning(Breaking ordering cycle by deleting job 
%s/%s, delete-unit-id, job_type_to_string(delete-type));
+log_error(Breaking ordering cycle by deleting job 
%s/%s, delete-unit-id, job_type_to_string(delete-type));
+status_printf(ANSI_HIGHLIGHT_RED_ON  SKIP  
ANSI_HIGHLIGHT_OFF, true, Ordering cycle found, skip %s, 
unit_description(delete-unit));
 transaction_delete_unit(tr, delete-unit);
 return -EAGAIN;
 }
-- 
1.8.0

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

[systemd-devel] [PATCH] Fix starting swap unit on symlink made it unstoppable

2012-10-13 Thread Olivier Brunel 
Starting a swap unit pointing to (What) a symlink (e.g. /dev/mapper/swap
or /dev/disk/by-uuid/...) would have said unit be marked active, follow
the one using the actual device (/dev/{dm-1,sda3}), but that new unit
would be seen as inactive.
Since all requests to stop swap units would follow/redirect to it,
and it is seen inactive, nothing would be done (swapoff never called).

This is because this unit would be treated twice in
swap_process_new_swap, the second call to swap_add_one causing it to
eventually be marked inactive.

Signed-off-by: Olivier Brunel i.am.jack.m...@gmail.com
---
The patch removes the call to udev_device_get_devnode, assuming that
device nodes (and not symlinks) are used under /proc/swaps, which
seems to be the case.

If that's not always true though, a strcmp(device, dn) would then fix
the issue, as well as a second one on the symlinks loop (to avoid
double calls to swap_add_one on the symink this time).


 src/core/swap.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/src/core/swap.c b/src/core/swap.c
index b4f53b7..9394a07 100644
--- a/src/core/swap.c
+++ b/src/core/swap.c
@@ -424,7 +424,6 @@ static int swap_process_new_swap(Manager *m, const char 
*device, int prio, bool
 
 if (stat(device, st) = 0  S_ISBLK(st.st_mode)) {
 struct udev_device *d;
-const char *dn;
 struct udev_list_entry *item = NULL, *first = NULL;
 
 /* So this is a proper swap device. Create swap units
@@ -433,10 +432,6 @@ static int swap_process_new_swap(Manager *m, const char 
*device, int prio, bool
 if (!(d = udev_device_new_from_devnum(m-udev, 'b', 
st.st_rdev)))
 return -ENOMEM;
 
-dn = udev_device_get_devnode(d);
-if (dn)
-r = swap_add_one(m, dn, device, prio, false, false, 
set_flags);
-
 /* Add additional units for all symlinks */
 first = udev_device_get_devlinks_list_entry(d);
 udev_list_entry_foreach(item, first) {
-- 
1.7.12.2

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