Re: [systemd-devel] [PATCH 1/3] core: Add LISTEN_NAMES environment variable
On 05/18/2015 09:45 PM, Lennart Poettering wrote:
On Mon, 18.05.15 17:55, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote:
On Mon, May 18, 2015 at 06:01:10PM +0200, Lennart Poettering wrote:
Being able to attach a name to the fds is hence really useful. logind
could use this to attach the session identifier to the fds, and would
hence be able to safely map the fds back to their sessions after
coming back from a restart...
Yeah, that makes sense. But currently there's no proposal how to specify
those identifiers. Would be nice discuss both sides of the proposal at
the same time.
sd_pid_notify_with_fds() would probably have to be extended to be
sd_pid_notify_with_fds2(pid_t pid, int unset_environment, const char
*state, const int *fds, const char* const *names, unsigned n_fds)
Hmm, nah. I think we can avoid adding a new call. Instead we should
explicitly allow non-unique names, and then simply pass the name to
use in a normal sd_notify_with_fds() text field, so that it is applied
to all fds pushed the same way. If you want to send multiple fds with
different ids, then one would do this with multiple
sd_pid_notify_with_fds() invocations.
Example:
sd_pid_notify_with_fds(0, false, "FDSTORE=1\nFDNAME=foobar", (int[]) {
STDIN_FILENO, STDOUT_FILENO }, 2);
This would push stdin and stdout of the client into PID 1 and label
both of them "foobar". On next invocation the process would then see:
LISTEN_FDS=2
LISTEN_NAMES=foobar:foobar
And what about socket units: we could automatically generate identifiers
like blah.socket-1, foo.socket-1, foo.socket-2 to allow sockets from multiple
socket files be distinguished. In principle this could be made configurable
through a new option, but I don't think it's worth the trouble.
I'd add a new option for this:
FileDescriptorName=waldi
would apply to all fds declared with a .socket unit. If you want to
apply distinct names to multiple fds, you should define them in two
seperate .socket units.
Hope that makes sense?
Make sens for me. I now see your point of view more clearly.
I will update my patches according to your remarks and idea described here.
Thank you for the review and clarification,
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/3] sd-daemon: Use LISTEN_NAMES env when available
On 05/18/2015 11:05 PM, Filipe Brandenburger wrote: Hi, On Mon, May 18, 2015 at 7:26 AM, Krzysztof Opasiak wrote: Matching between fds and list of expected paths is done in n^2 I don't think that's the case, because you can just stat() all the names and fstat() all the fds, then sort both lists on inode numbers and then traverse both in order matching inode orders on each list, so it's actually O(n * log n). Not that it matters that much, I don't expect this scheme to be used for a very large number of fds/labels... True but it is not possible using sd_is_*() functions, you have to implement this on your own;) -- Krzysztof Opasiak Samsung R&D Institute Poland Samsung Electronics ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/3] core: Add LISTEN_NAMES environment variable
On 05/16/2015 11:28 PM, Zbigniew Jędrzejewski-Szmek wrote: On Fri, May 15, 2015 at 05:35:48PM +0200, Lennart Poettering wrote: On Fri, 15.05.15 17:09, Krzysztof Opasiak (k.opas...@samsung.com) wrote: When passing file descriptors to service systemd pass also two environment variable: - LISTEN_PID - PID of service - LISTEN_FDS - Number of file descriptors passed to service Passed fds may have different types: socket, fifo etc. To distinguish them sd-daemon library provides a set of sd_is_*() functions which does stat on given fd and path and check if this fd is relaten with this path. This commit adds third environment variable: - LISTEN_NAMES - paths/addresses of passed fds this variable consist of fds names separated by :. Each fd name consist of two parts: fd_type=fd_address Why do we need the type at all? It can always be derived from the fd anyway, so why specify? Why it the motivation? Patch description talks tabout passing the path/address in LISTEN_NAMES. Isn't this something that can be queried already? TODO talks about "identifiers". Is "identifier" the same thing, or did the TODO item about have some different meaning? Not exactly. As far as I know it is not possible to get for example fifo path when you have only file descriptor. So it is not possible to ask what is fifo path for this fd? but you may only ask if this path related with this fd? Currently it is done by doing stat on fd and stat on path and compare the results. If we pass this data in env we don't need to do stat on path but only do strcmp() (path cmp exactly, because /a/b/c is equal to /ab///c). As far as I understood Lenart doing last hackfest paths are understood as identifiers but Lenart please correct me if I misunderstood something. -- Krzysztof Opasiak Samsung R&D Institute Poland Samsung Electronics ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/3] sd-daemon: Use LISTEN_NAMES env when available
On 05/15/2015 05:40 PM, Lennart Poettering wrote:
On Fri, 15.05.15 17:09, Krzysztof Opasiak (k.opas...@samsung.com) wrote:
LISTEN_NAMES environment variable contains details
about received file descriptors. Let's try to use it
instead of doing always two stats.
I am really not convinced that it is a good idea to store redundant
information in LISTEN_NAMES, especially if we don't have this
information in all cases anyway.
We also don't always have informations about paths as all descriptors
from fd store have unknown path and type (as far as I know and
understand systemd code but I may be wrong)
Please, let's keep this simple: LISTEN_NAMES should only carry actual
names, nothing else, and let's query the kernel for the actual fd
types.
I'm not sure if doing stat() to determine how he should interpret
content of field in env is simpler and easier but of course you are the
maintainer so you decide how things should be done;)
There's really no point in storing the types in $LISTEN_NAMEs, since
this code is no way performance senstive...
Matching between fds and list of expected paths is done in n^2 so it has
no performance impact as long as number of passed fds isn't big. I know
that it is usually done only once during service startup but it increase
latency between event occurrence and it processing.
+static const char *sd_get_fd_name(int fd) {
The "sd_" prefix we add for exported functions, don't bother with it
for internal calls.
Ok. Will fix this.
+static const char sep = ':';
+static const char escape = '\\';
+const char *env = NULL;
+const char *e = NULL;
+int i;
-assert_return(fd >= 0, -EINVAL);
+assert_return(fd >= 3, NULL);
assert_return() we use for verifiying parameters passed in from
external users to check for programming errors. Since this function is
static this generally doesn't apply. See CODING_STYLE for details...
will replace with traditional if.
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/3] core: Add LISTEN_NAMES environment variable
Hi,
On 05/15/2015 05:35 PM, Lennart Poettering wrote:
On Fri, 15.05.15 17:09, Krzysztof Opasiak (k.opas...@samsung.com) wrote:
When passing file descriptors to service systemd
pass also two environment variable:
- LISTEN_PID - PID of service
- LISTEN_FDS - Number of file descriptors passed to service
Passed fds may have different types: socket, fifo etc.
To distinguish them sd-daemon library provides a set of
sd_is_*() functions which does stat on given fd and path
and check if this fd is relaten with this path.
This commit adds third environment variable:
- LISTEN_NAMES - paths/addresses of passed fds
this variable consist of fds names separated by :.
Each fd name consist of two parts:
fd_type=fd_address
Why do we need the type at all? It can always be derived from the fd
anyway, so why specify?
Passing both type and path allows us to determine type of socket without
any syscall. For example sd_is_fifo() function is reduced to three
simple steps:
- find nth field in env
- do strncmp(field, "fifo=", length)
- do path cmp with value received from user
It is much faster as there is no context switch and consistent if we
take both type and path from env instead of doing stat to determine type
and then take path from env for comparsion. It doesn't add much more
complexity but eliminates stat on fd in most functions so why not to do
this?
@@ -1171,9 +1171,55 @@ static void do_idle_pipe_dance(int idle_pipe[4]) {
safe_close(idle_pipe[3]);
}
+static int build_listen_names(const char **fds_names, unsigned n_fds, char
**env)
+{
We generally place the opening bracket in the same line as the
function name...
I tried but it was stronger than me;) Will fix in v2.
+unsigned i, j;
+unsigned pos;
+int size;
+char *names = NULL;
+static const char separator = ':';
+static const char escape = '\\';
+static const char *prefix = "LISTEN_NAMES=";
Hmm, why not just use the literl strings/chars wherever we need
them. It sounds needlessly complex to use constants for this, after
all we only use this within this one function...
Also the last constant declares both a pointer and an array of string,
which appears unnecessary...
In may opinion this improves readability of the code. It simply
indicates that you are looking for a separator and not for some unnamed
semicolon. You don't need to look in documentation what does : means, as
you see descriptive variable name.
Moreover I have used this in more than one place and I know that it is
convenient to use compiler to check your typos instead of wasting half
an hour to find out that you made a typo when writing string constant
for nth time. If I write prefux compiler will complain about undefined
identifier but if I write "LISTEN_NANES=" compilation will be clear and
I will have to look it for this while testing.
I have no strong opinion, in C both defines and static const are good
enough in this use case. I may replace those with defines if you like?
+
+assert(fds_names);
+assert(env);
+
+size = strlen(prefix);
+for (i = 0; i < n_fds; ++i) {
+size += 1; /* for separator */
+if (!fds_names[i])
+continue;
+
+for (j = 0; fds_names[i][j]; ++j)
+if (fds_names[i][j] == separator)
+size += 2;
+else
+size += 1;
+}
+
+names = malloc(size);
+if (!names)
+return -ENOMEM;
+
+strcpy(names, prefix);
+pos = strlen(prefix);
+for (i = 0; i < n_fds; ++i) {
+for (j = 0; fds_names[i] && fds_names[i][j]; ++j) {
+if (fds_names[i][j] == separator)
+names[pos++] = escape;
+names[pos++] = fds_names[i][j];
+}
+names[pos++] = separator;
+}
+names[pos - 1] = '\0';
+*env = names;
+return 0;
+}
I am not entirely sure I grok this function, but doesn't this do what
strv_join() does anyway?
Well, not exactly it does a little bit more.
As use colons to separate paths in LISTEN_NAMES variable and we cannot
guarantee that socket, fifo etc path doesn't contain colons (: is a
valid path character in linux) we have to escape them. What this
function does is merging all the paths, escape semicolons in paths using
\ and place colons to separate paths. Example:
take:
socket=/run/my_test/func::other_func.socket
fifo=/run/other::test/myfifo
special=/dev/mydevice
produce:
socket=/run/my_test/func\:\:other_func.socket:fifo=/run/other\:\:test/myfifo:special=/dev/mydevice
+
+if (fds_names) {
+r = build_listen_names(fds_name
[systemd-devel] [PATCH 2/3] sd-daemon: Use LISTEN_NAMES env when available
LISTEN_NAMES environment variable contains details
about received file descriptors. Let's try to use it
instead of doing always two stats.
This commit reworks all sd_is_*() functions to try
parse LISTEN_NAMES variable in first step and do
stats only as fallback procedure or if field for given
fd is empty.
We do this only for fds from 3 up to 3 + LISTEN_FDS - 1.
For all other fds or when LISTEN_NAMES is not available
we still do two stats.
Please be careful, this may cause some troubles.
when service has not unset environment and close fdx where
3 <= x < 3 + LISTEN_FDS and open something else (file,
socket etc) he will probably get the same x as fd.
If he call sd_is_*() function with this fd we will try
to use content of LISTEN_NAMES but in is not valid now.
A solution is to not use sd-daemon library in such
scenario or simply unset environment before calling
sd-daemon functions for fds other than received from sd.
---
src/libsystemd/sd-daemon/sd-daemon.c | 463 +++---
1 file changed, 430 insertions(+), 33 deletions(-)
diff --git a/src/libsystemd/sd-daemon/sd-daemon.c
b/src/libsystemd/sd-daemon/sd-daemon.c
index 82ac72c..f9d7a74 100644
--- a/src/libsystemd/sd-daemon/sd-daemon.c
+++ b/src/libsystemd/sd-daemon/sd-daemon.c
@@ -38,6 +38,8 @@
#include "socket-util.h"
#include "sd-daemon.h"
+#define SIGNUM(x) (((x) > 0) - ((x) < 0))
+
_public_ int sd_listen_fds(int unset_environment) {
const char *e;
unsigned n;
@@ -80,6 +82,7 @@ _public_ int sd_listen_fds(int unset_environment) {
finish:
if (unset_environment) {
+unsetenv("LISTEN_NAMES");
unsetenv("LISTEN_PID");
unsetenv("LISTEN_FDS");
}
@@ -87,10 +90,89 @@ finish:
return r;
}
-_public_ int sd_is_fifo(int fd, const char *path) {
-struct stat st_fd;
+static const char *sd_get_fd_name(int fd) {
+static const char sep = ':';
+static const char escape = '\\';
+const char *env = NULL;
+const char *e = NULL;
+int i;
-assert_return(fd >= 0, -EINVAL);
+assert_return(fd >= 3, NULL);
+
+env = getenv("LISTEN_NAMES");
+if (!env)
+goto out;
+
+for (i = 3, e = env; *e && i < fd; ++e)
+if (*e == sep && (e == env || *(e - 1) != escape))
+++i;
+
+if (e && !*e)
+e = NULL;
+ out:
+return e;
+}
+
+static int sd_env_mem_cmp(const char *env_path, const char *buf, int length) {
+static const char sep = ':';
+static const char escape = '\\';
+int i = 0;
+
+/* FIXME: We assume we were able to print the memory to
+ * env. That is actually an invalid assumption. */
+while (*env_path && i < length) {
+if (*env_path == sep)
+return *buf ? -1 : 0;
+
+/* skip \ as escpae character */
+if (*env_path == escape && *(env_path + 1) == sep)
+++env_path;
+
+if (*env_path != *buf)
+break;
+
+++env_path;
+++buf;
+++i;
+}
+
+return *env_path == sep && i == length ? 0 : SIGNUM(*env_path - *buf);
+}
+
+static int sd_env_path_cmp(const char *env_path, const char *path) {
+static const char sep = ':';
+static const char escape = '\\';
+
+while (*env_path && *path) {
+if (*env_path == sep)
+return *path ? -1 : 0;
+
+/* skip \ as escpae character */
+if (*env_path == escape && *(env_path + 1) == sep)
+++env_path;
+
+if (*env_path != *path)
+break;
+
+/* Because paths:
+ * /a/b/cd
+ * /a//bc/d
+ * points to the same place
+ * we have to skip all redundant slashes */
+do
+++env_path;
+while (*env_path == '/' && *(env_path + 1) == '/');
+
+do
+++path;
+while (*path == '/' && *(path + 1) == '/');
+}
+
+return *env_path == sep && !*path ? 0 : SIGNUM(*env_path - *path);
+}
+
+static int sd_is_fifo_stat(int fd, const char *path) {
+struct stat st_fd;
if (fstat(fd, &st_fd) < 0)
return -errno;
@@ -117,11 +199,40 @@ _public_ int sd_is_fifo(int fd, const char *path) {
return 1;
}
-_public_ int sd_is_special(int fd, const char *path) {
-struct stat st_fd;
+_public_ int sd_is_fifo(int fd, const char *path) {
+static const char sep = ':';
+static const char fifo_token[] = "fifo";
+const char *fd_name;
assert_return(fd >= 0, -EINVAL);
+fd_name = sd_get_fd_name(fd);
+if (!fd_name || !*fd
[systemd-devel] [PATCH 3/3] TODO: Remove task related to LISTEN_NAMES
LISTEN_NAMES has been just implemented so there is no need to keep this in TODO file. --- TODO |3 --- 1 file changed, 3 deletions(-) diff --git a/TODO b/TODO index dd85f57..2ea9ee7 100644 --- a/TODO +++ b/TODO @@ -149,9 +149,6 @@ Features: that are not supported... http://lists.freedesktop.org/archives/systemd-devel/2015-February/028076.html -* Introduce $LISTEN_NAMES to complement $LISTEN_FDS, containing a - colon separated list of identifiers for the fds passed. - * maybe introduce WantsMountsFor=? Usecase: http://lists.freedesktop.org/archives/systemd-devel/2015-January/027729.html -- 1.7.9.5 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH 1/3] core: Add LISTEN_NAMES environment variable
When passing file descriptors to service systemd
pass also two environment variable:
- LISTEN_PID - PID of service
- LISTEN_FDS - Number of file descriptors passed to service
Passed fds may have different types: socket, fifo etc.
To distinguish them sd-daemon library provides a set of
sd_is_*() functions which does stat on given fd and path
and check if this fd is relaten with this path.
This commit adds third environment variable:
- LISTEN_NAMES - paths/addresses of passed fds
this variable consist of fds names separated by :.
Each fd name consist of two parts:
fd_type=fd_address
where:
fd_type is a type of fd (socket, mq, fifo etc)
fd_address is a suitable address or path.
Systemd may store fds passed by services so it may
be not feasible to determine what is their type.
Due to this we allow to pass empty field for some file
descriptor. This will mark fds as unknown and sd-library
will still do 2 stats to determine fd type.
Another use case for this convention is starting service
in chroot or namespace. In this case we cannot pass
any fs related path as we don't know what is inside
a container. We simply leave empty all fs-dependent fields.
---
src/core/execute.c | 69 ++--
src/core/execute.h |4 +-
src/core/service.c | 112
src/core/socket.c | 106 +++--
src/core/socket.h |3 +-
5 files changed, 260 insertions(+), 34 deletions(-)
diff --git a/src/core/execute.c b/src/core/execute.c
index 0cca481..39bfdc9 100644
--- a/src/core/execute.c
+++ b/src/core/execute.c
@@ -1171,9 +1171,55 @@ static void do_idle_pipe_dance(int idle_pipe[4]) {
safe_close(idle_pipe[3]);
}
+static int build_listen_names(const char **fds_names, unsigned n_fds, char
**env)
+{
+unsigned i, j;
+unsigned pos;
+int size;
+char *names = NULL;
+static const char separator = ':';
+static const char escape = '\\';
+static const char *prefix = "LISTEN_NAMES=";
+
+assert(fds_names);
+assert(env);
+
+size = strlen(prefix);
+for (i = 0; i < n_fds; ++i) {
+size += 1; /* for separator */
+if (!fds_names[i])
+continue;
+
+for (j = 0; fds_names[i][j]; ++j)
+if (fds_names[i][j] == separator)
+size += 2;
+else
+size += 1;
+}
+
+names = malloc(size);
+if (!names)
+return -ENOMEM;
+
+strcpy(names, prefix);
+pos = strlen(prefix);
+for (i = 0; i < n_fds; ++i) {
+for (j = 0; fds_names[i] && fds_names[i][j]; ++j) {
+if (fds_names[i][j] == separator)
+names[pos++] = escape;
+names[pos++] = fds_names[i][j];
+}
+names[pos++] = separator;
+}
+names[pos - 1] = '\0';
+*env = names;
+return 0;
+}
+
static int build_environment(
const ExecContext *c,
unsigned n_fds,
+const char **fds_names,
usec_t watchdog_usec,
const char *home,
const char *username,
@@ -1183,6 +1229,7 @@ static int build_environment(
_cleanup_strv_free_ char **our_env = NULL;
unsigned n_env = 0;
char *x;
+int r;
assert(c);
assert(ret);
@@ -1199,6 +1246,13 @@ static int build_environment(
if (asprintf(&x, "LISTEN_FDS=%u", n_fds) < 0)
return -ENOMEM;
our_env[n_env++] = x;
+
+if (fds_names) {
+r = build_listen_names(fds_names, n_fds, &x);
+if (r)
+return r;
+our_env[n_env++] = x;
+}
}
if (watchdog_usec > 0) {
@@ -1295,7 +1349,9 @@ static int exec_child(
ExecRuntime *runtime,
char **argv,
int socket_fd,
-int *fds, unsigned n_fds,
+int *fds,
+const char **fds_names,
+unsigned n_fds,
char **files_env,
int *exit_status) {
@@ -1787,7 +1843,7 @@ static int exec_child(
#endif
}
-r = build_environment(context, n_fds, params->watchdog_usec, home,
username, shell, &our_env);
+r = build_environment(context, n_fds, fds_names,
params->watchdog_usec, home, username, shell, &our_env);
if (r < 0) {
*exit_status = EXIT_MEMORY;
return r;
@@ -1841,7 +1897,9 @@ int exec_spawn(Unit *unit,
pid_t *ret) {
_cleanup_strv_free_ char **files_env = NULL;
-int *fds =
