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

compatibility with upstream and usermode #4

Closed
nuno-silva opened this issue Dec 9, 2018 · 8 comments
Closed

compatibility with upstream and usermode #4

nuno-silva opened this issue Dec 9, 2018 · 8 comments

Comments

@nuno-silva
Copy link

nuno-silva commented Dec 9, 2018

I'm using ntpclient to monitor the clock offset of a bunch of machines.

With the original ntpclient version, I could easily get this information (root or not):

root$ ntpclient -c1 -h pool.ntp.org
43441 60350.979    1963.0      5.9     890.3  21179.2     11344

In troglobit's fork, '-h' was changed to mean 'help' and the output goes to stderr (why?) so I have to adjust a few flags and redirect the output to stdout. It broke out of the box compatibility with the old version, but so far I can still live with that (as non root):

nobody$ ntpclient -c1 pool.ntp.org 2>&1
43441 58627.120    2270.0      8.3      27.8  63232.4     66222

Now the problem arises when I want to do the same thing as root. First, I need to disable daemon mode, which is enabled by default for root 😞. However, this still does not work! When running as root, the output is disabled and I need to enable debug mode to see anything (which also adds other unwanted messages):

root$ ntpclient -c1 -n -d pool.ntp.org 2>&1
packet of length 48 received
43441 59793.248    2063.0     34.0    5109.1  42770.4     -4915

To make it worse, the only way I could find this was by looking at the code:

https://github.com/troglobit/ntpclient/blob/435e52063a863e1b1ef0463f78cb1b156cd1bd00/src/ntpclient.c#L477-L483

That being said, I have two suggestions to solve this:

TL;DR: this fork adds a daemon mode to ntpclient, but almost breaks its original purpose. It would be good to not assume behaviour based on the EUID or to at least introduce a 'usermode' flag.

I don't mind preparing a pull request after getting some feedback on this.

@troglobit
Copy link
Owner

I can agree with you that the behavior should probably be more similar when running as root and non-root, but I don't agree entirely with your proposal. I forked ntpclient because I needed a small SNTP daemon to run in the background. For that I wanted it to fork and use syslog by default when started as root, as all UNIX daemons should do. Hence the "original purpose" is kind of a moot argument to bring up just because my version of ntpclient doesn't do what you want.

So, what I propose is to make usermode behave like when running as root, i.e. one would have to start with -n and -d to get the same debug output. But since a regular user cannot set the time I also propose to just exit if usermode and not debug mode. What do you think?

@nuno-silva
Copy link
Author

That would break compatibility not only for root, but also for non-root users.

For context, I came up with this problem because Gentoo Linux's ntpclient package recently started to use this fork. This, of course, broke a script I had done on top of ntpclient, as described in my original comment.

The clock skew messages that this fork considers as "debug output" where always printed in the original ntpclient (except the headers) and, at least for me, were the only point of using this program.

I understand you needed a simple daemon. What I don't understand is why the original behaviour of the program had to be changed by default. It no longer does the same thing... I think you should've either renamed it to something like ntpclient-daemon, or added the new functionality but keep it optional (e.g. --fork or --daemon flag). This would allow this fork to be a drop-in replacement for the original ntpclient with additional functionality that does not break anything.

@troglobit
Copy link
Owner

I see your point and understand your problem, but I hope you understand that I do Open Source primarily for myself and not make everyone else happy.

When I get some time over I may rename this project sntpd, which could use the new behavior, and a symlink called ntpclient which runs in the backwards compatible mode. That seems like the only way forward here to please more people.

@bes-internal
Copy link

I also use Gentoo on working servers and support this package in the tree, because it is a wonderful piece of code as a replacement for OpenNTPD. And I was very pleased that the project was supported and special thanks troglobit for that.

I have a few comments :

  1. Separating program behavior from the user launching it is a very strange practice, at least in *nix and just a bad user experience. Not only the root can be privileged to run daemons and etc. We should see error messages if there are not enough rights for some operation.
  2. In fact, backward compatibility is broken, but not in the “don't touch anything” thoughts, but the simplest way to display metrics as is (just to see how far behind you is). For example, it can be new -u for combination of -n -c1 and stdout. In conjunction with -v it can be a beautiful detailed and described output of all metrics.
  3. Open Source primarily for you - you give, but you get. In short, it is impossible for this program to become huge (if you start to please everyone), but we are now talking about an small alias for existing functionality. So I hope you, as an author, solve this problem for the whole community.

@troglobit
Copy link
Owner

troglobit commented Dec 19, 2018

First of all, I had no idea Gentoo had packaged my fork of ntpclient. That is great, but please don't expect Open Source software authors to fix issues and regressions to some older pre-forked version on the spot, and for free. I did most of my modifications to ntpclient back in 2010 (actually earlier, but published it then) to scratch an itch for the company I work for.

Eventually I may circle back to ntpclient and, as I mentioned above, do some cleanup and possibly rename the project. But until then, this is GitHub, if you don't like what you see, press the Fork button and submit a pull request.

troglobit added a commit that referenced this issue Dec 28, 2018
As mentioned in #4, this project has diverged quite a bit from Larry's
original ntpclient.  It is therefore time to rename it to avoid any more
connfusion.

Signed-off-by: Joachim Nilsson <troglobit@gmail.com>
@troglobit
Copy link
Owner

There you go. Not with a proper release yet, but the project has now been renamed sntpd and when called as ntpclient it (should) now works as the original by Larry. If you find issues with that compat mode, please file another bug report.

Happy New Year!

@bes-internal
Copy link

Fine! Great achievements in the new year and long life for this project

@nuno-silva
Copy link
Author

Thank you for your work! I'll give it a try as soon as I get a chance! Happy New Year!

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

No branches or pull requests

3 participants