Hi Felix,
The SIGTERM ignore issue I was experiencing before is no longer
reproducible after I apply your patch.
However, I'm concerned about a possible ignore of SIGTERM signal
received during a ubus_complete_request() call. If ctx->stack_depth is
0, any such signal received between prev_uloop_initialization and the
reset of ulopp_cancelling to false will be ignored. Is this
"uloop_cancelling = false" really necessary?
BTW, I think the reset of uloop_status and uloop_cancelled should be
executed before uloop_setup_signals() like so:
if (!recursive_calls++) {
uloop_status = 0;
uloop_cancelled = false;
uloop_setup_signals(true);
}
Cheers,
Alin
On Fri, Feb 3, 2017 at 2:47 PM, Felix Fietkau <n...@nbd.name> wrote:
> Hi Alin,
>
> On 2017-02-03 09:29, Alin Năstac wrote:
>> Hi Felix,
>>
>> SIGTERM & SIGINT signals received during ubus_complete_request()
>> waiting for ubus_poll_data() to return are ignored due to
>> uloop_cancelled being restored to its previous value it had before
>> uloop_poll_data() was called.
>>
>> The reproduction scenario is this:
>> 1) cancelled local variable is set to false, current value of
>> uloop_cancelled
>> 2) while ubus_poll_data() is waiting for a read event, a SIGTERM is
>> received, so uloop_cancelled is set to true
>> 3) after ubus_poll_data() returns, uloop_cancelled value gets
>> overwritten with false and SIGTERM signal ends up being ignored in the
>> uloop_run()
>>
>> The whole uloop_cancelled related logic in the ubus_complete_request()
>> seems to be focused on getting out from the current recursion of
>> uloop_run(), but from what I see uloop_run() capability to be called
>> recursively is no longer used in ubus, so I wonder if it is still
>> necessary.
> I left in uloop_cancelled primarily to deal with cleaning up recursive
> requests after Ctrl+c or SIGTERM when uloop is in use.
>
>> If the answer is no, the entire logic referring to uloop_cancelled and
>> uloop_end() should be removed from libubus-req.c. Otherwise an
>> additional uloop_cancelled_recursions bit mask would be needed that
>> will allow to control uloop_run() exit condition for each individual
>> recursion.
> I think the main problem was the fact that uloop_cancelled was used
> both for internal request termination and also for external use.
> Here's a patch that should resolve this issue, please test:
>
> ---
> diff --git a/libubus-io.c b/libubus-io.c
> index 1075c65..1343bb2 100644
> --- a/libubus-io.c
> +++ b/libubus-io.c
> @@ -154,9 +154,10 @@ int __hidden ubus_send_msg(struct ubus_context *ctx,
> uint32_t seq,
> return ret;
> }
>
> -static int recv_retry(int fd, struct iovec *iov, bool wait, int *recv_fd)
> +static int recv_retry(struct ubus_context *ctx, struct iovec *iov, bool
> wait, int *recv_fd)
> {
> int bytes, total = 0;
> + int fd = ctx->sock.fd;
> static struct {
> struct cmsghdr h;
> int fd;
> @@ -191,7 +192,7 @@ static int recv_retry(int fd, struct iovec *iov, bool
> wait, int *recv_fd)
>
> if (bytes < 0) {
> bytes = 0;
> - if (uloop_cancelled)
> + if (uloop_cancelled || ctx->cancel_poll)
> return 0;
> if (errno == EINTR)
> continue;
> @@ -274,7 +275,7 @@ static bool get_next_msg(struct ubus_context *ctx, int
> *recv_fd)
> int r;
>
> /* receive header + start attribute */
> - r = recv_retry(ctx->sock.fd, &iov, false, recv_fd);
> + r = recv_retry(ctx, &iov, false, recv_fd);
> if (r <= 0) {
> if (r < 0)
> ctx->sock.eof = true;
> @@ -298,7 +299,7 @@ static bool get_next_msg(struct ubus_context *ctx, int
> *recv_fd)
> iov.iov_base = (char *)ctx->msgbuf.data + sizeof(hdrbuf.data);
> iov.iov_len = blob_len(ctx->msgbuf.data);
> if (iov.iov_len > 0 &&
> - recv_retry(ctx->sock.fd, &iov, true, NULL) <= 0)
> + recv_retry(ctx, &iov, true, NULL) <= 0)
> return false;
>
> return true;
> @@ -311,7 +312,7 @@ void __hidden ubus_handle_data(struct uloop_fd *u,
> unsigned int events)
>
> while (get_next_msg(ctx, &recv_fd)) {
> ubus_process_msg(ctx, &ctx->msgbuf, recv_fd);
> - if (uloop_cancelled)
> + if (uloop_cancelled || ctx->cancel_poll)
> break;
> }
>
> @@ -326,6 +327,7 @@ void __hidden ubus_poll_data(struct ubus_context *ctx,
> int timeout)
> .events = POLLIN | POLLERR,
> };
>
> + ctx->cancel_poll = false;
> poll(&pfd, 1, timeout ? timeout : -1);
> ubus_handle_data(&ctx->sock, ULOOP_READ);
> }
> diff --git a/libubus-req.c b/libubus-req.c
> index db5061c..305e813 100644
> --- a/libubus-req.c
> +++ b/libubus-req.c
> @@ -122,7 +122,7 @@ static void ubus_sync_req_cb(struct ubus_request *req,
> int ret)
> {
> req->status_msg = true;
> req->status_code = ret;
> - uloop_end();
> + req->ctx->cancel_poll = true;
> }
>
> static int64_t get_time_msec(void)
> @@ -142,6 +142,7 @@ int ubus_complete_request(struct ubus_context *ctx,
> struct ubus_request *req,
> ubus_complete_handler_t complete_cb = req->complete_cb;
> int status = UBUS_STATUS_NO_DATA;
> int64_t timeout = 0, time_end = 0;
> + bool prev_uloop_cancelled = uloop_cancelled;
>
> if (req_timeout)
> time_end = get_time_msec() + req_timeout;
> @@ -149,29 +150,32 @@ int ubus_complete_request(struct ubus_context *ctx,
> struct ubus_request *req,
> ubus_complete_request_async(ctx, req);
> req->complete_cb = ubus_sync_req_cb;
>
> + if (!ctx->stack_depth)
> + uloop_cancelled = false;
> +
> ctx->stack_depth++;
> while (!req->status_msg) {
> - bool cancelled = uloop_cancelled;
> -
> - uloop_cancelled = false;
> if (req_timeout) {
> timeout = time_end - get_time_msec();
> if (timeout <= 0) {
> ubus_set_req_status(req, UBUS_STATUS_TIMEOUT);
> - uloop_cancelled = cancelled;
> break;
> }
> }
> +
> ubus_poll_data(ctx, (unsigned int) timeout);
>
> - uloop_cancelled = cancelled;
> if (ctx->sock.eof) {
> ubus_set_req_status(req,
> UBUS_STATUS_CONNECTION_FAILED);
> + ctx->cancel_poll = true;
> break;
> }
> }
> +
> ctx->stack_depth--;
> if (ctx->stack_depth)
> + ctx->cancel_poll = true;
> + else if (prev_uloop_cancelled)
> uloop_cancelled = true;
>
> if (req->status_msg)
> diff --git a/libubus.h b/libubus.h
> index ea53272..4e45cb6 100644
> --- a/libubus.h
> +++ b/libubus.h
> @@ -155,6 +155,7 @@ struct ubus_context {
>
> uint32_t local_id;
> uint16_t request_seq;
> + bool cancel_poll;
> int stack_depth;
>
> void (*connection_lost)(struct ubus_context *ctx);
>
_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
