| Message ID | 1434755017-8442-1-git-send-email-zajec5@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Hey Rafał, Rafał Miłecki <zajec5@gmail.com> writes: > + char ld_preload[64] = {}; /* Has to be big enough for all cases */ ... > + if (ld_preload[0]) > + strcat(ld_preload, ":"); > + strcat(ld_preload, "/lib/libpreload-seccomp.so"); I understand C is often like that but still using strcat here makes me feel uneasy. Proper range-checking would require a bit more code, but it's a one-time coding/reviewing and a neglectable runtime cost. > +static void __attribute__((constructor)) setnbf(void) > +{ > + setbuf(stdout, NULL); > +} Not sure if no buffering or line buffering would be the best choice here.
i dont like this idea at all. calling ld-preload on every started app just seems wrong On 20/06/2015 01:03, Rafał Miłecki wrote: > Using pipe automatically switches service to block buffering which kind > of breaks our logging. We won't get anything from FD until the buffer > gets filled fully or the service exits. This makes log messages appear > with an unwanted delay. > This adds a tiny libsetnbf.so that disables stdout buffering and uses it > for every service started by procd. > > Signed-off-by: Rafał Miłecki <zajec5@gmail.com> > --- > CMakeLists.txt | 7 +++++++ > service/instance.c | 12 +++++++++++- > service/setnbf.c | 6 ++++++ > 3 files changed, 24 insertions(+), 1 deletion(-) > create mode 100644 service/setnbf.c > > diff --git a/CMakeLists.txt b/CMakeLists.txt > index dfa9413..c3b7c1e 100644 > --- a/CMakeLists.txt > +++ b/CMakeLists.txt > @@ -10,6 +10,13 @@ IF(APPLE) > LINK_DIRECTORIES(/opt/local/lib) > ENDIF() > > + > +ADD_LIBRARY(setnbf SHARED service/setnbf.c) > +INSTALL(TARGETS setnbf > + LIBRARY DESTINATION lib > +) > + > + > SET(SOURCES procd.c signal.c watchdog.c state.c inittab.c rcS.c ubus.c system.c > service/service.c service/instance.c service/validate.c service/trigger.c service/watch.c > plug/coldplug.c plug/hotplug.c utils/utils.c) > diff --git a/service/instance.c b/service/instance.c > index 35b2def..726f859 100644 > --- a/service/instance.c > +++ b/service/instance.c > @@ -224,6 +224,7 @@ instance_run(struct service_instance *in, int _stdout, int _stderr) > struct blobmsg_list_node *var; > struct blob_attr *cur; > char **argv; > + char ld_preload[64] = {}; /* Has to be big enough for all cases */ > int argc = 1; /* NULL terminated */ > int rem, _stdin; > > @@ -238,8 +239,17 @@ instance_run(struct service_instance *in, int _stdout, int _stderr) > > if (!in->trace && !in->has_jail && in->seccomp) { > setenv("SECCOMP_FILE", in->seccomp, 1); > - setenv("LD_PRELOAD", "/lib/libpreload-seccomp.so", 1); > + if (ld_preload[0]) > + strcat(ld_preload, ":"); > + strcat(ld_preload, "/lib/libpreload-seccomp.so"); > } > + if (_stdout >= 0) { > + if (ld_preload[0]) > + strcat(ld_preload, ":"); > + strcat(ld_preload, "/lib/libsetnbf.so"); > + } > + if (ld_preload[0]) > + setenv("LD_PRELOAD", ld_preload, 1); > > blobmsg_list_for_each(&in->limits, var) > instance_limits(blobmsg_name(var->data), blobmsg_data(var->data)); > diff --git a/service/setnbf.c b/service/setnbf.c > new file mode 100644 > index 0000000..7b5f3bd > --- /dev/null > +++ b/service/setnbf.c > @@ -0,0 +1,6 @@ > +#include <stdio.h> > + > +static void __attribute__((constructor)) setnbf(void) > +{ > + setbuf(stdout, NULL); > +} >
Hi John, > i dont like this idea at all. calling ld-preload on every started app > just seems wrong I was the one suggesting the idea since we needed a solution which does not require modification of downstream programs. We could restrict the preloading to programs which requested stdio relaying support from procd and not preload for the rest. ~ Jow
On 20 June 2015 at 13:56, Jo-Philipp Wich <jow@openwrt.org> wrote: >> i dont like this idea at all. calling ld-preload on every started app >> just seems wrong > > I was the one suggesting the idea since we needed a solution which does > not require modification of downstream programs. We could restrict the > preloading to programs which requested stdio relaying support from procd > and not preload for the rest. AFAIK there are 3 solutions to this: 1) Modifying every app we want to support with procd + logging 2) Using PTY which I tried in https://patchwork.ozlabs.org/patch/486670/ 3) Using LD_PRELOAD The PTY was pointed as not the best choice, so that's why I continued with LD_PRELOAD. As Jo-Philipp pointed, it's the same solution "stdbuf" uses. I'm afraid there isn't any better alternative :(
On 20/06/2015 20:53, Rafał Miłecki wrote: > On 20 June 2015 at 13:56, Jo-Philipp Wich <jow@openwrt.org> wrote: >>> i dont like this idea at all. calling ld-preload on every started app >>> just seems wrong >> >> I was the one suggesting the idea since we needed a solution which does >> not require modification of downstream programs. We could restrict the >> preloading to programs which requested stdio relaying support from procd >> and not preload for the rest. > > AFAIK there are 3 solutions to this: > 1) Modifying every app we want to support with procd + logging > 2) Using PTY which I tried in https://patchwork.ozlabs.org/patch/486670/ > 3) Using LD_PRELOAD > > The PTY was pointed as not the best choice, so that's why I continued > with LD_PRELOAD. As Jo-Philipp pointed, it's the same solution > "stdbuf" uses. I'm afraid there isn't any better alternative :( > oh well, i still dont like it but that is not really relevant i guess ... :)
Hi, 2015-06-20 21:35 GMT+02:00 John Crispin <blogic@openwrt.org>: > > > On 20/06/2015 20:53, Rafał Miłecki wrote: > > On 20 June 2015 at 13:56, Jo-Philipp Wich <jow@openwrt.org> wrote: > >>> i dont like this idea at all. calling ld-preload on every started app > >>> just seems wrong > >> > >> I was the one suggesting the idea since we needed a solution which does > >> not require modification of downstream programs. We could restrict the > >> preloading to programs which requested stdio relaying support from procd > >> and not preload for the rest. > > > > AFAIK there are 3 solutions to this: > > 1) Modifying every app we want to support with procd + logging > > 2) Using PTY which I tried in https://patchwork.ozlabs.org/patch/486670/ > > 3) Using LD_PRELOAD > > > > The PTY was pointed as not the best choice, so that's why I continued > > with LD_PRELOAD. As Jo-Philipp pointed, it's the same solution > > "stdbuf" uses. I'm afraid there isn't any better alternative :( > > > > oh well, i still dont like it but that is not really relevant i guess ... > :) > Aren't you using LD_PRELOAD for procd jailing stuff ?
On 20/06/2015 21:46, Etienne Champetier wrote: > Hi, > > 2015-06-20 21:35 GMT+02:00 John Crispin <blogic@openwrt.org > <mailto:blogic@openwrt.org>>: > > > > On 20/06/2015 20:53, Rafał Miłecki wrote: > > On 20 June 2015 at 13:56, Jo-Philipp Wich <jow@openwrt.org <mailto:jow@openwrt.org>> wrote: > >>> i dont like this idea at all. calling ld-preload on every started app > >>> just seems wrong > >> > >> I was the one suggesting the idea since we needed a solution which does > >> not require modification of downstream programs. We could restrict the > >> preloading to programs which requested stdio relaying support from procd > >> and not preload for the rest. > > > > AFAIK there are 3 solutions to this: > > 1) Modifying every app we want to support with procd + logging > > 2) Using PTY which I tried in https://patchwork.ozlabs.org/patch/486670/ > > 3) Using LD_PRELOAD > > > > The PTY was pointed as not the best choice, so that's why I continued > > with LD_PRELOAD. As Jo-Philipp pointed, it's the same solution > > "stdbuf" uses. I'm afraid there isn't any better alternative :( > > > > oh well, i still dont like it but that is not really relevant i > guess ... :) > > Aren't you using LD_PRELOAD for procd jailing stuff ? yes, i do. you simply did not understood what i said. i dont have a problem with preloading per-se but with the solution for setting the buffering. however if there really is no saner way then so be it. > > > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel >
diff --git a/CMakeLists.txt b/CMakeLists.txt index dfa9413..c3b7c1e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -10,6 +10,13 @@ IF(APPLE) LINK_DIRECTORIES(/opt/local/lib) ENDIF() + +ADD_LIBRARY(setnbf SHARED service/setnbf.c) +INSTALL(TARGETS setnbf + LIBRARY DESTINATION lib +) + + SET(SOURCES procd.c signal.c watchdog.c state.c inittab.c rcS.c ubus.c system.c service/service.c service/instance.c service/validate.c service/trigger.c service/watch.c plug/coldplug.c plug/hotplug.c utils/utils.c) diff --git a/service/instance.c b/service/instance.c index 35b2def..726f859 100644 --- a/service/instance.c +++ b/service/instance.c @@ -224,6 +224,7 @@ instance_run(struct service_instance *in, int _stdout, int _stderr) struct blobmsg_list_node *var; struct blob_attr *cur; char **argv; + char ld_preload[64] = {}; /* Has to be big enough for all cases */ int argc = 1; /* NULL terminated */ int rem, _stdin; @@ -238,8 +239,17 @@ instance_run(struct service_instance *in, int _stdout, int _stderr) if (!in->trace && !in->has_jail && in->seccomp) { setenv("SECCOMP_FILE", in->seccomp, 1); - setenv("LD_PRELOAD", "/lib/libpreload-seccomp.so", 1); + if (ld_preload[0]) + strcat(ld_preload, ":"); + strcat(ld_preload, "/lib/libpreload-seccomp.so"); } + if (_stdout >= 0) { + if (ld_preload[0]) + strcat(ld_preload, ":"); + strcat(ld_preload, "/lib/libsetnbf.so"); + } + if (ld_preload[0]) + setenv("LD_PRELOAD", ld_preload, 1); blobmsg_list_for_each(&in->limits, var) instance_limits(blobmsg_name(var->data), blobmsg_data(var->data)); diff --git a/service/setnbf.c b/service/setnbf.c new file mode 100644 index 0000000..7b5f3bd --- /dev/null +++ b/service/setnbf.c @@ -0,0 +1,6 @@ +#include <stdio.h> + +static void __attribute__((constructor)) setnbf(void) +{ + setbuf(stdout, NULL); +}
Using pipe automatically switches service to block buffering which kind of breaks our logging. We won't get anything from FD until the buffer gets filled fully or the service exits. This makes log messages appear with an unwanted delay. This adds a tiny libsetnbf.so that disables stdout buffering and uses it for every service started by procd. Signed-off-by: Rafał Miłecki <zajec5@gmail.com> --- CMakeLists.txt | 7 +++++++ service/instance.c | 12 +++++++++++- service/setnbf.c | 6 ++++++ 3 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 service/setnbf.c