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 k.opas...@samsung.com 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 RD 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 RD 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
Hi, On Mon, May 18, 2015 at 7:26 AM, Krzysztof Opasiak k.opas...@samsung.com 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... Cheers, Filipe ___ 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 Mon, 18.05.15 16:26, Krzysztof Opasiak (k.opas...@samsung.com) wrote: 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;) Well, we have relatively nice wrappers in sd-daemon.h that simplify these stat() checks... 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. Well, the n^2 is pretty theoretic, as using fixed paths in the fs is kinda the antithesis to using a variable number of fds. If people write daemons that take a variable number of fds they will probably not expect them to be mapped to fixed names in the fs, but would discriminate them by other keys, for example whether they are IP or AF_UNIX sockets, or whether they are stream or datagram, and so on. But to do such checks we already have the right APIs from the kernel... 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] sd-daemon: Use LISTEN_NAMES env when available
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.
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.
There's really no point in storing the types in $LISTEN_NAMEs, since
this code is no way performance senstive...
+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.
+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...
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[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_name || *fd_name == sep)
+
