>>>>> "RS" == Robert Story <[EMAIL PROTECTED]> writes:
WH> Doing it as an assert is wrong.
RS> You didn't really read my response, did you?
Oh, yes I did.
>> But it wasn't an assert, it was a netsnmp_assert. And if you look up the
>> definition of that macro (library/snmp_assert.h), it will only log a message
>> by default. The assert (segfault) will only happen if explicity enabled.
>> There's not even a configure flag to enable it, it has to be manually added
>> or explicity set in CFLAGS.
RS> So, if dave saw an assert, it's because he asked for it.
My point is that I never ever want asserts to segfault for code that
isn't critical. I don't think failing to build a PDU is critical.
Logged? certainly.
Asserts (and yes, I mean netsnmp_asserts) should be limited to "its
going to break anyway" cases. IE, if you continue on you'll be
playing with fire anyway.
RS> I think they are very useful for several reasons, in various
RS> difference cases.
I agree they're useful in some cases. Lazy cases, mostly, but cases
none the less.
IMHO, the right thing to do (and I've said this on the phone to you
before) is define a netsnmp_assert_and_return(error message, return
value) which could be used in 90% of the current cases where returning
from the function would be most of the proper error handling thing to
do anyway. Right now, you're in the worst of both cases. You know a
NULL shouldn't be there (say), but instead of doing the more common
error checking seen in most of our functions:
if (xxx = NULL || yyy == NULL)
return NULL;
You're doing something far worse:
netsnmp_assert(xxx = NULL || yyy == NULL)
And it continues!!! Is it good you noticed the lack of checking? you
bet. But instead of writing attempting to do the minimal thing, it
takes the cheap way out and continues. I'd bet that in most of those
places that asserts have been added you could have returned an error
code or something instead and the code would be much better off, much
more robust and much more production oriented when asserts are only logging.
RS> Silently handling it [an error case] can mask a larger problem.
Agreed.
RS> Logging it (the current default for netsnmp_assert) will at least,
RS> hopefully, get us some bug reports/email messages asking us about
RS> it.
Agreed.
RS> How often do you check your log files to look for error messages
RS> that indicate a bug?
I see asserts all the time on my screen that come from your code. So
many logging them is most likely useless. But I'm happy that it
didn't segfault in the middle when I was attempting to something
completely unrelated and the assert didn't affect it anyway.
RS> And I have full asserts turned on in all my builds, so I'm damn
RS> well gonna notice it. I actually think turning on the assert
RS> behaviour should be the default in developer builds. (Yeah, yeah,
RS> I know, never gonna happen.)
I'd agree if the asserts were always in places where i understood the
code and could fix them. But segfaulting in code unrelated to my
problem means I won't turn them on (think alarms, persistent storage,
etc which is where I think most of your asserts I see are triggered
from).
RS> The other case where I like netsnmp_asserts is anyplace in the
RS> code that I happen to run across that doesn't have error handling,
That seems fine. Adding checks where they don't exist would certainly
be a good thing. Why not return too?
RS> Often this will happen in code that I'm not very familiar with, or
RS> while I'm working on something else and don't have time for a
RS> detailed investigation.
Um, if you're not sure whether the assert should be added then please
please please never add it.
RS> I will readily admit that when writing new code, any form of
RS> assert in lieu of proper error handling is a bad idea. But when
RS> dealing with a big mess of legacy code that you didn't write,
RS> might not totally understand and don't really have time to fully
RS> investigate and fix, they can help determine what problems are
RS> actually happening, and thus need to be fixed.]
Yes, but it also means by committing them that you're pushing the
faults into someone else's time who may not be willing or have the
time to track them down. If that's the case, it's much more likely
they'll turn asserts off instead!
RS> You find me funding to do proper error handling for the current cases of
RS> asserts, and I'll gladly get rid of them all.
I wish I could find funding to have you fix all my mistakes. I think
I've been looking for that offer for ages. Wish I was rich!
--
Wes Hardaker
Sparta, Inc.
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
Net-snmp-coders mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
