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_names, n_fds, &x);
+ if (r)
+ return r;
We usually indicate errors with negative errno-style integers, hence
please make sure to check for errors with "if (r < 0" or so. See
CODING_STYLE for details...
Will fix this for v2.
+ our_env[n_env++] = x;
+ }
}
Also, if you add an additional env var to the env var block you need
to increase the allocated size of our_env by one.
Will fix in v2.
+ int *fds = NULL;
+ const char **fds_names = NULL;
Because C is the way it is, string arrays (usually called strv in the
systemd sources) and "const" don't really mix that well. We hence
usually don't bother with "const" if we use them. Hence, consider
dropping "const" from this (and the other) declarations of "char **"
variables and parameters.
I may drop const if you like, no problem.
Regarding passing fds around I generally think so we should just
define a new struct for this and use it everywhere. More specifically,
please intrdouce a new struct "NamedFileDescriptor" or so, that looks
something like this:
typedef struct NamedFileDescriptor {
char *name;
int fd;
} NamedFileDescriptor;
And that we use wherever we store or pass around fds for activation
purposes...
Ok, may be usefull. I'll try to use this in v2.
--
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
