[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [PATCH v2] ntpd: support logging to syslog even if running in foreground
From:       Denys Vlasenko <vda.linux () googlemail ! com>
Date:       2021-10-07 20:41:31
Message-ID: CAK1hOcN_6kA241mkHQceqU=nnMdvJpyVqvFaQYOOGrDwU+9UTA () mail ! gmail ! com
[Download RAW message or body]

On Wed, Jul 7, 2021 at 12:06 AM Aleksander Mazur <deweloper@wp.pl> wrote:
> Please consider attached version of the patch, which works for me.
>
> Dnia 2021-02-08, o godz. 16:37:14
> <deweloper@wp.pl> napisał(a):
>
> > Dnia 2021-01-16, o godz. 15:11:22
> > Joachim Wiberg <troglobit@gmail.com> napisał(a):
> >
> > > This patch adds support in ntpd for logging to syslog, regardless if
> > > running in the foreground or not.
> > >
> >
> > Hi,
> >
> > Unfortunately the patch doesn't work in my case.
> > Everything is still printed to stdout, nothing to syslog.
> >
> > $ ntpd -dd -n -s -S ./script
> > ntpd: bad address 'europe.pool.ntp.org'
> > ntpd: poll:1s sockets:0 interval:1s
> > ntpd: bad address 'europe.pool.ntp.org'
> > ntpd: poll:13s sockets:0 interval:1s
> > ntpd: bad address 'europe.pool.ntp.org'
> > ntpd: poll:29s sockets:0 interval:1s
> > ^Cntpd: bad address 'europe.pool.ntp.org'
> >
> > Isn't the condition present at this line:
> > > OPT_s = (1 << (9+ENABLE_FEATURE_NTP_AUTH)) * ENABLE_FEATURE_NTPD_SERVER,
> > inconsistent with the string passed to getopt32?
> > I modified that part as follows and it started working.
> > OPT_s = (1 << (7+ENABLE_FEATURE_NTP_AUTH)),
> > OPT_l = (1 << (8+ENABLE_FEATURE_NTP_AUTH)) * ENABLE_FEATURE_NTPD_SERVER,
> > OPT_I = (1 << (9+ENABLE_FEATURE_NTP_AUTH)) * ENABLE_FEATURE_NTPD_SERVER,
> >
> > > Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
> > > ---
> > >  networking/ntpd.c | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/networking/ntpd.c b/networking/ntpd.c
> > > index 1f17b08ef..9307cb731 100644
> > > --- a/networking/ntpd.c
> > > +++ b/networking/ntpd.c
> > > @@ -72,7 +72,7 @@
> > >  //kbuild:lib-$(CONFIG_NTPD) += ntpd.o
> > >
> > >  //usage:#define ntpd_trivial_usage
> > > -//usage:   "[-dnqNw"IF_FEATURE_NTPD_SERVER("l] [-I IFACE")"] [-S
> > > PROG]" +//usage:    "[-dnqsNw"IF_FEATURE_NTPD_SERVER("l] [-I IFACE")"]
> > > [-S PROG]" //usage: IF_NOT_FEATURE_NTP_AUTH(" [-p PEER]...")
> > >  //usage:   IF_FEATURE_NTP_AUTH(" [-k KEYFILE] [-p [keyno:N:]PEER]...")
> > >  //usage:#define ntpd_full_usage "\n\n"
> > > @@ -82,6 +82,7 @@
> > >  //usage:     "\n   -q      Quit after clock is set"
> > >  //usage:     "\n   -N      Run at high priority"
> > >  //usage:     "\n   -w      Do not set time (only query peers),
> > > implies -n" +//usage:     "\n       -s      Log to syslog, even if
> > > running in foreground, -n" //usage:     "\n -S PROG Run PROG
> > > after stepping time, stratum change, and every 11 min" //usage:
> > > IF_NOT_FEATURE_NTP_AUTH( //usage:     "\n   -p PEER Obtain time
> > > from PEER (may be repeated)" @@ -107,6 +108,7 @@
> > >
> > >  #include "libbb.h"
> > >  #include <math.h>
> > > +#include <syslog.h>
> > >  #include <netinet/ip.h> /* For IPTOS_DSCP_AF21 definition */
> > >  #include <sys/timex.h>
> > >  #ifndef IPTOS_DSCP_AF21
> > > @@ -389,6 +391,7 @@ enum {
> > >     OPT_S = (1 << (6+ENABLE_FEATURE_NTP_AUTH)),
> > >     OPT_l = (1 << (7+ENABLE_FEATURE_NTP_AUTH)) *
> > > ENABLE_FEATURE_NTPD_SERVER, OPT_I = (1 << (8+ENABLE_FEATURE_NTP_AUTH)) *
> > > ENABLE_FEATURE_NTPD_SERVER,
> > > +   OPT_s = (1 << (9+ENABLE_FEATURE_NTP_AUTH)) *
> > > ENABLE_FEATURE_NTPD_SERVER, /* We hijack some bits for other purposes */
> > >     OPT_qq = (1 << 31),
> > >  };
> > > @@ -2484,6 +2487,7 @@ static NOINLINE void ntp_init(char **argv)
> > >                     IF_FEATURE_NTP_AUTH("k:")  /* compat */
> > >                     "wp:*S:"IF_FEATURE_NTPD_SERVER("l") /* NOT compat
> > > */ IF_FEATURE_NTPD_SERVER("I:") /* compat */
> > > +                   "s"    /* NOT compat */
> > >                     "d" /* compat */
> > >                     "46aAbgL" /* compat, ignored */
> > >                             "\0"
> > > @@ -2580,6 +2584,12 @@ static NOINLINE void ntp_init(char **argv)
> > >             config_close(parser);
> > >     }
> > >  #endif
> > > +
> > > +   if (opts & OPT_s) {
> > > +           openlog(applet_name, LOG_PID, LOG_DAEMON);
> > > +           logmode = LOGMODE_SYSLOG;
> > > +   }
> > > +
> > >     if (peers) {
> > >  #if ENABLE_FEATURE_NTP_AUTH
> > >             while (peers) {

I was sitting on this so long, I almost forgot about it.
Sorry.

So... we have a problem that we are not compatible with "standard" ntpd,
and I would like to fix this. But your patch goes into the direction
of becoming even more incompatible, since "standard" ntpd already has -s:

 -s statsdir
      Specify the directory path for files created by the statistics
      facility. This is the same operation as the "statsdir DIR"
      configuration command.

Since "standard" ntpd has ~26 options already,
I think we need to bite the bullet and add long options to our ntpd,
they tend to be more collision-less.

Please resend the patch with --syslog option.

The longer-term plan: add --peer and --server options, deprecate -p and -l.
MAybe add --iface and deprecate -I too. (Why did I even add -I in 2014??).
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[prev in list] [next in list] [prev in thread] [next in thread]