[Nagiosplug-devel] check_dns.c patch review request

[Thomas Guyot-Sionnest]

-----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.
> 
> 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).
> 
> Thanks
> 

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[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-----