Re: [RFC 1/2] shared/mgmt: Add request timeout handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Luiz,

> This adds request timeout handling, by default the timeout is set to 2
> seconds but in case the user want a different timeout it can use
> mgmt_send_timeout to set a different value.
> ---
> src/shared/mgmt.c | 70 +++++++++++++++++++++++++++++++++++++++++------
> src/shared/mgmt.h |  5 ++++
> 2 files changed, 67 insertions(+), 8 deletions(-)
> 
> diff --git a/src/shared/mgmt.c b/src/shared/mgmt.c
> index 41457b430..291a2c636 100644
> --- a/src/shared/mgmt.c
> +++ b/src/shared/mgmt.c
> @@ -25,6 +25,9 @@
> #include "src/shared/queue.h"
> #include "src/shared/util.h"
> #include "src/shared/mgmt.h"
> +#include "src/shared/timeout.h"
> +
> +#define MGMT_TIMEOUT 2
> 
> struct mgmt {
> 	int ref_count;
> @@ -49,6 +52,7 @@ struct mgmt {
> };
> 
> struct mgmt_request {
> +	struct mgmt *mgmt;
> 	unsigned int id;
> 	uint16_t opcode;
> 	uint16_t index;
> @@ -57,6 +61,8 @@ struct mgmt_request {
> 	mgmt_request_func_t callback;
> 	mgmt_destroy_func_t destroy;
> 	void *user_data;
> +	int timeout;
> +	unsigned int timeout_id;
> };
> 
> struct mgmt_notify {
> @@ -81,6 +87,9 @@ static void destroy_request(void *data)
> 	if (request->destroy)
> 		request->destroy(request->user_data);
> 
> +	if (request->timeout_id)
> +		timeout_remove(request->timeout_id);
> +
> 	free(request->buf);
> 	free(request);
> }
> @@ -150,6 +159,26 @@ static void write_watch_destroy(void *user_data)
> 	mgmt->writer_active = false;
> }
> 
> +static bool request_timeout(void *data)
> +{
> +	struct mgmt_request *request = data;
> +
> +	if (!request)
> +		return false;
> +
> +	request->timeout_id = 0;
> +
> +	queue_remove_if(request->mgmt->pending_list, NULL, request);
> +
> +	if (request->callback)
> +		request->callback(MGMT_STATUS_TIMEOUT, 0, NULL,
> +						request->user_data);
> +
> +	destroy_request(request);
> +
> +	return false;
> +}
> +
> static bool send_request(struct mgmt *mgmt, struct mgmt_request *request)
> {
> 	struct iovec iov;
> @@ -169,6 +198,12 @@ static bool send_request(struct mgmt *mgmt, struct mgmt_request *request)
> 		return false;
> 	}
> 
> +	if (request->timeout)
> +		request->timeout_id = timeout_add_seconds(request->timeout,
> +							request_timeout,
> +							request,
> +							NULL);
> +
> 	util_debug(mgmt->debug_callback, mgmt->debug_data,
> 				"[0x%04x] command 0x%04x",
> 				request->index, request->opcode);
> @@ -566,7 +601,8 @@ bool mgmt_set_close_on_unref(struct mgmt *mgmt, bool do_close)
> static struct mgmt_request *create_request(struct mgmt *mgmt, uint16_t opcode,
> 				uint16_t index, uint16_t length,
> 				const void *param, mgmt_request_func_t callback,
> -				void *user_data, mgmt_destroy_func_t destroy)
> +				void *user_data, mgmt_destroy_func_t destroy,
> +				int timeout)
> {
> 	struct mgmt_request *request;
> 	struct mgmt_hdr *hdr;
> @@ -598,12 +634,18 @@ static struct mgmt_request *create_request(struct mgmt *mgmt, uint16_t opcode,
> 	hdr->index = htobs(index);
> 	hdr->len = htobs(length);
> 
> +	/* Use a weak reference so requests don't prevent mgmt_unref to
> +	 * cancel requests and free in case of the last reference is dropped by
> +	 * the user.
> +	 */
> +	request->mgmt = mgmt;
> 	request->opcode = opcode;
> 	request->index = index;
> 
> 	request->callback = callback;
> 	request->destroy = destroy;
> 	request->user_data = user_data;
> +	request->timeout = timeout;
> 
> 	return request;
> }
> @@ -735,10 +777,11 @@ unsigned int mgmt_send_tlv(struct mgmt *mgmt, uint16_t opcode, uint16_t index,
> 	return ret;
> }
> 
> -unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index,
> -				uint16_t length, const void *param,
> -				mgmt_request_func_t callback,
> -				void *user_data, mgmt_destroy_func_t destroy)
> +unsigned int mgmt_send_timeout(struct mgmt *mgmt, uint16_t opcode,
> +				uint16_t index, uint16_t length,
> +				const void *param, mgmt_request_func_t callback,
> +				void *user_data, mgmt_destroy_func_t destroy,
> +				int timeout)
> {
> 	struct mgmt_request *request;
> 
> @@ -746,7 +789,7 @@ unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index,
> 		return 0;
> 
> 	request = create_request(mgmt, opcode, index, length, param,
> -					callback, user_data, destroy);
> +					callback, user_data, destroy, timeout);
> 	if (!request)
> 		return 0;
> 
> @@ -766,6 +809,15 @@ unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index,
> 	return request->id;
> }
> 
> +unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index,
> +				uint16_t length, const void *param,
> +				mgmt_request_func_t callback,
> +				void *user_data, mgmt_destroy_func_t destroy)
> +{
> +	return mgmt_send_timeout(mgmt, opcode, index, length, param, callback,
> +					user_data, destroy, MGMT_TIMEOUT);
> +}
> +

I am not happy with this. No every command has to setup a timeout handler and so on. This is not really what should be done since the mgmt communication with the kernel is actually pretty low latency.

If you want to add a special send_timeout, then do that, but leave the send alone.

Regards

Marcel




[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux