Improve nsexec logging#2836
Merged
cyphar merged 2 commits intoopencontainers:masterfrom Apr 13, 2021
Merged
Conversation
This was referenced Mar 8, 2021
Closed
kolyshkin
force-pushed
the
improve-nsexec-logging
branch
3 times, most recently
from
d1e0960 to
73d5aba
Compare
Member
|
This still draft? |
Contributor
Author
|
No longer a draft; @cyphar PTAL (this is mostly your code) |
In order to make 'runc --debug' actually useful for debugging nsexec bugs, provide information about all the internal operations when in debug mode. [@kolyshkin: rebasing; fix formatting via indent for make validate to pass] Signed-off-by: Aleksa Sarai <asarai@suse.de> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
force-pushed
the
improve-nsexec-logging
branch
from
73d5aba to
8c15967
Compare
Contributor
Author
|
Rebased again; PTAL @cyphar |
Contributor
Author
|
centos7 fails This is the second time I see it, not sure what's happening but definitely unrelated to this PR. CI restarted. |
cyphar
previously approved these changes
Apr 9, 2021
Member
cyphar
left a comment
cyphar
left a comment
There was a problem hiding this comment.
LGTM. I'll take your word for it that most of this new code is mine. 😬
Contributor
Author
|
CentOS CI failure is a flake (#2760), restarting. |
mrunalp
reviewed
Apr 12, 2021
Since the previous commit, some strings logged by write_log() contain a
literal newline, which leads to errors like this one:
> # time="2020-06-07T15:41:37Z" level=error msg="failed to decode \"{\\\"level\\\":\\\"debug\\\", \\\"msg\\\": \\\"nsexec-0[2265]: update /proc/2266/uid_map to '0 1000 1\\n\" to json: invalid character '\\n' in string literal"
The fix is to escape such characters.
Add a simple (as much as it can be) routine which implements JSON string
escaping as required by RFC4627, section 2.5, plus escaping of DEL (0x7f)
character (not required, but allowed by the standard, and usually done
by tools such as jq).
As much as I hate to code something like this, I was not able to find
a ready to consume and decent C implementation (not using glib).
Added a test case (and some additional asserts in C code, conditionally
enabled by the test case) to make sure the implementation is correct.
The test case have to live in a separate directory so we can use
different C flags to compile the test, and use C from go test.
[v2: try to simplify the code, add more tests]
[v3: don't do exit(1), try returning an error instead]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
force-pushed
the
improve-nsexec-logging
branch
from
8c15967 to
928ef7a
Compare
mrunalp
approved these changes
Apr 12, 2021
Contributor
Author
|
@cyphar PTAL; the only change after your review was this: #2836 (comment) |
cyphar
approved these changes
Apr 13, 2021
| free(s); | ||
| // As malloc failed, strdup can fail, too, so in the worst case | ||
| // scenario NULL will be returned from here. | ||
| return strdup("escape_json_string: out of memory"); |
Member
There was a problem hiding this comment.
I feel like it makes more sense to always return NULL (and then in the caller provide the fallback error message), but it doesn't really matter imho.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft as it's blocked by #2835In order to make
runc --debugactually useful for debugging nsexecbugs, provide information about all the internal operations when in
debug mode.
Before:
After:
This is a carry of #2460 with a few fixes on top:
make cfmt;Closes: #2460
(this was originally developed in #2487)