nsexec.c: don't write logs that are to be discarded #3201
Conversation
|
#3157 merged. |
kolyshkin
force-pushed
the
nsexec-less-logs
branch
from
018b709 to
516b5eb
Compare
|
Rebased, no longer a draft. @cyphar @AkihiroSuda @thaJeztah PTAL |
newProcess do not need those extra arguments, they can be handled in the caller. Signed-off-by: Kir Kolyshkin <[email protected]>
Instead of passing _LIBCONTAINER_LOGLEVEL as a string (like "debug" or "info"), use a numeric value. Also, simplify the init log level passing code -- since we actually use the same level as the runc binary, just get it from logrus. This is a preparation for the next commit. Signed-off-by: Kir Kolyshkin <[email protected]>
This makes it possible to use bail() even if logging is not set up (yet), so we don't have to think whether it's OK to use it or not. In addition, this might help some unit tests that do not set log forwarding. Signed-off-by: Kir Kolyshkin <[email protected]>
The code already parses an environment variable into an integer twice, and we're about to add a third one. Factor it out to getenv_int(). Signed-off-by: Kir Kolyshkin <[email protected]>
Currently, if the log level is not set to e.g. "debug", runc init sends some debug logs to the parent, which parses and discards it. It is better to not send those in the first place. Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
force-pushed
the
nsexec-less-logs
branch
from
516b5eb to
f1b703f
Compare
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
changes in go code LGTM
I reviewed the C code as well, which looks good afaics, but I'm not a C-coder, so would appreciate someone else to also review in case I missed something.
| if (i < 0) | ||
| return; |
There was a problem hiding this comment.
Do we have a convention on wether or not to use curly braces for these? I realise it's common in C to omit them, but (e.g.) a couple of lines up we do use curly braces (given, in that case because of the comment).
Wondering if we need to have a convention (also as a safeguard), at least for "new" code.
There was a problem hiding this comment.
Good point.
In Go, obligatory curly braces (and, ultimately, gofmt) were introduced in order to minimize such discussions.
In C, curly braces are optional if the body has a single statement, which sometimes leads to bugs like this
zero();
if (val > max) /* Fixup val because something something */
val = max;
one();
two();It's kinda hard to spot that one() is called unconditionally here (especially if the code is lengthier than in this example).
Fortunately, here we use indent (which is a poor man's gofmt for C), which will show a warning about wrong indentation in CI. It won't prevent a bug, but at least it will be obvious.
This is why I think both styles (with and without braces) are OK in here.
There was a problem hiding this comment.
Yes, I'm ok with this (especially because it's already for quite some code), just have to think about Apple's infamous "goto fail" security issue every time I see code without the curly braces 😂 https://www.imperialviolet.org/2014/02/22/applebug.html
There was a problem hiding this comment.
I wish we have some linter to take care of it (I mean real linter, not the indentation checker, although it seems to alleviate the issue so it is visible to the naked eye).
3 participants
(this is based on and includes #3157; only review the last 5 commits)Currently, if the log level is not set to e.g. "debug", runc init sends
some debug logs to the parent, which parses and discards it.
It is better to not send those in the first place.
Amend nsexec.c to honor
_LIBCONTAINER_LOGLEVEL.