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

Do not spit on the console 2nd stage (inspired by Debian) #1762

Closed
metalefty opened this issue Dec 24, 2020 · 3 comments · Fixed by #1765
Closed

Do not spit on the console 2nd stage (inspired by Debian) #1762

metalefty opened this issue Dec 24, 2020 · 3 comments · Fixed by #1765

Comments

@metalefty
Copy link
Member

Thank you for all of your contributions, @aquesnel, especially logging improvement.

I would like to share some basic directions about logging on the console.

As Debian's downstream patch expresses, they don't like xrdp spit logs on the console when it runs as daemons. I agree with Debian guys at this point.

It is partially done in #1142. I would like to complete this. Here's my thoughts about logging:

  1. xrdp and xrdp-sesman should not write anything on stdout/stderr when they run as daemons (except fatal errors)
  2. information such as configuration summaries can be dumped when it runs in the foreground

Any feedback is welcome, thanks.

Thoughts:

I personally would like to pick downstream patches up and merge proactively if they're useful to everyone. Because I'm an upstream developer and also a distro package maintainer. Upstream first is a good strategy in most cases. Less downstream patches should make both upstream and downstream happy.

@aquesnel
Copy link
Contributor

hi @metalefty ,

Thanks for asking for my opinion.
I agree with you about only logging fatal messages to the console when running as a daemon. I also agree that configuration being dumped to the console when not running as a daemon is a good idea.

I looked at the Debian patch that you linked to, and I don't think it should be applyed it as is because it unconditionally removes useful debugging information. I think that xrdp should conditionally output that information if the user asks for it or create a config that downstream maintainers can define a default for their distribution to disable the information.

I think that a solution based on the logging config will have the biggest impact because as I continue to migrate to the logging to the unified macros across the whole code base then not writing to the console will just follow the configuration.

My ideas for addressing this issue are:

  • set the default log level for the console to error or fatal
  • or, when running as a daemon conditionally set the default log level for the console to error or fatal, and if the not running as a daemon then leave the console default to info.
  • or, add a command line parameter to enable dumping the config to the console on start up

@matt335672
Copy link
Member

Hi both,

I added an additional member dump_on_start to struct log_config as part of #1711. At the moment this is used to suppress dumping the logging output to stdout when running a console utility like sesrun which is (I think) exactly what you're trying to do here. It doesn't currently have an easy way to set it in other circumstances.

I was going to suggest using this, but on the other hand I think it might be best to remove it entirely.

If I take @aquesnel's suggestions above as a starting point, this gives the following outline implementation:-

  • Remove dump_on_start from struct log_config
  • Add an extra boolean argument to log_start() to dump the logging config (log_start() is only used by xrdp and xrdp-sesman)
  • Plumb in an extra command-line parameter to xrdp and xrdp-sesman to allow the boolean value to be changed from whatever the default is.
  • Update the man pages for xrdp and xrdp-sesman

Maybe roughly an hour's work with review time on top.

Does that sound like it's along the right lines? In particular @aquesnel, is it compatible with what you see as the future direction of logging?

@aquesnel
Copy link
Contributor

hi both,

Yes @matt335672 , I agree with your suggested outline.

My plans for logging are to continue the logging macro unification work that I started in #1633 and to add more detailed trace logging like what I've done in #1742. So the changes here are compatible with the work I'm doing.

@matt335672 matt335672 linked a pull request Jan 7, 2021 that will close this issue
matt335672 added a commit that referenced this issue Jan 7, 2021
Remove output on stdout by default on daemon startup (#1762)
metalefty added a commit to metalefty/xrdp that referenced this issue Dec 8, 2021
Picked from Debian's local patch [1].

We have worked on reducing Debian's local patches in neutrinolabs#1762 neutrinolabs#1765.
This is the last one left and helps to  remove shutup-daemon.diff
completely. I don't think this log to stdout is necessary.

[1] https://salsa.debian.org/debian-remote-team/xrdp/-/blob/debian/0.9.17-2/debian/patches/shutup-daemon.diff
derekschrock pushed a commit to derekschrock/xrdp that referenced this issue Dec 15, 2022
Picked from Debian's local patch [1].

We have worked on reducing Debian's local patches in neutrinolabs#1762 neutrinolabs#1765.
This is the last one left and helps to  remove shutup-daemon.diff
completely. I don't think this log to stdout is necessary.

[1] https://salsa.debian.org/debian-remote-team/xrdp/-/blob/debian/0.9.17-2/debian/patches/shutup-daemon.diff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants