Hi, team!
I've submitted numerous patches to the net-snmp project for errors so
esoteric that it took days to find and understand them.
In my painful searches through the commentless code, I built up an
understanding of the necessary components and added in many comments
to help steer people in the right direction, save them hours of time,
and help everyone extend the code.
I recently realized that a patch that I submitted a year or two ago
had been stripped of all of its comments (and thus a good deal of its
usefulness). Granted, my fix (to prevent infinite looping) was kept,
but all of the knowledge and reasoning behind that fix was stripped.
Here's a piece the original patch:
@@ -590,10 +591,24 @@
{
netsnmp_pdu *newpdu;
- if ((pdu->command != SNMP_MSG_RESPONSE)
+ if (
+ // We're only going to fix a PDU if it was a *response* to us.
+ (pdu->command != SNMP_MSG_RESPONSE)
+ // We're only going to fix a PDU that had an error.
|| (pdu->errstat == SNMP_ERR_NOERROR)
+ // We're only going to fix a PDU that has varbinds.
|| (0 == pdu->variables)
- || (pdu->errindex <= 0)) {
+ // We're only going to fix a PDU with a positive error-index.
+ || (pdu->errindex <= 0)
+ // We're only going to fix a PDU if we can actually remove
+ // the varbind that was listed as being "bad". If the number
+ // of varbinds is *less* than the error-index value, then
+ // we can't really do anything except simply resend the PDU
+ // and hope for a better response, and that's not a good
+ // thing to do; previously, this had led to infinite loops
+ // of send, receive, and resend.
+ || (pdu->errindex > snmp_varbind_len(pdu))
+ ) {
return 0; /* pre-condition tests fail */
}
You'll note that most of the "+" signs are comments. To someone who's
trying to find the cause of a problem, these are gold; to that person,
these variables and conditions have no meaning. If you haven't worked
in the code for long periods of time, a lot of the "obvious" things
that snmp-gurus might know are completely new and foreign to the rest
of us.
Maybe someone was against the C++-style comments ("//" comments);
however, C99 has included them in the C language (see section 6.4.9,
"Comments", item #2).
In any case, I've seen this kind of behavior in many open-source
projects, and it only hurts the project. If we cannot justify (to our
readers, to our developers, to our helpers in the open-source
community), then we're back to square one of trying to control the
knowledge about the code.
Unless there is some huge issue of which I am unaware, I strongly
suggest that everyone fully comment his code; justify its existence,
and make note of its shortcomings. In the end, we all benefit.
Thank you for your time,
Doug
------------------------------------------------------------------------------
_______________________________________________
Net-snmp-coders mailing list
Net-snmp-coders@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
