Hi Marcel,
On Tue, Jan 25, 2022 at 1:38 PM Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote:
>
> 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.
Well we don't have any way to check if the kernel is really responding
within a reasonable time, so I thought it would be a good idea to have
a default to capture when a command /reply does not respond, otherwise
it is pretty hard to debug such conditions since the request will stay
in the queue indefinitely.
> If you want to add a special send_timeout, then do that, but leave the send alone.
>
> Regards
>
> Marcel
>
--
Luiz Augusto von Dentz
