Skip to content
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

Split daemon's logging backends in separate modules #487

Merged
merged 1 commit into from
Jun 4, 2017

Conversation

nurupo
Copy link
Member

@nurupo nurupo commented Mar 2, 2017

  • log_backend.h defines an interface for logging back-ends, which log_backend_[stdout|syslog].[h|c] implement. These implementations are then put together into a BACKENDS array in log.c, which is indexed by the LOG_BACKEND enum. Haven't really done any such modular implementations in C before, so any corrections are welcome.

  • Renamed

    • open_log() --> log_open()
    • close_log() --> log_close()
    • write_log() --> log_write()

    Should probably have been a separate commit/PR, but I happened to do the renaming and modularization at the same time, so there we go.

I meant to make logging modular from the beginning, but at the time of writing it I thought that maybe it's simpler to keep all logging backends relatively separate in log.c as there is so little code, instead of scattering them in multiple files and adding this struct with function pointers glue, at least until there is more code for the logging that would warrant the splitting, but there was some confusion as to why I want to keep backends relatively separate

The part I don't understand is the "keeping backends separate". You still have explicit knowledge of every backend in this function. What is the advantage of having my_foo(...) instead of foo(...)?
-- @iphydf

, so this PR should remove this confusion now that the logging backends are truly separate.


This change is Reviewable

@nurupo nurupo added this to the v0.1.8 milestone Mar 2, 2017
@nurupo
Copy link
Member Author

nurupo commented Mar 3, 2017

@Zer0-One says that I might be over-engineering it with this PR, as there is no reason for having more than two backends -- one for running in foreground (stdout) and one for the background (syslog).

I still think that putting stdout/syslog into separate files makes log.c a lot cleaner, and that having the backend abstraction is still useful even for just 2 backends.

If you really insist that it's over-engineering, I could compromise on keeping stdout and syslog bakends in separate files, but removing the the support for adding additional logging backends: remove log_backend.h which defines the common interface for backends, remove the open() and close() functions from log_backend_stdout.[c|h] as they are noops and with the backend interface gone we don't have to have them, and make log.c call the backend functions directly in switch-case on the LOG_BACKEND enum instead of using the BACKENDS array since there is no backend interface anymore.

@Zer0-One
Copy link
Member

Zer0-One commented Mar 4, 2017

I like your second suggestion more, but IMO this is simple enough to keep within a single module.


#include <stdarg.h>

typedef struct Log_Backend_Impl {
Copy link
Member

Choose a reason for hiding this comment

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

All of this can be moved to the .c file. It's used only once.

@iphydf
Copy link
Member

iphydf commented Mar 5, 2017

Check the checkbox for collaborator pushes.

@nurupo
Copy link
Member Author

nurupo commented Mar 6, 2017

Checked.

@iphydf iphydf requested review from iphydf and removed request for iphydf March 26, 2017 19:29
@iphydf iphydf removed their assignment Mar 26, 2017
@iphydf
Copy link
Member

iphydf commented Apr 12, 2017

I'm ok with this change. In fact, I really don't care about the bootstrap daemon's logging backend design. Are we going to merge this?

@iphydf iphydf modified the milestones: v0.1.8, v0.1.9 Apr 12, 2017
@nurupo
Copy link
Member Author

nurupo commented Apr 13, 2017

Oh, totally forgot about this PR. I guess I have to modify the code to the compromise I have described in #487 (comment).

@nurupo nurupo force-pushed the daemon-modulerize-logging branch 3 times, most recently from d1664a1 to b306af7 Compare April 13, 2017 04:54
@nurupo
Copy link
Member Author

nurupo commented Apr 13, 2017

@Zer0-One does the last commit look better? If so, I will squash the commits.

@CLAassistant
Copy link

CLAassistant commented Jun 3, 2017

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@iphydf
Copy link
Member

iphydf commented Jun 4, 2017

@nurupo travis is happy. Can you squash the commits?

@nurupo nurupo force-pushed the daemon-modulerize-logging branch from ed2aee0 to b0aec02 Compare June 4, 2017 20:07
@nurupo
Copy link
Member Author

nurupo commented Jun 4, 2017

Done.

@iphydf
Copy link
Member

iphydf commented Jun 4, 2017

:lgtm_strong:


Review status: 0 of 11 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@iphydf iphydf merged commit b0aec02 into TokTok:master Jun 4, 2017
@iphydf iphydf added refactor Refactoring production code, eg. renaming a variable, not affecting semantics and removed code cleanup labels May 4, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactoring production code, eg. renaming a variable, not affecting semantics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants