[getdns-api] getdns_return_t for destroy methods

[Goyal, Neel]

On 3/10/14, 4:15 PM, "Paul Hoffman" <paul.hoffman at vpnc.org> wrote:

>On Mar 10, 2014, at 5:15 PM, Goyal, Neel <ngoyal at verisign.com> wrote:
>
>> 
>>>> 
>>>> The implementation can not.  But, the application using the
>>>> implementation/library *can* do it from within a callback, because the
>>>> callbacks are part of that application.
>>>> 
>>>> What we propose, is to let a getdns_context_destroy, when used from
>>>> within a callback, return GETDNS_RETURN_GOOD if the
>>>> library/implementation can deal with it, and
>>>>GETDNS_RETURN_GENERIC_ERROR
>>>> if it can not.
>>>> 
>>>> Our implementation currently deals with it, but it is administratively
>>>> cumbersome (implementation wise) and therefore susceptible for bugs in
>>>> the future.  For robustness we would rather disallow it.
>>> 
>>> OK, that makes more sense. How about this instead:
>>> 
>>> getdns_return_t getdns_context_destroy(struct getdns_context* context);
>>> 
>>> Returns destroys the context and GETDNS_RETURN_GOOD if the context
>>> exists.  If the context does not exist (for example, because it was
>>> previously destroyed), the function returns
>>>GETDNS_RETURN_GENERIC_ERROR.
>> 
>>
>> If context is already destroyed and not NULL, the code
>> will crash.  
>
>So you do not check whether or not the "context" argument is an argument
>you know about?
>
>> This is analogous to the double free problem that exists in C.
>
>Sure, but I thought that was why we had these constructors and
>destructors, but maybe not.

Hopefully an example can clarify what I mean.  Get your C hat ready.
Please consider the following:

getdns_context* context = NULL;
getdns_context_create(&context, 1);
// the context variable now points to some location in memory that contains
// our internal context data
getdns_context_destroy(context);
// the memory that context points to has been freed up and cleared.
// however, the context variable still points to that memory location and
is
// not NULL (context != NULL)

So calling getdns_context_destroy(context); again will fault because the
variable passed in is not NULL, and the code considers it valid.  There’s
no way around this unless we take in a context** and set *context to NULL
in our destructor.  I don’t recommend that since C programmers will be
used to the behavior in the current API.

>
>> We are trying to disallow destroying a valid context within an API
>>user¹s
>> asynchronous callback.  How about the last sentence becomes "If the
>> context cannot be destroyed (for example, within the body of a
>> getdns_callback_t), the function returns GETDNS_RETURN_GENERIC_ERROR.".
>
>Wouldn't a context that could not be destroyed and not NULL also be "a
>context that cannot be destroyed"?

Yes - did I miss something?  I wanted to clarify the “context does not
exist (for example, because it was previously destroyed)” portion.