[PATCH] winbindd: disconnect child process if request is cancelled at main process

Uri Simchoni urisimchoni at gmail.com
Wed Jun 24 02:09:13 MDT 2015


Thanks very much for reviewing.
I've tried following your suggestions and the example and indeed the
result is much more pleasant. Review appreciated.

One change I did make was to use the subrequest and not the child
pointer as a test for whether we have anything to do, and this is
because unlike the smbcli example, the request may be queued initially
with no real "io" request. In that state we want the cleanup to be a
no-op. More generally it's the presence of a subrequest that calls for
cleanup considerations and if there isn't one, then the cleanup should
be a no-op. Given that, I did not include the setting of child to NULL
once we're done with it, although one might want that out of general
hygiene considerations.

Thanks,
Uri.

On Wed, Jun 24, 2015 at 1:05 AM, Stefan (metze) Metzmacher
<metze at samba.org> wrote:
> Hi Uri,
>
>> This patch fixes what seems to be breakage of line protocol between
>> the main winbindd process and one of its child processes, if a request
>> that is right now being served at the child process, is cancelled
>> (deleted) at the main process. The current code seems to nicely clean
>> all related objects and leave nothing dangling behind, except there's
>> an unanswered request in the pipe between the parent and the child,
>> and so the next request will read something that's not the response to
>> its request.
>>
>> The fix is to close the connection and fork a new child. A different
>> possible fix would be to detach the request and let it finish, without
>> processing the result. The advantage of opening a new connection is
>> "getting back to business" instead of continue waiting. The
>> disadvantage is possibly creating more and more winbindd child
>> processes. Perhaps this should be accompanied by actively killing the
>> child process (at least sending SIGTERM once) and not letting it exit
>> when it fails to write its response.
>>
>> The patch is related to the rework I'm doing to the patch that
>> controls number of fds, although technically it is not part of this
>> rework as it is a bug fix and needed in both the old patch of
>> stopping/resuming accept and the new patch I'm working on.
>
> Some generic comments:
> - Please don't use tevent_req_set_cleanup_fn(req, NULL); The
>   cleanup function need some logic to turn into a no-op.
>   E.g. using something like: if (state->subreq == NULL) { return; }
>
> - tevent_req_error(subreq, ECANCELED); is wrong a caller
>   need to use tevent_req_cancel(subreq). Only the implementation
>   is allowed to call tevent_req_{done,error}(req) (or wrappers),
>   but not the caller.
>
> Regarding the specific patch:
>
> I think the cleanup function should call:
>
>
> TALLOC_FREE(state->subreq);
>
> if (state->child == NULL) {
>     /* nothing to cleanup */
>     return;
> }
>
> if (req_state == TEVENT_REQ_DONE) {
>     /* we're done and don't need a reference to the child anymore */
>     state->child = NULL;
>     return;
> }
>
> close(state->child->sock);
> state->child->sock = -1;
> DLIST_REMOVE(winbindd_children, state->child);
> state->child = NULL;
> return;
>
> That way we just need to remember the subreq and set the timeout on
> the subreq.
>
> metze
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cancel-child2.patch
Type: application/octet-stream
Size: 3861 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20150624/46b431b4/attachment.obj>


More information about the samba-technical mailing list