[bugzilla-daemon@xxxxxxxxxxxxx]

http://bugs.wireshark.org/bugzilla/show_bug.cgi?id=1179





------- Comment #26 from frederic.heem@xxxxxxxxx  2006-11-06 09:45 GMT -------
(In reply to comment #25)
> After looking (again) more than another two hours at your remaining patch and
> the "fixes" ...
> 
> Your fixes are ONLY fixes of bugs you've introduced yourself by your own code.

Please tell me the offending file and line  so we'll discuss about them.

> I'm about to reverting them all, as they don't make any sense and will only
> complicate and slow down the loop code (which is really performance relevant -
> to not drop packets while capturing!).
>

Could you tell me where it slows down the code ? Have you done any performance
testing ? Do you really think an extra "if" will hurt the performance ? 

> capture_loop.c - by design - doesn't do any checks but will depend that ALL
> given capture_opts parameters are valid (the caller - dumpcap.c/tshark must
> ensure this). By injecting sniffer_dbus_main() at the wrong place you've simply
Where sniffer_dbus_main() should be ?
Do you think checking in dumpcap.c/tshark/ is faster than checking in
cap_pipe_open_live ?
> produced a lot of mess.
>

> Example of a wrong fix (capture_loop.c line 246), you've added the "if(pipename
> == NULL)" statement:
> 
>   g_log(LOG_DOMAIN_CAPTURE_CHILD, G_LOG_LEVEL_DEBUG, "cap_pipe_open_live: %s",
> pipename);
> 
>   if(pipename == NULL){
>     g_log(LOG_DOMAIN_CAPTURE_CHILD, G_LOG_LEVEL_WARNING, "cap_pipe_open_live:
> pipe name is NULL");
>     g_snprintf(errmsg, errmsgl,
>                "The capture session could not be initiated because the pipe is
> not valid, maybe lack of privileges?");
>     return -1;
>   }
> 
> The mistakes:
> - IF pipename IS really NULL, on Win32 this will crash already in the call to
> g_log() (Win32 don't like %s filled with NULL)
That's a glib bug, please fill a bug report to the glib mailing list.
> - the pipename string is completely untested (caused by your injected code) -
> so the problems may be caused by a variety of reasons
> 
> The real solution:
> Prevent cap_pipe_open_live() from being called with a pipename==NULL.
>

I disagree, checking has to be done at the callee, and optionally at the
caller. The reason is very simple, there are may callers and one callee, check
the callee had to do it once, and only once, checking callers has to be done
every time the function is used.  
The real problem is programmers are lazy and hardly check for NULL pointers,
therefore every function has to check its parameters, never trust the caller
because one day it will provide bad parameters.
> Conclusion: EVERY fix I've looked at is caused by your misunderstanding of the
> code implications, not by any mistakes in the code itself!!!
> 

You miss something here, because EVERY fix I've resolved has already been
committed by Jörg !!!!

> AFAIK: in your d-bus code you've used g_mutex which won't work on Win32 and you

Again, open a glib bug report. 

> use the term sniffer which is trademarked by Network Associates ...
> 
Didn't know, but does that mean it is now forbidden to use "sniffer" in source
code ?
> Thanks for taking a lot of my time by throwing a lot of code at us without just
> asking first how to do it right ...
> 
> Your code therefore is buggy by design and should not be included in the
> dumpcap  sources.
Please do not confuse the bug that I may have introduced, and the
wireshark/glib bugs. 
Where are the bugs you find that are not related to glib ? 
Could you explain me why you treat the code as "buggy by design" ? Be
constructive and propose something better.

> A solution might be to convert capture_loop/dumpcap into a
> library and use it by a separate dumpcap-dbus (or alike) program (which we then
At least, we can agree on something, creating a library is the best solution.

> don't have to maintain here). But that would require a better design discussed
> at the mailing list first.
> 


-- 
Configure bugmail: http://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.