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 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 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 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 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_names, n_fds, x);
+if (r)
+return r;
We usually
Re: [systemd-devel] [PATCH 1/3] core: Add LISTEN_NAMES environment variable
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) 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. Zbyszek ___ 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 Mon, 18.05.15 16:37, Krzysztof Opasiak (k.opas...@samsung.com) wrote: 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 Lennart doing last hackfest paths are understood as identifiers but Lenart please correct me if I misunderstood something. Well, I don't think it's a good idea to include paths in the ids, since they are so problematic in a namespaced world. Instead, we should just define our own namespace for the ids, and just require them to be short strings, independent from any fs path, fd type or so... 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 1/3] core: Add LISTEN_NAMES environment variable
On Mon, 18.05.15 10:19, Richard Maw (richard@codethink.co.uk) wrote: On Sat, May 16, 2015 at 09:28:22PM +, Zbigniew Jędrzejewski-Szmek wrote: 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? I assumed that since the TODO came about at roughly the same time that systemd gained the ability to hold onto fds for services while they restarted, that this would be a way to identify the purpose of returned file descriptors without having to store a mapping of the inode number and device number to fd purpose into /run. The provided patch doesn't add a way to pass an identifier for a fd to systemd though so if that were the motivation, then this patch wouldn't be sufficient. The TODO list item is purely about adding manual identifiers to fds, it's not about adding type information to them: the kernel already carries enough type information for us, the only problem is that sometimes the type alone is not enough, we want to properly give the fds a label. This has a number of usecases. One of them is this: logind wants to store fds referring to DRM or input devices in PID 1, so that it can be restarted at any time without losing access to the DRM/input devices it manages. Now, if you have multiple sessions that access the same devices, then each of the fds referring to these DRM/input device nodes look pretty much the same: their fstat() data is identical, their /proc/self/fd/fd symlink is the same, hence it's impossible to figure out which fd belongs to which session simply by looking at them with fstat() and /proc. 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... The names are useful in other cases too: if you have a daemon that listens on two protocols at the same time (let's say POP3 and IMAP4) on different ports, but not necessarily use the standard ports. hence you need a nice way during actviation to let your daemon know which one to use for which... (so far this was solved by having configuration also for the daemon that then maps the ports back to the protocol, but using ids for this is nicer, as it requires only one set of configuration). I hope that makes sense as rationale. The TODO list item is really about labelling fds, not about attaching type info to the fds. 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 1/3] core: Add LISTEN_NAMES environment variable
On Mon, 18.05.15 15:36, Krzysztof Opasiak (k.opas...@samsung.com) wrote: Passing both type and path allows us to determine type of socket without any syscall. But how is that beneficial? THese sycalls are not slow, and this is not perfoimance sensitive anyway... 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? Well, I disagree. It *does* add considerable complexity, especially since it doesn't cover namespaced environments... I am very sure $LISTEN_NAMES should only carry identifier strings chosen by humans, and not duplicate what you can already do with fstat() and /proc. 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. Well, so far we preferred short code over unnecessary verbose code, as that helps readability too... 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: I'd simply dictate that the names included in $LISTEN_NAMES are not allowed to contain colons. We should make this easy for implementors. Escaping schemes are good if we need to be universal but if we don't have to because we define our own namespace, then we should avoid requiring them. 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 1/3] core: Add LISTEN_NAMES environment variable
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?
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 1/3] core: Add LISTEN_NAMES environment variable
On Sat, May 16, 2015 at 09:28:22PM +, Zbigniew Jędrzejewski-Szmek wrote: 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? I assumed that since the TODO came about at roughly the same time that systemd gained the ability to hold onto fds for services while they restarted, that this would be a way to identify the purpose of returned file descriptors without having to store a mapping of the inode number and device number to fd purpose into /run. The provided patch doesn't add a way to pass an identifier for a fd to systemd though so if that were the motivation, then this patch wouldn't be sufficient. ___ 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 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? Zbyszek ___ 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 = NULL; unsigned
Re: [systemd-devel] [PATCH 1/3] core: Add LISTEN_NAMES environment variable
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?
@@ -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
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
