Message ID | 20200130173722.75554-1-ldir@darbyshire-bryant.me.uk |
---|---|
State | Rejected |
Headers | show |
Series | [OpenWrt-Devel,procd] instance: Improve missing jail binary message | expand |
Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> [2020-01-30 17:37:23]: Hi Kevin, thanks for looking into that. > Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> > --- > service/instance.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/service/instance.c b/service/instance.c > index e872ba0..b78a65f 100644 > --- a/service/instance.c > +++ b/service/instance.c > @@ -824,7 +824,8 @@ instance_jail_parse(struct service_instance *in, struct blob_attr *attr) > > r = stat(UJAIL_BIN_PATH, &s); > if (r < 0) { > - ERROR("unable to find %s: %m (%d)\n", UJAIL_BIN_PATH, r); > + ULOG_WARN("Cannot jail service %s::%s. %s: %m (%d) Are jails enabled?\n", > + in->srv->name, in->name, UJAIL_BIN_PATH, r); I added this message in commit 557f11b3a20f ("instance: provide error feedback if ujail binary is missing") because I've spent non trivial amount of time on finding this, but back then I actually didn't realized, that this code path is probed every time, leading to this error messages in cases where the ujail binary is absent (most of the time as of now). This change with service name/instance is indeed helpful, but it doesn't solve current issue, that we produce this perhaps misleading error/warning every time when procd (re)starts service which contains jails features in its init script. I'm not sure if it makes sense to waste more time on this as there is a plan to make jails enabled by default soon. If there is a will to make the UX better, then I see following solutions: 1. turn that ULOG_WARN into DEBUG, because I think, that most of us increases procd's debug level while debugging the init foo issues, we wont pollute logs anymore 2. move the ujail detection logic into procd's init library and send service jail params to procd only if it makes sense and adjust procd code accordingly if necessary Just my two cents. -- ynezz
diff --git a/service/instance.c b/service/instance.c index e872ba0..b78a65f 100644 --- a/service/instance.c +++ b/service/instance.c @@ -824,7 +824,8 @@ instance_jail_parse(struct service_instance *in, struct blob_attr *attr) r = stat(UJAIL_BIN_PATH, &s); if (r < 0) { - ERROR("unable to find %s: %m (%d)\n", UJAIL_BIN_PATH, r); + ULOG_WARN("Cannot jail service %s::%s. %s: %m (%d) Are jails enabled?\n", + in->srv->name, in->name, UJAIL_BIN_PATH, r); return 0; }
Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> --- service/instance.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)