Re: poll magic for pflogd

2016-05-31 Thread Ted Unangst
Tobias Ulmer wrote:
> On Tue, Sep 22, 2015 at 11:46:08AM -0400, Ted Unangst wrote:
> > Todd C. Miller wrote:
> > > On Mon, 21 Sep 2015 20:13:05 -0400, "Ted Unangst" wrote:
> > > 
> > > > We can put a "long" poll() in front of pcap to wait until there are 
> > > > packets
> > > > (maybe never if you aren't using pf logging), and then let the timeout 
> > > > work
> > > > it's magic.
> > > 
> > > This:
> > > 
> > > > +   if (poll(, 1, INFTIM) == 1) {
> > > 
> > > will also match POLLHUP and POLLERR which doesn't appear to be what
> > > you want.  You need to explicitly check for pfd.revents & POLLIN.
> > 
> > poll, you sneaky bastard. I wonder if this program could also benefit from
> > handling signals via kevent, but that's a bigger change.
> > 
> 
> WIP:

Do you have an udpated version of this? I let it drop, but I think we should
try moving ahead.

> 
> Index: pflogd.c
> ===
> RCS file: /home/vcs/cvs/openbsd/src/sbin/pflogd/pflogd.c,v
> retrieving revision 1.51
> diff -u -p -r1.51 pflogd.c
> --- pflogd.c  7 Feb 2015 02:09:13 -   1.51
> +++ pflogd.c  23 Sep 2015 03:47:37 -
> @@ -32,6 +32,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -57,8 +58,6 @@ int Debug = 0;
>  static int snaplen = DEF_SNAPLEN;
>  static int cur_snaplen = DEF_SNAPLEN;
>  
> -volatile sig_atomic_t gotsig_close, gotsig_alrm, gotsig_hup, gotsig_usr1;
> -
>  char *filename = PFLOGD_LOG_FILE;
>  char *interface = PFLOGD_DEFAULT_IF;
>  char *filter = NULL;
> @@ -66,7 +65,6 @@ char *filter = NULL;
>  char errbuf[PCAP_ERRBUF_SIZE];
>  
>  int log_debug = 0;
> -unsigned int delay = FLUSH_DELAY;
>  
>  char *copy_argv(char * const *);
>  void  dump_packet(u_char *, const struct pcap_pkthdr *, const u_char *);
> @@ -81,10 +79,6 @@ int   reset_dump(int);
>  int   scan_dump(FILE *, off_t);
>  int   set_snaplen(int);
>  void  set_suspended(int);
> -void  sig_alrm(int);
> -void  sig_usr1(int);
> -void  sig_close(int);
> -void  sig_hup(int);
>  void  usage(void);
>  
>  static int try_reset_dump(int);
> @@ -95,6 +89,7 @@ static intbuflen = 0;   /* allocated s
>  static char  *buffer = NULL; /* packet buffer */
>  static char  *bufpos = NULL; /* position in buffer */
>  static intbufleft = 0;   /* bytes left in buffer */
> +static intbuftimer = 0; /* periodic buffer flush on/off */
>  
>  /* if error, stop logging but count dropped packets */
>  static int suspended = -1;
> @@ -162,30 +157,6 @@ usage(void)
>  }
>  
>  void
> -sig_close(int sig)
> -{
> - gotsig_close = 1;
> -}
> -
> -void
> -sig_hup(int sig)
> -{
> - gotsig_hup = 1;
> -}
> -
> -void
> -sig_alrm(int sig)
> -{
> - gotsig_alrm = 1;
> -}
> -
> -void
> -sig_usr1(int sig)
> -{
> - gotsig_usr1 = 1;
> -}
> -
> -void
>  set_pcap_filter(void)
>  {
>   struct bpf_program bprog;
> @@ -224,7 +195,7 @@ if_exists(char *ifname)
>  int
>  init_pcap(void)
>  {
> - hpcap = pcap_open_live(interface, snaplen, 1, PCAP_TO_MS, errbuf);
> + hpcap = pcap_open_live(interface, snaplen, 1, 0, errbuf);
>   if (hpcap == NULL) {
>   logmsg(LOG_ERR, "Failed to initialize: %s", errbuf);
>   return (-1);
> @@ -439,6 +410,7 @@ void
>  dump_packet_nobuf(u_char *user, const struct pcap_pkthdr *h, const u_char 
> *sp)
>  {
>   FILE *f = (FILE *)user;
> + fprintf(stderr, "%s called\n", __func__);
>  
>   if (suspended) {
>   packets_dropped++;
> @@ -453,7 +425,7 @@ dump_packet_nobuf(u_char *user, const st
>   ftruncate(fileno(f), pos - sizeof(*h))) {
>   logmsg(LOG_ERR, "Write failed, corrupted logfile!");
>   set_suspended(1);
> - gotsig_close = 1;
> + raise(SIGTERM);
>   return;
>   }
>   goto error;
> @@ -499,6 +471,7 @@ flush_buffer(FILE *f)
>   bufpos = buffer;
>   bufleft = buflen;
>   bufpkt = 0;
> + buftimer = 0;
>  
>   return (0);
>  }
> @@ -512,6 +485,7 @@ purge_buffer(void)
>   bufpos = buffer;
>   bufleft = buflen;
>   bufpkt = 0;
> + buftimer = 0;
>  }
>  
>  /* append packet to the buffer, flushing if necessary */
> @@ -521,6 +495,8 @@ dump_packet(u_char *user, const struct p
>   FILE *f = (FILE *)user;
>   size_t len = sizeof(*h) + h->caplen;
>  
> + fprintf(stderr, "%s called\n", __func__);
> +
>   if (len < sizeof(*h) || h->caplen > (size_t)cur_snaplen) {
>   logmsg(LOG_NOTICE, "invalid size %zu (%d/%d), packet dropped",
>  len, cur_snaplen, snaplen);
> @@ -553,6 +529,7 @@ dump_packet(u_char *user, const struct p
>   bufpos += len;
>   bufleft -= len;
>   bufpkt++;
> + buftimer = 1;
>  
>   return;
>  }
> @@ -575,6 +552,9 @@ main(int argc, char **argv)
>   int ch, np, ret, Xflag = 0;
>   pcap_handler phandler = dump_packet;
>   

Re: poll magic for pflogd

2015-09-22 Thread Todd C. Miller
On Mon, 21 Sep 2015 20:13:05 -0400, "Ted Unangst" wrote:

> We can put a "long" poll() in front of pcap to wait until there are packets
> (maybe never if you aren't using pf logging), and then let the timeout work
> it's magic.

This:

> + if (poll(, 1, INFTIM) == 1) {

will also match POLLHUP and POLLERR which doesn't appear to be what
you want.  You need to explicitly check for pfd.revents & POLLIN.

 - todd



Re: poll magic for pflogd

2015-09-22 Thread Ted Unangst
Todd C. Miller wrote:
> On Mon, 21 Sep 2015 20:13:05 -0400, "Ted Unangst" wrote:
> 
> > We can put a "long" poll() in front of pcap to wait until there are packets
> > (maybe never if you aren't using pf logging), and then let the timeout work
> > it's magic.
> 
> This:
> 
> > +   if (poll(, 1, INFTIM) == 1) {
> 
> will also match POLLHUP and POLLERR which doesn't appear to be what
> you want.  You need to explicitly check for pfd.revents & POLLIN.

poll, you sneaky bastard. I wonder if this program could also benefit from
handling signals via kevent, but that's a bigger change.



poll magic for pflogd

2015-09-21 Thread Ted Unangst
I think this will help pflogd sleep better at night...

My understanding of the pcap/bpf timeout is that it will always wait that
long, even if packets are received, so that you don't get one read() per
packet. But using this timeout doesn't mean wait forever until you get
something.

We can put a "long" poll() in front of pcap to wait until there are packets
(maybe never if you aren't using pf logging), and then let the timeout work
it's magic.


Index: pflogd.c
===
RCS file: /cvs/src/sbin/pflogd/pflogd.c,v
retrieving revision 1.51
diff -u -p -r1.51 pflogd.c
--- pflogd.c7 Feb 2015 02:09:13 -   1.51
+++ pflogd.c22 Sep 2015 00:08:16 -
@@ -35,6 +35,8 @@
 #include 
 #include 
 #include 
+
+#include 
 #include 
 #include 
 #include 
@@ -575,6 +577,7 @@ main(int argc, char **argv)
int ch, np, ret, Xflag = 0;
pcap_handler phandler = dump_packet;
const char *errstr = NULL;
+   struct pollfd pfd;
 
ret = 0;
 
@@ -685,9 +688,16 @@ main(int argc, char **argv)
} else if (Xflag)
return (0);
 
+   pfd.fd = pcap_get_selectable_fd(hpcap);
+   pfd.events = POLLIN;
while (1) {
-   np = pcap_dispatch(hpcap, PCAP_NUM_PKTS,
-   phandler, (u_char *)dpcap);
+   np = 0;
+   if (poll(, 1, INFTIM) == 1) {
+   np = pcap_dispatch(hpcap, PCAP_NUM_PKTS,
+   phandler, (u_char *)dpcap);
+   } else if (pfd.revents & (POLLERR|POLLHUP)) {
+   np = -1;
+   }
if (np < 0) {
if (!if_exists(interface)) {
logmsg(LOG_NOTICE, "interface %s went away",