This change moves time conversion related code from pf to be available system-wide.
Once the function is available system-wide, it can be used in driver code.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 78493
Build 75376: arc lint + arc unit

Event Timeline

vidwer_fbsdbugs_gmail.com created this revision.Thu, Oct 29, 5:36 PM

Herald added subscribers: vegeta_tuxpowered.net, glebius, melifaro and 3 others. · View Herald TranscriptThu, Oct 29, 5:36 PM

vidwer_fbsdbugs_gmail.com requested review of this revision.Thu, Oct 29, 5:36 PM

Harbormaster completed remote builds in B78493: Diff 190739.Thu, Oct 29, 5:36 PM

vidwer_fbsdbugs_gmail.com added a child revision: D60972: introduce MSEC_TO_NSEC().Thu, Oct 29, 5:36 PM

Other than not removing the pfvar.h include I'm happy with the pf changes, but please try to get someone to okay the addition in sys/time.h too.

sys/netpfil/pf/pf.c
77	I don't think we should remove this one. It defines far more than just SEC_TO_NSEC(). It still builds without it, but that's because if_pfsync.h includes it too.
sys/netpfil/pf/pf_ioctl.c
77	Same here. Don't remove this.

address comments

Harbormaster completed remote builds in B78565: Diff 190966.Fri, Oct 30, 11:25 AM

vidwer_fbsdbugs_gmail.com marked 2 inline comments as done.Fri, Oct 30, 11:29 AM

imp accepted this revision.Fri, Oct 30, 5:29 PM

imp added inline comments.

sys/sys/time.h
427	It's better to include sys/_types.h here, or better yet, just use the _uint64_t types to avoid namespace pollution.

This revision is now accepted and ready to land.Fri, Oct 30, 5:29 PM

IMHO, more review is needed here. This is a system header that is installed in /usr/include. We should check if other common APIs exist or not for this operation. Does it match existing operations that we already define there?

In D60971#1399834, @glebius wrote:

IMHO, more review is needed here. This is a system header that is installed in /usr/include. We should check if other common APIs exist or not for this operation. Does it match existing operations that we already define there?

Not that I, as a novice contributor with a perhaps inexperienced eye, can see: I ran git grep sec | grep UINT64_MAX.

In D60971#1399834, @glebius wrote:

IMHO, more review is needed here. This is a system header that is installed in /usr/include. We should check if other common APIs exist or not for this operation. Does it match existing operations that we already define there?

We don't have an API for this directly. We do for sbintime and I initially thought that could be used instead, but no.
And since it's only exposed in the kernel.