On Wed, Oct 22, 2014 at 03:11:54PM +0200, Lukas Slebodnik wrote:
On (22/10/14 14:26), Jakub Hrozek wrote:
>Hi,
>
>attached is a patch for an issue I found when working on the SELinux
>child.
>From d2bb267d4d7bdb39666cf95d8b457c16a51ef88a Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek(a)redhat.com>
>Date: Tue, 21 Oct 2014 16:18:02 +0200
>Subject: [PATCH] GPO: Terminate request on error
>
>---
> src/providers/ad/ad_gpo.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
>index
c81bd49e0c840afda19772f0591f51c9a3e2d617..83edbe4fb5a7e617cab95c0330ceae31392b18b2 100644
>--- a/src/providers/ad/ad_gpo.c
>+++ b/src/providers/ad/ad_gpo.c
>@@ -3898,11 +3898,13 @@ static void gpo_cse_done(struct tevent_req *subreq)
> "ad_gpo_parse_gpo_child_response failed: [%d][%s]\n",
> ret, strerror(ret));
> tevent_req_error(req, ret);
>+ return;
> } else if (child_result != 0){
> DEBUG(SSSDBG_CRIT_FAILURE,
> "Error in gpo_child: [%d][%s]\n",
> child_result, strerror(child_result));
> tevent_req_error(req, child_result);
>+ return;
> }
Pavel has already acked this patch.
I just want to start discussion about how to prevent such problems.
On some places we use to have similar construct in done/immediately section.
src/providers/ad/ad_domain_info.c- if (ret != EOK) {
src/providers/ad/ad_domain_info.c: tevent_req_error(req, ret);
src/providers/ad/ad_domain_info.c- } else {
src/providers/ad/ad_domain_info.c- tevent_req_done(req);
src/providers/ad/ad_domain_info.c- }
src/providers/ad/ad_domain_info.c- tevent_req_post(req, ev);
src/providers/ad/ad_domain_info.c- return req;
Maybe we should use it also on other places.
This is a pattern we use in _send(), yes. Do you suggest that we use it
also in other functions?
I'm not sure tevent_req_done is normally needed, but we could say that
we prefer 'goto fail' instead.
I was also thinking about a macro tevent_req_terminate() that would also
return, but I know that macros that change the flow of program are
considered evil by most developers..