[Nagiosplug-devel] check_dns.c patch review request
dermoth at aei.ca
Fri Jul 10 05:55:34 CEST 2009
-----BEGIN PGP SIGNED MESSAGE-----
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:
> 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.
> 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).
With more in-depth look...
> + /* Strip leading spaces */
> + for (; *temp_buffer != '\0' && *temp_buffer == ' '; temp_buffer++)
> + /* NOOP */;
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); 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.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
-----END PGP SIGNATURE-----
More information about the Devel