One OR MORE of source and destination addresses?

[Bill Moran]

"Ralph Hempel" <rhempel at bmts.com> wrote:
> 
> > I just took a look at the code:
> > 
> >  if (q != NULL) { /* should never occur */
> >   if (last_log != time_second) {
> >    last_log = time_second;
> >    printf("ipfw: install_state: entry already present, done\n");
> >   }
> >   return 0;
> >  }
> > 
> > What if I just hack the "printf ..." line out of there? Would that 'solve'
> > it? I know it's dirty; but would things still work?
> 
> I'll jump in here as a software manager and say NO!!!!!
> 
> Note, I have no idea if it will still work, but as a professional
> programmer, the question raises a number of issues :-)
> 
> 1. First of all, the original programmer took time to comment
>    this line:
> 
>     if (q != NULL) { /* should never occur */
> 
>    OK. There's no indication WHY it should never occur, but still, the comment
>    is there.
> 
> 2. By adding this line: 
> 
>     if (last_log != time_second) {
> 
>    He's limiting the printed errors to one every second, so you
>    are not beeing flooded with as many messages as are actually
>    ocurring.
> 
>    Is last_log used anywhere else?
> 
> 3. This line:
> 
>      return 0;
> 
>    will still return 0 if the error occurs, so the program will
>    work the same with or without the diagnostic message.
> 
> I'd do some more digging and find out exactly WHY this is a "should never
> occur case" to be sure that the log is not needed. If you don't print
> the log, then why do the test, except to return 0 :-)

I was thinking about this over lunch, then I saw your post ... and the
reality is that someone should really file a PR.

Mark, since you have a real-world application where this problem occurs,
it would be idea if you could file a PR with your description of what
you're trying to do and the problem it's causing.

As best I can tell, the problem is _not_ in install_state ... only the
symptom is in install_state.  The problem is that code that is calling
install_state is calling it twice for some reason.  Taking that into
consideration, there's a possibility that this is fixed in -CURRENT,
but I haven't found any commit entries to that tune.

-- 
Bill Moran
Potential Technologies
http://www.potentialtech.com