On Tue, Mar 07, 2017 at 10:17:16AM +0100, Patrick Wildt wrote:
> currently the pf status struct contains the time since pf was enabled as
> seen on the wall clock. This means when time drifts, or is set to some
> earlier value, the time will be off. If we use time since uptime it
> always increments and shows how long pf has been running compared to
> its uptime.
>
> Does this make sense? Opinions?
Kernel interfaces should not return time deltas. Userland cannot
measure the moment when the syscall was made, so there is always
uncertainty about the scheduler delay. Returning time_second is
bad as it changes when wall-clock time is set. Passing values
containing time_uptime is what we want, this is what the diff does.
Switching from time_second to time_uptime based values also produces
less fallout in ports.
OK bluhm@, commit after unlock
> diff --git a/sbin/pfctl/pfctl_parser.c b/sbin/pfctl/pfctl_parser.c
> index e241b11f6fc..3cb321a33e0 100644
> --- a/sbin/pfctl/pfctl_parser.c
> +++ b/sbin/pfctl/pfctl_parser.c
> @@ -520,15 +520,17 @@ void
> print_status(struct pf_status *s, int opts)
> {
> char statline[80], *running, *debug;
> - time_t runtime;
> + time_t runtime = 0;
> + struct timespec uptime;
> int i;
> char buf[PF_MD5_DIGEST_LENGTH * 2 + 1];
> static const char hex[] = "0123456789abcdef";
>
> - runtime = time(NULL) - s->since;
> + if (!clock_gettime(CLOCK_UPTIME, &uptime))
> + runtime = uptime.tv_sec - s->since;
> running = s->running ? "Enabled" : "Disabled";
>
> - if (s->since) {
> + if (runtime) {
> unsigned int sec, min, hrs;
> time_t day = runtime;
>
> diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
> index 56a43a55ab8..fc409a1a7d8 100644
> --- a/sys/net/pf_ioctl.c
> +++ b/sys/net/pf_ioctl.c
> @@ -994,7 +994,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
> struct proc *p)
> error = EEXIST;
> else {
> pf_status.running = 1;
> - pf_status.since = time_second;
> + pf_status.since = time_uptime;
> if (pf_status.stateid == 0) {
> pf_status.stateid = time_second;
> pf_status.stateid = pf_status.stateid << 32;
> @@ -1009,7 +1009,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
> struct proc *p)
> error = ENOENT;
> else {
> pf_status.running = 0;
> - pf_status.since = time_second;
> + pf_status.since = time_uptime;
> pf_remove_queues();
> DPFPRINTF(LOG_NOTICE, "pf: stopped");
> }
> @@ -1605,7 +1605,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
> struct proc *p)
> bzero(pf_status.counters, sizeof(pf_status.counters));
> bzero(pf_status.fcounters, sizeof(pf_status.fcounters));
> bzero(pf_status.scounters, sizeof(pf_status.scounters));
> - pf_status.since = time_second;
> + pf_status.since = time_uptime;
>
> break;
> }
> diff --git a/usr.bin/systat/pf.c b/usr.bin/systat/pf.c
> index 6e282bb7359..4df9ba11b0e 100644
> --- a/usr.bin/systat/pf.c
> +++ b/usr.bin/systat/pf.c
> @@ -220,7 +220,8 @@ void
> print_pf(void)
> {
> char *debug;
> - time_t tm;
> + time_t tm = 0;
> + struct timespec uptime;
> int i;
> struct pf_status *s = &status;
>
> @@ -229,7 +230,8 @@ print_pf(void)
> if (end > num_disp)
> end = num_disp;
>
> - tm = time(NULL) - s->since;
> + if (!clock_gettime(CLOCK_UPTIME, &uptime))
> + tm = uptime.tv_sec - s->since;
>
> ADD_LINE_S("pf", "Status", s->running ? "Enabled" : "Disabled");
> ADD_LINE_A("pf", "Since", tm);
> diff --git a/usr.sbin/snmpd/mib.c b/usr.sbin/snmpd/mib.c
> index f53d9379b07..acd3b751563 100644
> --- a/usr.sbin/snmpd/mib.c
> +++ b/usr.sbin/snmpd/mib.c
> @@ -1650,7 +1650,8 @@ int
> mib_pfinfo(struct oid *oid, struct ber_oid *o, struct ber_element **elm)
> {
> struct pf_status s;
> - time_t runtime;
> + time_t runtime = 0;
> + struct timespec uptime;
> char str[11];
>
> if (pf_get_stats(&s))
> @@ -1661,10 +1662,8 @@ mib_pfinfo(struct oid *oid, struct ber_oid *o, struct
> ber_element **elm)
> *elm = ber_add_integer(*elm, s.running);
> break;
> case 2:
> - if (s.since > 0)
> - runtime = time(NULL) - s.since;
> - else
> - runtime = 0;
> + if (!clock_gettime(CLOCK_UPTIME, &uptime))
> + runtime = uptime.tv_sec - s.since;
> runtime *= 100;
> *elm = ber_add_integer(*elm, runtime);
> ber_set_header(*elm, BER_CLASS_APPLICATION, SNMP_T_TIMETICKS);
