[PATCHES] some new tevent features

[Stefan (metze) Metzmacher]

Hi Volker,

thanks for the review! I added comments inline.

>> >From 0a2bce1393e4c8e65b8f69c051c9a236319584b0 Mon Sep 17 00:00:00 2001
>> From: Stefan Metzmacher <metze at samba.org>
>> Date: Fri, 27 Sep 2013 03:41:29 +0200
>> Subject: [PATCH 04/14] tevent: add/use tevent_req_destructor
>>
>> This makes sure we call tevent_req_received(req) on talloc_free()
>> and cleanup things in a defined order.
> 
> Are we 100% sure that no tevent_req user already does this?

I don't understand that question.

In most cases the _recv() function should already call
tevent_req_received().

But in some cases the _recv function doesn't or a request is
abandoned by an implicit or explicit talloc_free(req).

Calling tevent_req_received() in the destructor (only if
tevent_req_received()
was not called before) means we free the children of 'req' in a defined
order,
req->internal.trigger and req->internal.timer before req->data.

This also means that we have req->internal.state = TEVENT_REQ_RECEIVED;
while a possible destructor of req->data runs.

The main reason for this change is a patch that will follow later
and introduces a cleanup callback that will be triggered by
tevent_req_received(). This can be used to mark a connection as
broken, if a caller does talloc_free(req) of a still pending request,
which might only be partially send to a socket.

Is this patch ok for you with this description?

>> Signed-off-by: Stefan Metzmacher <metze at samba.org>
>> ---
>>  lib/tevent/tevent_req.c |   15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/tevent/tevent_req.c b/lib/tevent/tevent_req.c
>> index 581b818..b3db1e7 100644
>> --- a/lib/tevent/tevent_req.c
>> +++ b/lib/tevent/tevent_req.c
>> @@ -51,6 +51,8 @@ char *tevent_req_print(TALLOC_CTX *mem_ctx, struct tevent_req *req)
>>  	return req->private_print(req, mem_ctx);
>>  }
>>  
>> +static int tevent_req_destructor(struct tevent_req *req);
>> +
>>  struct tevent_req *_tevent_req_create(TALLOC_CTX *mem_ctx,
>>  				    void *pdata,
>>  				    size_t data_size,
>> @@ -86,10 +88,18 @@ struct tevent_req *_tevent_req_create(TALLOC_CTX *mem_ctx,
>>  
>>  	req->data = data;
>>  
>> +	talloc_set_destructor(req, tevent_req_destructor);
>> +
>>  	*ppdata = data;
>>  	return req;
>>  }
>>  
>> +static int tevent_req_destructor(struct tevent_req *req)
>> +{
>> +	tevent_req_received(req);
>> +	return 0;
>> +}
>> +
>>  void _tevent_req_notify_callback(struct tevent_req *req, const char *location)
>>  {
>>  	req->internal.finish_location = location;
>> @@ -199,7 +209,8 @@ bool tevent_req_is_in_progress(struct tevent_req *req)
>>  
>>  void tevent_req_received(struct tevent_req *req)
>>  {
>> -	TALLOC_FREE(req->data);
>> +	talloc_set_destructor(req, NULL);
>> +
>>  	req->private_print = NULL;
>>  	req->private_cancel = NULL;
>>  
>> @@ -207,6 +218,8 @@ void tevent_req_received(struct tevent_req *req)
>>  	TALLOC_FREE(req->internal.timer);
>>  
>>  	req->internal.state = TEVENT_REQ_RECEIVED;
>> +
>> +	TALLOC_FREE(req->data);
>>  }
>>  
>>  bool tevent_req_poll(struct tevent_req *req,
>> -- 
>> 1.7.9.5
>>
>>

>> >From fa5a9325fe4308e68c16eeb10b44e9ba802cfd5e Mon Sep 17 00:00:00 2001
>> From: Stefan Metzmacher <metze at samba.org>
>> Date: Thu, 5 Dec 2013 08:47:27 +0100
>> Subject: [PATCH 07/14] tevent: make use of talloc_get_type_abort() in
>>  tevent_queue.c
> 
> In most code I write, I use different formatting that uses
> one less line. Just a comment, not blocking the

I often use this in order to use always use the same pattern, while not
hitting
the 80 chars per line limit.

This pattern also works for things like this:

        struct dcerpc_binding_handle_raw_call_state *state =
                tevent_req_data(req,
                struct dcerpc_binding_handle_raw_call_state);

>> >From 7c41b464bf70bb6f30e2f78a1587c1e6ef28ca52 Mon Sep 17 00:00:00 2001
>> From: Stefan Metzmacher <metze at samba.org>
>> Date: Thu, 5 Dec 2013 08:47:27 +0100
>> Subject: [PATCH 11/14] tevent: make use of talloc_get_type_abort() in
>>  tevent_epoll.c
>>
> 
> This is a slight change in behaviour if anything is NULL.
> Now we abort and don't just return false. Is that intended?

Yes, I looked at the existing caller and to me it can't happen.

I'll change the function to use 'void' instead of 'bool' as return
value, ok?

>> Signed-off-by: Stefan Metzmacher <metze at samba.org>
>> ---
>>  lib/tevent/tevent_epoll.c |   26 +++++++++++---------------
>>  1 file changed, 11 insertions(+), 15 deletions(-)
>>
>> diff --git a/lib/tevent/tevent_epoll.c b/lib/tevent/tevent_epoll.c
>> index 599c190..951fa42 100644
>> --- a/lib/tevent/tevent_epoll.c
>> +++ b/lib/tevent/tevent_epoll.c
>> @@ -120,17 +120,10 @@ _PRIVATE_ bool tevent_epoll_set_panic_fallback(struct tevent_context *ev,
>>  				bool (*panic_fallback)(struct tevent_context *ev,
>>  						       bool replay))
>>  {
>> -	struct epoll_event_context *epoll_ev;
>> -
>> -	if (ev->additional_data == NULL) {
>> -		return false;
>> -	}
>> +	struct epoll_event_context *epoll_ev =
>> +		talloc_get_type_abort(ev->additional_data,
>> +		struct epoll_event_context);
>>  
>> -	epoll_ev = talloc_get_type(ev->additional_data,
>> -				struct epoll_event_context);
>> -	if (epoll_ev == NULL) {
>> -		return false;
>> -	}
>>  	epoll_ev->panic_fallback = panic_fallback;
>>  	return true;
>>  }

metze