lightweight packet sniffer review

This is a simple packet sniffer that turns on LEDs when there's network activity. It works by picking up filters in a configuration file and listen for activity in those filters. When there's a captured packet, it sets a bit in a bit-mask that corresponds to the filter index in the config file. Then, once all the filters have been polled, it sends the bit-mask to an LED device ( in this case, an Arduino ) and the lights turn on.

I'm just looking for suggestions or any sort of feedback on the code. All comments are welcome.

```
#include "main.h"

int close_signal = 0;

int main( int argc, char * argv[] ){

struct filter_node * head = 0;
FILE * led_device = 0;
FILE * config_file = 0;
char * line_buffer = 0;
char errbuf[ PCAP_ERRBUF_SIZE ];
char * dev = 0;
bpf_u_int32 maskp;
bpf_u_int32 netp;

unsigned int led_mask = 0;

printf( "Starting %s..\n", argv[ 0 ] );

/ Daemonize / {
pid_t p_id = fork( ); // start in new process
if( p_id 0 ) return 0;

if( setsid( ) cap_handle = pcap_open_live( 0, BUFSIZ, 1, -1, errbuf );
if( !temp->cap_handle ){
printf( "pcap_open_live(): %s\n" , errbuf );
free( temp );
continue;
}

// compile filter
if( pcap_compile( temp->cap_handle, &temp->fp, line_buffer, 0, netp ) cap_handle );
free( temp );
continue;
}

// assign filter to handle
if( pcap_setfilter( temp->cap_handle, &temp->fp ) cap_handle );
pcap_freecode( &temp->fp );
free( temp );
continue;
}

// set to non blocking calls
// this is so that each filter can be checked
// in the same instance of time
if( pcap_setnonblock( temp->cap_handle, 1, errbuf ) cap_handle );
pcap_freecode( &temp->fp );
free( temp );
continue;
}

temp->index = i; // assign the index of the

Your code looks nice and compiles cleanly (once I filled in the blanks). I
have a few comments, the main one being your lack of functions. Putting
everything in main is not practicable beyond a certain size and you have
exceeded that size. Various functions should be extracted from main,
including at least the daemonize code, creating each filter node and the main
loop. So main might look something like (simplified):

daemonize();
    head = configure_filter_chain();
    if (head) {
        int led = open_led();
        while (!close_signal) {
            filter(head, led);
        }
    }

Some detailed comments:

-
use perror to print errors where errno has been set. This prints to
stderr, not stdout, so you will need to open a descriptor for that.

-
do you need to open descriptors for unused stdin/stderr (or stdin/stdout -
see previous point)? Are they used? If not why not just fclose(stdin)?

-
it is better to open resources only when you need them. For example you
opened a file on the LED device long before it is needed.

-
why use buffered i/o on the LED? Wouldn't unbuffered (ie open/write) be
more suitable than buffered (fopen/fwrite/fflush)?

-
the content of your big for-loop has a few issues. For a start, it should
probably be a function. Also, you allocate memory before it is needed, and
hence have to free it at several points. And your error recovery (closing
resources obtained) is duplicated. Something like the following would be
neater, where create_node allocates a new node and adds it to the linked
list:

static void create_filter_node(const char *line, bpf_u_int32 netp, struct filter_node **head)
{
    char errbuf[PCAP_ERRBUF_SIZE];
    struct bpf_program fp;
    int compiled = -1;

    pcap_t *handle = pcap_open_live(0, BUFSIZ, 1, -1, errbuf);
    if (!handle){
        fprintf(stderr, "..." , errbuf);
    }
    else if ((compiled = pcap_compile(handle, &fp, line, 0, netp)) < 0){
        fprintf(stderr, "..." , line);
    }
    else if (pcap_setfilter(handle, &fp) < 0){
        fprintf(stderr, "..." , line);
    }
    else if (pcap_setnonblock(handle, 1, errbuf) < 0){
        fprintf(stderr, "..." , errbuf);
    }
    else if (create_node(handle, fp, i, head)) {
        return;  // ie. Success!
    }

    if (handle) {
        pcap_close(handle);
    }
    if (compiled == 0) {
        pcap_freecode(&fp);
    }
}

Some people use goto statements to handle this sort of clean-up, instead
of the if-else-if chain. In this case that seems unnecessary.

And some pedantic comments

-
Adding spaces inside brackets is unusual. It is more normal to write if (condition)
rather than if( condition )

-
dev is unused

-
on failure it is normal to return EXIT_FAILURE (from stdlib) not -1

-
define loop variables in the for-loop if possible, so for (int i=0; ...)

StackExchange Code Review Q#25600, answer score: 2