Nsock IOCP engine review

[Henri Doreau <henri.doreau () gmail com>]

Hello,

I just reviewed the new nsock IOCP engine. I unfortunately could not
test it but I have no doubt that others will seriously stress it.
Congratulations for the good work. I have a couple concerns though.
Two of which are minor, one's slightly more annoying.

Starting with the small ones:

* there seems to be engine-specific data embedded directly into the
nsp. That doesn't make sense to me... Why not having them in the
engine_data which you use anyway?

* in nbase_misc.c: is it ok to unconditionally specify the
WSA_FLAG_OVERLAPPED? What if another engine is selected?


Now, for something bigger:
* the following is everywhere, and is an anti-pattern, something that
the nsock engines are supposed to guard us against:
"""
#ifdef HAVE_IOCP
if (engine_is_iocp(nsp))
    iocp_specific_call()
#endif
"""

This makes the code difficult to review/understand/test/maintain. The
function was even checked without being called at some point (removed
as of r35969). I regard this as a proof that such a pattern is very
error-prone.

Can you consider moving these blocks to engine-specific functions?
Maybe they would fit somewhere in the existing functions, or maybe the
interface has to evolve. If so, find the appropriate abstractions, add
corresponding calls to the engine operations and implement them as
such (they can be left empty for the other engines if irrelevant, no
problem with that of course)...

I would have nacked the change if asked. Now I'm fine with not
reverting r36093 if you can address the issues above. Otherwise I
consider that it introduces a significant technical debt, despite how
nice the feature is! It's good work, but not ready to land IMHO.

Tudor, Fyodor, Dan? What do you think?

-- 
Henri
_______________________________________________
Sent through the dev mailing list
https://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/