[SSSD] [PATCH] c-ares 1.7.0 related changes

Martin Nagy mnagy at redhat.com
Tue Dec 8 19:26:47 UTC 2009


On Tue, 2009-12-08 at 19:53 +0100, Jakub Hrozek wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 12/08/2009 02:20 PM, Martin Nagy wrote:
> > Unfortunately, that still doesn't solve the issue. These functions are
> > surprisingly difficult to get right. The way the code is now, you don't
> > link the list items. I also just noticed that we don't handle cases were
> > *reply_list == NULL. Also, if we return in the middle of the function,
> > we forget to free the original reply_list. Also,
> > ares_free_data(reply_list); is wrong, because you're passing ares the
> > double pointer, the correct way would be to pass *reply_list (and even
> > better, simply old_list).
> > 
> 
> Thank you for the review, the issues should be fixed now.

Ah, just some tiny bits:
You don't call ares_free_data() if something fails. 

ptr->next = talloc_zero(new_list, struct ares_srv_reply);
if (ptr == NULL) {

You probably wanted to test ptr->next for NULL I would say.
These are applicable to both functions. Fix these and you have an ack.
Otherwise I must say that I like how you wrote them, there is a very
nice trade-off between elegance and simplicity.

> > Seeing as these functions are so much tricky, I propose one of the
> > following to be done:
> > 
> > 1. Fix the issues outlined, create an extensive test similar to
> > test_copy_hostent in resolv-test.c (for at least 2 items in the list).
> > Include a special case in the test calling the function with NULL list,
> > etc. Also don't forget about leak checks. Try running the test with
> > mudflap to make sure we don't access any pointers that we shouldn't.
> > This way, I'd be pretty confident that the code is good.
> 
> We discussed this on IRC with Martin - writing these tests is
> nontrivial, because the a) the rewrite functions are static b) the ares
> parsing functions allocate some private data which are freed in the
> rewrite functions in our resolv code with ares_free_data.
> 
> We could work around this by shadowing ares_free_data with a dummy
> function etc., but since we need these patches to build and rawhide, the
> code seems to be working (for manual testing) and we currently don't use
> the SRV and TXT parsing anywhere, this could be a post-1.0 task.

Yes, I agree, please file a bug about this after the patch is acked and
pushed and (optionally) assign it to me (I have an idea how to do this).

Martin




More information about the sssd-devel mailing list