[Nagiosplug-devel] check_dns.c patch review request

[David Horn]

On Thu, Jul 9, 2009 at 11:55 PM, Thomas Guyot-Sionnest<dermoth at aei.ca> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 09/07/09 11:07 PM, Thomas Guyot-Sionnest wrote:
>> On 08/07/09 05:25 PM, David Horn wrote:
>>> This is just a request to please review and/or commit the changes to
>>> check_dns to support IPv6 AAAA record types (and others).
>>
>>> Let me know if there are any test cases you want me to write, or any
>>> additional information or changes needed to accept this patch.
>>
>>> Patch is attached to this tracker entry:
>>> https://sourceforge.net/tracker/?func=detail&aid=2741269&group_id=29880&atid=397599
>>
>> It looks good. I just have one question - on what basis did you increase
>> the ADDRESS_LENGTH macro? It would be helpful to know on what it's based
>> or how it was calculated.

I originally increased the ADDRESS_LENGTH macro (incorrectly) thinking
that it was needed for IPv6, but after you asked the question, I did
some research and found some actual answers:

1) String representations for IPv6 (.ip6.arpa reverse notation or
standard presentation notation) do NOT need an increase above the
original 256 bytes.
2) Standard "on the wire" DNS ascii notation of domain names result in
a maximum length of 255 characters, hence the original setting. (RFC
2181/RFC 1034)
3) With IDN (International Domain Names), the presentation string
(displayed to the user) is allocated in bind9 nslookup source as being
1024 bytes since certain country specific domains support multibyte.
Since check_dns calls nslookup, the appropriate thing is probably to
change this macro to 1024 to match.  I will see if I can find some
multibyte test cases.

lib/dns/include/dns/name.h:#define DNS_NAME_MAXTEXT 1023
lib/dns/include/dns/name.h:#define DNS_NAME_FORMATSIZE (DNS_NAME_MAXTEXT + 1)

>>
>> Also, for inclusion we'll need to add tests too (plugins/t/check_dns.t).
>> If you can write them that would be awesome, otherwise it would really
>> help if you could at least give us a list of commands and what to expect
>> from them (return code and/or text match).
>>

I will work on writing the tests needed.

>> Thanks
>>
>
> With more in-depth look...
>
>> +      /* Strip leading spaces */
>> +      for (; *temp_buffer != '\0' && *temp_buffer == ' '; temp_buffer++)
>> +        /* NOOP */;
>

1) I do not mind cleaning up the string parsing a bit (I originally
just moved existing code into a function to minimize changes).
2) There is also one "bug" I also did not fix (to preserve backwards
compatability), that I can look into here as well. (When a dns query
results in a response, but not an answer to the original question)

> Testing for *temp_buffer == ' ' is sufficient as \0 will evaluate as
> false. Even better is using isspace() to remove tabs as well. I would
> write it as:
>
>    /* Strip leading spaces */
>    for (temp_buffer; isspace(temp_buffer[0]); temp_buffer++);
>
> The strip() function could be fixed to use isspace too... or just copy
> the code for np_extract_value in lib/utils_base.c:
>
>    for (i=strlen(temp_buffer)-1; isspace(temp_buffer[i]); i--)
>        temp_buffer[i] = '\0';
>
> Finally, there's no reason to test if temp_buffer is NULL afterward. It
> can't be if it wasn't previously (at worse it will still be a pointer
> pointing to a '\0') and if it is, you crashed already.
>
>
> Note that in the leading strip loop, my code checks if temp_buffer is
> not null... You could add an if statement before the for loop too -
> here's my code in lib/utils_base.c:
>
>>   if (value) for (i=strlen(value)-1; isspace(value[i]); i--) value[i] = '\0';
>
> And then the NULL check could actually make it work if the buffer is
> passed as a NULL.
>
> - --
> Thomas
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.6 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iD4DBQFKVru26dZ+Kt5BchYRAoe/AJ9kn5Ehahb9Xd0MjLQxUqua1sqMZwCXdcbg
> kyuH8pFZGTQLLfGW6SdSoA==
> =9tgx
> -----END PGP SIGNATURE-----
>
> ------------------------------------------------------------------------------
> Enter the BlackBerry Developer Challenge
> This is your chance to win up to $100,000 in prizes! For a limited time,
> vendors submitting new applications to BlackBerry App World(TM) will have
> the opportunity to enter the BlackBerry Developer Challenge. See full prize
> details at: http://p.sf.net/sfu/Challenge
> _______________________________________________________
> Nagios Plugin Development Mailing List Nagiosplug-devel at lists.sourceforge.net
> Unsubscribe at https://lists.sourceforge.net/lists/listinfo/nagiosplug-devel
> ::: Please include plugins version (-v) and OS when reporting any issue.
> ::: Messages without supporting info will risk being sent to /dev/null
>