[systemd-devel] [PATCH 1/3] core: Add LISTEN_NAMES environment variable

[Lennart Poettering]

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?

> @@ -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...

> +        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...

> +
> +        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?

> +
> +                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...

> +                        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.

> +        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.

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...

Lennart

-- 
Lennart Poettering, Red Hat