Re: ifstated: consistent use of log.c

[Jeremie Courreges-Anglas]

On Mon, Aug 07 2017, Rob Pierce  wrote:
> On Sun, Aug 06, 2017 at 06:47:38PM +0200, Jeremie Courreges-Anglas wrote:
>> On Thu, Aug 03 2017, Rob Pierce  wrote:
>> > As a result ifstated.c no longer needs err.h.
>> >
>> > Index: ifstated.c
>> > ===
>> > RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
>> > retrieving revision 1.56
>> > diff -u -p -r1.56 ifstated.c
>> > --- ifstated.c 24 Jul 2017 12:33:59 -  1.56
>> > +++ ifstated.c 3 Aug 2017 23:59:13 -
>> > @@ -37,7 +37,6 @@
>> >  #include 
>> >  #include 
>> >  #include 
>> > -#include 
>> >  #include 
>> >  #include 
>> >  #include 
>> > @@ -102,7 +101,7 @@ main(int argc, char *argv[])
>> >break;
>> >case 'D':
>> >if (cmdline_symset(optarg) < 0)
>> > -  errx(1, "could not parse macro definition %s",
>> > +  fatalx("could not parse macro definition %s",
>> >optarg);
>> >break;
>> >case 'f':
>> > @@ -135,7 +134,7 @@ main(int argc, char *argv[])
>> >if (opts & IFSD_OPT_NOACTION) {
>> >if ((newconf = parse_config(configfile, opts)) == NULL)
>> >exit(1);
>> > -  warnx("configuration OK");
>> > +  fprintf(stderr, "configuration OK\n");
>> 
>> This changes the output from
>> 
>>   ifstated: configuration OK
>> 
>> to
>> 
>>   configuration OK
>> 
>> which is a bit less helpful.  To keep the output the same, you could use
>> warnx, fprintf + __progname/getprogname instead, or just
>> 
>>  errx(0, "configuration OK");
>> 
>> I don't really have a preference here.
>
> Good catch. I honestly didn't notice the change in output that was introduced
> by my diff. I was following the code in other networking daemons.
>
> I would like to standardize on log.c and remove the err.h include, so maybe
> the __progname approach is best if we want to keep the same output, or maybe
> having a few lingering uses of err.h is ok.
>
> Many network daemons (below) do not reference themselves when confirming the
> validity of their configuration file with the -n option. Is it better for
> ifstated to retain it's historical output on the configuration check, or to be
> made more consistent with other network daemons?

Hah, thanks for checking the other daemons.  I'm fine with your first
diff then, ok jca@

> Regards,
>
> Rob
>
> bgpd/bgpd.c:  fprintf(stderr, "configuration OK\n");
> dvmrpd/dvmrpd.c:  fprintf(stderr, "configuration OK\n");
> eigrpd/eigrpd.c:  fprintf(stderr, "configuration OK\n");
> httpd/httpd.c:fprintf(stderr, "configuration OK\n");
> ldapd/ldapd.c:fprintf(stderr, "configuration ok\n");
> ldpd/ldpd.c:  fprintf(stderr, "configuration OK\n");
> ntpd/ntpd.c:  fprintf(stderr, "configuration OK\n");
> ospf6d/ospf6d.c:  fprintf(stderr, "configuration OK\n");
> ospfd/ospfd.c:fprintf(stderr, "configuration OK\n");
> radiusd/radiusd.c:fprintf(stderr, "configuration OK\n");
> relayd/relayd.c:  fprintf(stderr, "configuration OK\n");
> ripd/ripd.c:  fprintf(stderr, "configuration OK\n");
> sasyncd/sasyncd.c:fprintf(stderr, "configuration OK\n");
> smtpd/smtpd.c:fprintf(stderr, "configuration OK\n");
> snmpd/snmpd.c:fprintf(stderr, "configuration ok\n");
> switchd/switchd.c:fprintf(stderr, "configuration OK\n");
> vmd/vmd.c:fprintf(stderr, "configuration OK\n");
> ypldap/ypldap.c:  fprintf(stderr, "configuration OK\n");
>
>> >exit(0);
>> >}
>> >  
>> > @@ -147,7 +146,7 @@ main(int argc, char *argv[])
>> >log_setverbose(opts & IFSD_OPT_VERBOSE);
>> >  
>> >if ((rt_fd = socket(PF_ROUTE, SOCK_RAW, 0)) < 0)
>> > -  err(1, "no routing socket");
>> > +  fatal("no routing socket");
>> >  
>> >rtfilter = ROUTE_FILTER(RTM_IFINFO);
>> >if (setsockopt(rt_fd, PF_ROUTE, ROUTE_MSGFILTER,
>> > @@ -604,7 +603,7 @@ fetch_ifstate(void)
>> >struct ifaddrs *ifap, *ifa;
>> >  
>> >if (getifaddrs() != 0)
>> > -  err(1, "getifaddrs");
>> > +  fatal("getifaddrs");
>> >  
>> >for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
>> >if (ifa->ifa_addr->sa_family == AF_LINK) {
>> >
>> 
>> 
>> -- 
>> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
>

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: ifstated: consistent use of log.c

[Rob Pierce]

On Sun, Aug 06, 2017 at 06:47:38PM +0200, Jeremie Courreges-Anglas wrote:
> On Thu, Aug 03 2017, Rob Pierce  wrote:
> > As a result ifstated.c no longer needs err.h.
> >
> > Index: ifstated.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
> > retrieving revision 1.56
> > diff -u -p -r1.56 ifstated.c
> > --- ifstated.c  24 Jul 2017 12:33:59 -  1.56
> > +++ ifstated.c  3 Aug 2017 23:59:13 -
> > @@ -37,7 +37,6 @@
> >  #include 
> >  #include 
> >  #include 
> > -#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -102,7 +101,7 @@ main(int argc, char *argv[])
> > break;
> > case 'D':
> > if (cmdline_symset(optarg) < 0)
> > -   errx(1, "could not parse macro definition %s",
> > +   fatalx("could not parse macro definition %s",
> > optarg);
> > break;
> > case 'f':
> > @@ -135,7 +134,7 @@ main(int argc, char *argv[])
> > if (opts & IFSD_OPT_NOACTION) {
> > if ((newconf = parse_config(configfile, opts)) == NULL)
> > exit(1);
> > -   warnx("configuration OK");
> > +   fprintf(stderr, "configuration OK\n");
> 
> This changes the output from
> 
>   ifstated: configuration OK
> 
> to
> 
>   configuration OK
> 
> which is a bit less helpful.  To keep the output the same, you could use
> warnx, fprintf + __progname/getprogname instead, or just
> 
>   errx(0, "configuration OK");
> 
> I don't really have a preference here.

Good catch. I honestly didn't notice the change in output that was introduced
by my diff. I was following the code in other networking daemons.

I would like to standardize on log.c and remove the err.h include, so maybe
the __progname approach is best if we want to keep the same output, or maybe
having a few lingering uses of err.h is ok.

Many network daemons (below) do not reference themselves when confirming the
validity of their configuration file with the -n option. Is it better for
ifstated to retain it's historical output on the configuration check, or to be
made more consistent with other network daemons?

Regards,

Rob

bgpd/bgpd.c:fprintf(stderr, "configuration OK\n");
dvmrpd/dvmrpd.c:fprintf(stderr, "configuration OK\n");
eigrpd/eigrpd.c:fprintf(stderr, "configuration OK\n");
httpd/httpd.c:  fprintf(stderr, "configuration OK\n");
ldapd/ldapd.c:  fprintf(stderr, "configuration ok\n");
ldpd/ldpd.c:fprintf(stderr, "configuration OK\n");
ntpd/ntpd.c:fprintf(stderr, "configuration OK\n");
ospf6d/ospf6d.c:fprintf(stderr, "configuration OK\n");
ospfd/ospfd.c:  fprintf(stderr, "configuration OK\n");
radiusd/radiusd.c:  fprintf(stderr, "configuration OK\n");
relayd/relayd.c:fprintf(stderr, "configuration OK\n");
ripd/ripd.c:fprintf(stderr, "configuration OK\n");
sasyncd/sasyncd.c:  fprintf(stderr, "configuration OK\n");
smtpd/smtpd.c:  fprintf(stderr, "configuration OK\n");
snmpd/snmpd.c:  fprintf(stderr, "configuration ok\n");
switchd/switchd.c:  fprintf(stderr, "configuration OK\n");
vmd/vmd.c:  fprintf(stderr, "configuration OK\n");
ypldap/ypldap.c:fprintf(stderr, "configuration OK\n");

> > exit(0);
> > }
> >  
> > @@ -147,7 +146,7 @@ main(int argc, char *argv[])
> > log_setverbose(opts & IFSD_OPT_VERBOSE);
> >  
> > if ((rt_fd = socket(PF_ROUTE, SOCK_RAW, 0)) < 0)
> > -   err(1, "no routing socket");
> > +   fatal("no routing socket");
> >  
> > rtfilter = ROUTE_FILTER(RTM_IFINFO);
> > if (setsockopt(rt_fd, PF_ROUTE, ROUTE_MSGFILTER,
> > @@ -604,7 +603,7 @@ fetch_ifstate(void)
> > struct ifaddrs *ifap, *ifa;
> >  
> > if (getifaddrs() != 0)
> > -   err(1, "getifaddrs");
> > +   fatal("getifaddrs");
> >  
> > for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
> > if (ifa->ifa_addr->sa_family == AF_LINK) {
> >
> 
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: ifstated: consistent use of log.c

[Jeremie Courreges-Anglas]

On Thu, Aug 03 2017, Rob Pierce  wrote:
> As a result ifstated.c no longer needs err.h.
>
> Index: ifstated.c
> ===
> RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
> retrieving revision 1.56
> diff -u -p -r1.56 ifstated.c
> --- ifstated.c24 Jul 2017 12:33:59 -  1.56
> +++ ifstated.c3 Aug 2017 23:59:13 -
> @@ -37,7 +37,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -102,7 +101,7 @@ main(int argc, char *argv[])
>   break;
>   case 'D':
>   if (cmdline_symset(optarg) < 0)
> - errx(1, "could not parse macro definition %s",
> + fatalx("could not parse macro definition %s",
>   optarg);
>   break;
>   case 'f':
> @@ -135,7 +134,7 @@ main(int argc, char *argv[])
>   if (opts & IFSD_OPT_NOACTION) {
>   if ((newconf = parse_config(configfile, opts)) == NULL)
>   exit(1);
> - warnx("configuration OK");
> + fprintf(stderr, "configuration OK\n");

This changes the output from

  ifstated: configuration OK

to

  configuration OK

which is a bit less helpful.  To keep the output the same, you could use
warnx, fprintf + __progname/getprogname instead, or just

errx(0, "configuration OK");

I don't really have a preference here.

>   exit(0);
>   }
>  
> @@ -147,7 +146,7 @@ main(int argc, char *argv[])
>   log_setverbose(opts & IFSD_OPT_VERBOSE);
>  
>   if ((rt_fd = socket(PF_ROUTE, SOCK_RAW, 0)) < 0)
> - err(1, "no routing socket");
> + fatal("no routing socket");
>  
>   rtfilter = ROUTE_FILTER(RTM_IFINFO);
>   if (setsockopt(rt_fd, PF_ROUTE, ROUTE_MSGFILTER,
> @@ -604,7 +603,7 @@ fetch_ifstate(void)
>   struct ifaddrs *ifap, *ifa;
>  
>   if (getifaddrs() != 0)
> - err(1, "getifaddrs");
> + fatal("getifaddrs");
>  
>   for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
>   if (ifa->ifa_addr->sa_family == AF_LINK) {
>


-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE