-
Notifications
You must be signed in to change notification settings - Fork 305
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sdjournal: add GetEntry method to retrieve all fields #164
Conversation
0cfaefa
to
eacd850
Compare
Looks good to me : should it support JsonEncode to produce a matching output? |
@wrouesnel sounds reasonable but I would vote not to do so. I think json encoding is out of this package scope (although in my use-case would be a really helpful shortcut...) WDYT on this @lucab @jonboulle? |
Personally, I also think json encoding is out of scope here and easily doable in the consuming application. (I just had a quick glance at code, haven't reviewed yet) |
Ok, thanks! |
if got != want { | ||
t.Fatalf("Bad result for entry.Fields[\"MESSAGE\"]: got %s, want %s", got, want) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is fragile, it will easily pick up messages from other tests and get confused. I think you should instead filter on something a bit more specific (for example introducing a custom field key with a random entry, and matching on that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe filter by _PID
(though beware concurrent tests).
Some remarks inline. I think the test definitely needs to be reworked, I already saw it failing several times here (it is a pity this can't be run on Travis). |
@ wrouesnel @wolfeidau: as original supporters of this, do you have any other concerns with it? Does it cover your usecases (minus the json part)? |
None from me! |
eacd850
to
975031c
Compare
@lucab PTAL, i fixed your comments and reworked the test using a custom field with a timestamp as a random string, WDYT? |
@glerchundi Thanks, I think it is much better now. I'll take another pass at this, just to double-check the C-casting part. In the meanwhile, would you mind word-wrapping the commit message as described in the contributing page and add a "fixes" line for #142? |
975031c
to
559bbee
Compare
@lucab done, I referenced the issue in the commit, PTAL whenever you have time, thanks :) |
559bbee
to
8bea57f
Compare
I reread every comments and splitted this PR in two commits, one for the implementation itself and the other for facilitating access to the fields themselves. |
c8324a7
to
3b78080
Compare
I think you just hit an unrelated bug here, as most of the time the test fails for me with:
Hardcoding the match filter works, though, so I'm wondering if the bug lies somewhere else in the match subsystem. Is the test working fine on your system? Which systemd version are you running? |
@lucab wow, that's weird, I run the test against two environments, Debian and Ubuntu (both inside virtual machines):
And it worked on both...., which systemd version are you using? |
8562d27
to
ed33ef2
Compare
@lucab can you try if doing some retries, in this case 5 (I made this in the same way your reading follower does), works on your machine? Maybe journald is not emitting the result that fast to the sd_* subsystem and we're facing a race condition here. |
@glerchundi yes I think I was hitting the R/W race you describe, just retrying one more time seems to reliably pass the test. I have no better proposals here, so I'll merge it as is. |
@lucab ok, wait a minute I slightly changed just for your use case, i'll rever to the last time you reviewed. |
ed33ef2
to
a443091
Compare
@glerchundi ah, no hurry then. I actually think you can leave the retry there, as the test is inherently racy. |
Ah, I reverted already because I'm quite sure once the EOF (r==0) is emitted next wait will fail so if you don't care I think it's better to keep it as simple as possible and think in a better solution, WDYT? |
a443091
to
896af86
Compare
…elds Implements the same semantics as the JOURNAL_FOREACH_DATA_RETVAL macro in journal-internal.h and produces a *JournalEntry with every field with its corresponding data in a map[string]string. Also it includes address fields like realtime (__REALTIME_TIMESTAMP), monotonic (__MONOTONIC_TIMESTAMP and cursor (__CURSOR). It mimics more or less `journalctl -o json`. Fixes coreos#142 Supersedes coreos#136
As GetEntry outputs every field in a string keyed map, it eases access to a concrete field without hardcoding it. Ref: https://www.freedesktop.org/software/systemd/man/systemd.journal-fields.html
896af86
to
7fd54aa
Compare
@lucab should be mergeable now, recleaned commits, squashed, rebased and that git hell. |
@glerchundi sorry for the confusion 😄. Yes, it is ok to land this as is and then explore other issues/races separately. I just double-checked the commits in this branch and they seem ok now. May I proceed? |
Yes please! |
🎉 🎉 Thanks for your time @lucab |
Implements the same semantics as the JOURNAL_FOREACH_DATA_RETVAL macro in journal-internal.h and produces a *JournalEntry with every field with its corresponding data in a map[string]string. Also it includes address fields like realtime (__REALTIME_TIMESTAMP), monotonic (__MONOTONIC_TIMESTAMP and cursor (__CURSOR).
It mimics more or less
journalctl -o json
.Fixes #142
Supersedes #136