No worries, the 308 patch fixed both issues the way *I* wanted them fixed.

But rather than just committing it I sent it to you for review with the
explanation.  I figured that since there's nothing that p*ss*s me off more
than having somebody hack at something I just fixed rather than proposing a
fix or letting me know the problem so I could fix it myself, I'd do you the
same courtesy.

Thanks!

-----Burton

For the record, let me try and clear up the confusing wording... (I know
*you* know the code, but other's don't)

>> >2) WRT to addressQueue - unless the queued host is really, really busy,
>> >there's a significant risk of resolving the queued host before there's a
>> >host entry to receive the information.  Which wastes effort.
>> >   Also, the (address.c) decrement, myGlobals.addressQueuedCurrent--;
isn't
>> >conditional, nor is the value initialzed to the # of records in the
queue,
>> >so the stats will be wrong.
>> >
>> >
>> I can't follow you here, sorry.


In protocols.c and hash.c, whenever we find a new host, we invoke
ipaddr2str().  In the multi-threaded case, this adds the address to the
queue (stored in a gdbm database).  A separate thread (or threads,
controlled by MAX_NUM_DEQUEUE_THREADS) pulls entries from this queue and
attempts to resolve them - this is the "MAKE_ASYNC_ADDRESS_RESOLUTION"
process - see dequeueAddress() in address.c.

Under normal operation, the size of this queue is pretty small.  It only
gets big if there's a burst of new hosts (say a port scan or a new p2p user,
etc.).  The stats are on info.html in the "Address Resolution" section,
specifically "Queued", e.g.:

Total Queued 46
Not queued (duplicate) 0
Maximum Queued 9
Current Queue 0


If ntop stops, for any reason, and we retain the queue:

1. The myGlobals.addressQueueXXXX stats will be incorrect... that could be
fixed by scanning the queue and counting things.

and

2. It's likely that the host entries will not exist at the time
dequeueAddress() calls resolveAddress().

If you follow the logic, resolveAddress() calls updateHostNameInfo() ->
updateDeviceHostNameInfo(), which does this:

  el = findHostByNumIP(addr, actualDeviceId);

  accessAddrResMutex("updateHostNameInfo");

  if(el != NULL) {
    if(strlen(symbolic) >= (MAX_LEN_SYM_HOST_NAME-1))
      symbolic[MAX_LEN_SYM_HOST_NAME-2] = '\0';
    strcpy(el->hostSymIpAddress, symbolic);
  }

  releaseAddrResMutex();

so the work gets thrown away.  Although the data would be stored in the
cache, there's no certainty it would ever be referenced.  A port scan type
burst isn't likely to be repeated.  So we might save some future DNS lookups
at the cost of doing more DNS lookups at restart.  And a really large queue
would delay lookups for new hosts that were referenced.

Given the effort for #1, and the delay in startup it could cause, and the
limited benefit from #2, it just seemed to me that the small benefit vs.
potential large cost wasn't a good way to go.


_______________________________________________
Ntop-dev mailing list
[EMAIL PROTECTED]
http://listgateway.unipi.it/mailman/listinfo/ntop-dev

Reply via email to