[systemd-devel] [PATCH 1/3] core: Add LISTEN_NAMES environment variable
Krzysztof Opasiak
k.opasiak at samsung.com
Mon May 18 06:36:35 PDT 2015
Hi,
On 05/15/2015 05:35 PM, Lennart Poettering wrote:
> On Fri, 15.05.15 17:09, Krzysztof Opasiak (k.opasiak at 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
More information about the systemd-devel
mailing list