Re: [systemd-devel] [PATCH] manager: Ensure user's systemd runtime directory exists.
On Fri, 07.11.14 09:38, Colin Guthrie (gm...@colin.guthr.ie) wrote: > Lennart Poettering wrote on 07/11/14 01:06: > > On Wed, 05.11.14 14:51, Colin Guthrie (gm...@colin.guthr.ie) wrote: > > > >> Colin Guthrie wrote on 03/11/14 08:02: > >>> Zbigniew Jędrzejewski-Szmek wrote on 02/11/14 18:18: > On Sun, Nov 02, 2014 at 02:04:20PM +, Colin Guthrie wrote: > > This mirrors code in dbus.c when creating the private socket and > > avoids error messages like: > > > > systemd[1353]: bind(/run/user/603/systemd/notify) failed: No such file > > or directory > > systemd[1353]: Failed to fully start up daemon: No such file or > > directory > > Seems reasonable. But why not move the mkdir_parent_label() to the shared > code path? Even if the dir is created elsewhere, it seems cleaner to > ensure > here that it is available. > >>> > >>> Well, to be honest, I just copied the structure from dbus.c. > >>> > >>> I can easily do as you suggest in both places if you think it's nicer. I > >>> guess this would add two unnecessary stat()s (at least - not looked at > >>> the mkdir... implementation!) on boot however, so might just be better > >>> leaving it as is (not that that is a real problem practically speaking, > >>> especially in tmpfs!). > >> > >> Just pushed as is for now. I'm sure any moving of mkdir*() to common > >> code path can come later (both here and in dbus.c) if it's deemed more > >> readable and doesn't have a negative impact on performance (I'd expect > >> it to be negligible, but I'm not an embedded guy) > > > > It's not an inner loop. We it is usually called exactly once. We do > > not optimize such cases for individual syscalls. Stuff that ends up in > > inner loops is something to optimize, possibly. > > > > Anyway, I moved the mkdir now to the common path. Not that it really > > matters, but more because I wanted to cast the result to (void), in > > order to make sure Coverity doesn't trip up over us ignoring the > > return value from mkdir(). And while I was at it... > > Cool, as I mentioned in the thread, you likely want to make a similar > change in src/core/dbus.c too when creating the private socket (and the > dir to hold it). > > If nothing else you'll want the (void) cast on mkdir call. There may be > other reasons to leave it as it is tho', so I don't want to assume too > much and make the change myself in case there are subtleties that are > beyond me this early in the morning! I made the two codepaths look more alike now. 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] manager: Ensure user's systemd runtime directory exists.
Lennart Poettering wrote on 07/11/14 01:06: > On Wed, 05.11.14 14:51, Colin Guthrie (gm...@colin.guthr.ie) wrote: > >> Colin Guthrie wrote on 03/11/14 08:02: >>> Zbigniew Jędrzejewski-Szmek wrote on 02/11/14 18:18: On Sun, Nov 02, 2014 at 02:04:20PM +, Colin Guthrie wrote: > This mirrors code in dbus.c when creating the private socket and > avoids error messages like: > > systemd[1353]: bind(/run/user/603/systemd/notify) failed: No such file or > directory > systemd[1353]: Failed to fully start up daemon: No such file or directory Seems reasonable. But why not move the mkdir_parent_label() to the shared code path? Even if the dir is created elsewhere, it seems cleaner to ensure here that it is available. >>> >>> Well, to be honest, I just copied the structure from dbus.c. >>> >>> I can easily do as you suggest in both places if you think it's nicer. I >>> guess this would add two unnecessary stat()s (at least - not looked at >>> the mkdir... implementation!) on boot however, so might just be better >>> leaving it as is (not that that is a real problem practically speaking, >>> especially in tmpfs!). >> >> Just pushed as is for now. I'm sure any moving of mkdir*() to common >> code path can come later (both here and in dbus.c) if it's deemed more >> readable and doesn't have a negative impact on performance (I'd expect >> it to be negligible, but I'm not an embedded guy) > > It's not an inner loop. We it is usually called exactly once. We do > not optimize such cases for individual syscalls. Stuff that ends up in > inner loops is something to optimize, possibly. > > Anyway, I moved the mkdir now to the common path. Not that it really > matters, but more because I wanted to cast the result to (void), in > order to make sure Coverity doesn't trip up over us ignoring the > return value from mkdir(). And while I was at it... Cool, as I mentioned in the thread, you likely want to make a similar change in src/core/dbus.c too when creating the private socket (and the dir to hold it). If nothing else you'll want the (void) cast on mkdir call. There may be other reasons to leave it as it is tho', so I don't want to assume too much and make the change myself in case there are subtleties that are beyond me this early in the morning! Col -- Colin Guthrie gmane(at)colin.guthr.ie http://colin.guthr.ie/ Day Job: Tribalogic Limited http://www.tribalogic.net/ Open Source: Mageia Contributor http://www.mageia.org/ PulseAudio Hacker http://www.pulseaudio.org/ Trac Hacker http://trac.edgewall.org/ ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] manager: Ensure user's systemd runtime directory exists.
On Wed, 05.11.14 14:51, Colin Guthrie (gm...@colin.guthr.ie) wrote: > Colin Guthrie wrote on 03/11/14 08:02: > > Zbigniew Jędrzejewski-Szmek wrote on 02/11/14 18:18: > >> On Sun, Nov 02, 2014 at 02:04:20PM +, Colin Guthrie wrote: > >>> This mirrors code in dbus.c when creating the private socket and > >>> avoids error messages like: > >>> > >>> systemd[1353]: bind(/run/user/603/systemd/notify) failed: No such file or > >>> directory > >>> systemd[1353]: Failed to fully start up daemon: No such file or directory > >> > >> Seems reasonable. But why not move the mkdir_parent_label() to the shared > >> code path? Even if the dir is created elsewhere, it seems cleaner to ensure > >> here that it is available. > > > > Well, to be honest, I just copied the structure from dbus.c. > > > > I can easily do as you suggest in both places if you think it's nicer. I > > guess this would add two unnecessary stat()s (at least - not looked at > > the mkdir... implementation!) on boot however, so might just be better > > leaving it as is (not that that is a real problem practically speaking, > > especially in tmpfs!). > > Just pushed as is for now. I'm sure any moving of mkdir*() to common > code path can come later (both here and in dbus.c) if it's deemed more > readable and doesn't have a negative impact on performance (I'd expect > it to be negligible, but I'm not an embedded guy) It's not an inner loop. We it is usually called exactly once. We do not optimize such cases for individual syscalls. Stuff that ends up in inner loops is something to optimize, possibly. Anyway, I moved the mkdir now to the common path. Not that it really matters, but more because I wanted to cast the result to (void), in order to make sure Coverity doesn't trip up over us ignoring the return value from mkdir(). And while I was at it... 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] manager: Ensure user's systemd runtime directory exists.
Colin Guthrie wrote on 03/11/14 08:02: > Zbigniew Jędrzejewski-Szmek wrote on 02/11/14 18:18: >> On Sun, Nov 02, 2014 at 02:04:20PM +, Colin Guthrie wrote: >>> This mirrors code in dbus.c when creating the private socket and >>> avoids error messages like: >>> >>> systemd[1353]: bind(/run/user/603/systemd/notify) failed: No such file or >>> directory >>> systemd[1353]: Failed to fully start up daemon: No such file or directory >> >> Seems reasonable. But why not move the mkdir_parent_label() to the shared >> code path? Even if the dir is created elsewhere, it seems cleaner to ensure >> here that it is available. > > Well, to be honest, I just copied the structure from dbus.c. > > I can easily do as you suggest in both places if you think it's nicer. I > guess this would add two unnecessary stat()s (at least - not looked at > the mkdir... implementation!) on boot however, so might just be better > leaving it as is (not that that is a real problem practically speaking, > especially in tmpfs!). Just pushed as is for now. I'm sure any moving of mkdir*() to common code path can come later (both here and in dbus.c) if it's deemed more readable and doesn't have a negative impact on performance (I'd expect it to be negligible, but I'm not an embedded guy) Cheers! Col -- Colin Guthrie gmane(at)colin.guthr.ie http://colin.guthr.ie/ Day Job: Tribalogic Limited http://www.tribalogic.net/ Open Source: Mageia Contributor http://www.mageia.org/ PulseAudio Hacker http://www.pulseaudio.org/ Trac Hacker http://trac.edgewall.org/ ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] manager: Ensure user's systemd runtime directory exists.
Zbigniew Jędrzejewski-Szmek wrote on 02/11/14 18:18: > On Sun, Nov 02, 2014 at 02:04:20PM +, Colin Guthrie wrote: >> This mirrors code in dbus.c when creating the private socket and >> avoids error messages like: >> >> systemd[1353]: bind(/run/user/603/systemd/notify) failed: No such file or >> directory >> systemd[1353]: Failed to fully start up daemon: No such file or directory > > Seems reasonable. But why not move the mkdir_parent_label() to the shared > code path? Even if the dir is created elsewhere, it seems cleaner to ensure > here that it is available. Well, to be honest, I just copied the structure from dbus.c. I can easily do as you suggest in both places if you think it's nicer. I guess this would add two unnecessary stat()s (at least - not looked at the mkdir... implementation!) on boot however, so might just be better leaving it as is (not that that is a real problem practically speaking, especially in tmpfs!). Col -- Colin Guthrie gmane(at)colin.guthr.ie http://colin.guthr.ie/ Day Job: Tribalogic Limited http://www.tribalogic.net/ Open Source: Mageia Contributor http://www.mageia.org/ PulseAudio Hacker http://www.pulseaudio.org/ Trac Hacker http://trac.edgewall.org/ ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] manager: Ensure user's systemd runtime directory exists.
On Sun, Nov 02, 2014 at 02:04:20PM +, Colin Guthrie wrote:
> This mirrors code in dbus.c when creating the private socket and
> avoids error messages like:
>
> systemd[1353]: bind(/run/user/603/systemd/notify) failed: No such file or
> directory
> systemd[1353]: Failed to fully start up daemon: No such file or directory
Seems reasonable. But why not move the mkdir_parent_label() to the shared
code path? Even if the dir is created elsewhere, it seems cleaner to ensure
here that it is available.
Zbyszek
> ---
> src/core/manager.c | 12
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/src/core/manager.c b/src/core/manager.c
> index ed7fdc7..2c77757 100644
> --- a/src/core/manager.c
> +++ b/src/core/manager.c
> @@ -662,9 +662,11 @@ static int manager_setup_notify(Manager *m) {
> return -errno;
> }
>
> -if (m->running_as == SYSTEMD_SYSTEM)
> +if (m->running_as == SYSTEMD_SYSTEM) {
> m->notify_socket = strdup("/run/systemd/notify");
> -else {
> +if (!m->notify_socket)
> +return log_oom();
> +} else {
> const char *e;
>
> e = getenv("XDG_RUNTIME_DIR");
> @@ -674,9 +676,11 @@ static int manager_setup_notify(Manager *m) {
> }
>
> m->notify_socket = strappend(e, "/systemd/notify");
> +if (!m->notify_socket)
> +return log_oom();
> +
> +mkdir_parents_label(m->notify_socket, 0755);
> }
> -if (!m->notify_socket)
> -return log_oom();
>
> strncpy(sa.un.sun_path, m->notify_socket,
> sizeof(sa.un.sun_path)-1);
> r = bind(fd, &sa.sa, offsetof(struct sockaddr_un, sun_path)
> + strlen(sa.un.sun_path));
> --
> 2.1.2
>
> ___
> 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
[systemd-devel] [PATCH] manager: Ensure user's systemd runtime directory exists.
This mirrors code in dbus.c when creating the private socket and
avoids error messages like:
systemd[1353]: bind(/run/user/603/systemd/notify) failed: No such file or
directory
systemd[1353]: Failed to fully start up daemon: No such file or directory
---
src/core/manager.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/core/manager.c b/src/core/manager.c
index ed7fdc7..2c77757 100644
--- a/src/core/manager.c
+++ b/src/core/manager.c
@@ -662,9 +662,11 @@ static int manager_setup_notify(Manager *m) {
return -errno;
}
-if (m->running_as == SYSTEMD_SYSTEM)
+if (m->running_as == SYSTEMD_SYSTEM) {
m->notify_socket = strdup("/run/systemd/notify");
-else {
+if (!m->notify_socket)
+return log_oom();
+} else {
const char *e;
e = getenv("XDG_RUNTIME_DIR");
@@ -674,9 +676,11 @@ static int manager_setup_notify(Manager *m) {
}
m->notify_socket = strappend(e, "/systemd/notify");
+if (!m->notify_socket)
+return log_oom();
+
+mkdir_parents_label(m->notify_socket, 0755);
}
-if (!m->notify_socket)
-return log_oom();
strncpy(sa.un.sun_path, m->notify_socket,
sizeof(sa.un.sun_path)-1);
r = bind(fd, &sa.sa, offsetof(struct sockaddr_un, sun_path) +
strlen(sa.un.sun_path));
--
2.1.2
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
