Skip to content

journalctl: add remote log dir to search path when --merge is passed#4970

Merged
poettering merged 1 commit intosystemd:masterfrom
mbachry:journalctl-merge-fix
Dec 23, 2016
Merged

journalctl: add remote log dir to search path when --merge is passed#4970
poettering merged 1 commit intosystemd:masterfrom
mbachry:journalctl-merge-fix

Conversation

@mbachry
Copy link
Copy Markdown
Contributor

@mbachry mbachry commented Dec 23, 2016

The journalctl man page says: "-m, --merge Show entries interleaved from all available journals, including remote ones.", but current version of journalctl doesn't live up to this promise. This patch simply adds /var/log/journal/remote to search path if --merge flag is used.

Should fix issue #3618

poettering
poettering reviewed Dec 23, 2016
View reviewed changes
Comment thread src/journal/sd-journal.c Outdated
NULSTR_FOREACH(p, search_paths)
(void) add_root_directory(j, p, true);

if (!j->flags & SD_JOURNAL_LOCAL_ONLY)
Copy link
Copy Markdown
Member

@poettering poettering Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't look right. you are using a boolean negation of the flags field. I figure you mean:

if (!(j->flags & SD_JOURNAL_LOCAL_ONLY))

Other than that I think the patch looks OK

@poettering poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Dec 23, 2016
@mbachry
journalctl: add remote log dir to search path when --merge is passed
1821ee3 
The journalctl man page says: "-m, --merge Show entries interleaved from all
available journals, including remote ones.", but current version of journalctl
doesn't live up to this promise. This patch simply adds
"/var/log/journal/remote" to search path if --merge flag is used.

Should fix issue systemd#3618
@mbachry mbachry force-pushed the journalctl-merge-fix branch from 9d666fc to 1821ee3 Compare December 23, 2016 21:22
@mbachry
Copy link
Copy Markdown
Contributor Author

mbachry commented Dec 23, 2016

Thanks for quick review. I force pushed the fix.

@poettering
Copy link
Copy Markdown
Member

poettering commented Dec 23, 2016

lgtm

@poettering poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Dec 23, 2016
@poettering poettering merged commit 574b77e into systemd:master Dec 23, 2016
@poettering poettering mentioned this pull request Dec 24, 2016
Closed
1 task
@makeamark1
Copy link
Copy Markdown

makeamark1 commented Sep 27, 2018 •
edited
Loading

@mbachry @poettering This pull request added /var/log/journal/remote to the search path, which is good. Should also /run/log/journal/remote then be added to the search path in the same place? This would be consistent with how both /var/log/journal and /run/log/journal are added to the search path a couple of lines above. There are use cases where remote logs do not need to be persistent, so it would be useful for --merge to work also in those cases.

On the other hand, I just found the following comment elsewhere in the same file: We consider everything local that is in a directory for the local machine ID, or that is stored in /run. Is it really correct though to force everything in /run to be considered local? Seems like /run/log/journal/remote would be useful for remote logs that do not need to be persisted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@poettering poettering poettering left review comments

Assignees

No one assigned

Labels

good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed journal

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mbachry @poettering @makeamark1