[SSSD] [PATCH] Patches and improvements for the child_common.c module

Jakub Hrozek jhrozek at redhat.com
Thu Dec 11 11:01:39 UTC 2014


Hi,

attached are some small patches and most importantly unit tests I wrote
recently when patching the child_common.c module.

Please note that these patches shouldn't be applied until we split off
the master branch from 1.12, but if I don't send them out before the
Christmas break I'll forget completely :-)
-------------- next part --------------
>From 00eccfcc29f6e81e19f5852e6551a9497bab118b Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Sat, 29 Nov 2014 10:10:30 +0100
Subject: [PATCH 1/5] UTIL: Make child_sig_handler static

The function was only used internally, makes no sense to keep it in the
child_common module API.
---
 src/util/child_common.c | 10 +++++++---
 src/util/child_common.h |  4 ----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/util/child_common.c b/src/util/child_common.c
index d3f488d181e83245f8a3ec003b60671248745bd5..3b159f9d9a6144d348369ee39a5cca93fcd9c46e 100644
--- a/src/util/child_common.c
+++ b/src/util/child_common.c
@@ -262,6 +262,10 @@ struct sss_child_ctx_old {
     void *pvt;
 };
 
+static void child_sig_handler(struct tevent_context *ev,
+                              struct tevent_signal *sige, int signum,
+                              int count, void *__siginfo, void *pvt);
+
 int child_handler_setup(struct tevent_context *ev, int pid,
                         sss_child_callback_t cb, void *pvt,
                         struct sss_child_ctx_old **_child_ctx)
@@ -533,9 +537,9 @@ void fd_nonblocking(int fd)
 static void child_invoke_callback(struct tevent_context *ev,
                                   struct tevent_immediate *imm,
                                   void *pvt);
-void child_sig_handler(struct tevent_context *ev,
-                       struct tevent_signal *sige, int signum,
-                       int count, void *__siginfo, void *pvt)
+static void child_sig_handler(struct tevent_context *ev,
+                              struct tevent_signal *sige, int signum,
+                              int count, void *__siginfo, void *pvt)
 {
     int ret, err;
     struct sss_child_ctx_old *child_ctx;
diff --git a/src/util/child_common.h b/src/util/child_common.h
index e659388ece3677b7746c159d7de3e86171bb4146..9124bc3fd602a9d8aa38cb31bf1feffa5989ff94 100644
--- a/src/util/child_common.h
+++ b/src/util/child_common.h
@@ -107,10 +107,6 @@ int read_pipe_recv(struct tevent_req *req, TALLOC_CTX *mem_ctx,
 /* The pipes to communicate with the child must be nonblocking */
 void fd_nonblocking(int fd);
 
-void child_sig_handler(struct tevent_context *ev,
-                       struct tevent_signal *sige, int signum,
-                       int count, void *__siginfo, void *pvt);
-
 /* Never returns EOK, ether returns an error, or doesn't return on success */
 errno_t exec_child(TALLOC_CTX *mem_ctx,
                    int *pipefd_to_child, int *pipefd_from_child,
-- 
1.9.3

-------------- next part --------------
>From 5b219f1196ca9500ad2d7b1c3a9a508f8f06f4b0 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Mon, 1 Dec 2014 12:06:00 +0100
Subject: [PATCH 2/5] TESTS: Cover child_common.c with unit tests

The module wasn't tested properly, which made it harder to patch it.
---
 Makefile.am                          |   3 +
 src/tests/cmocka/dummy_child.c       |  29 ++++
 src/tests/cmocka/test_child_common.c | 299 +++++++++++++++++++++++++++++++++++
 src/util/child_common.c              |  19 ++-
 src/util/child_common.h              |   7 -
 5 files changed, 344 insertions(+), 13 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 85699d11ed85d4af7b9be5905271e6ce2eba62eb..1a87561bf1e6612ba384fff4bcbacd83184a17a6 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2170,6 +2170,9 @@ test_child_common_CFLAGS = \
     $(AM_CFLAGS) \
     -DCHILD_DIR=\"$(builddir)\" \
     $(NULL)
+test_child_common_LDFLAGS = \
+    -Wl,-wrap,child_io_destructor \
+    $(NULL)
 test_child_common_LDADD = \
     $(CMOCKA_LIBS) \
     $(POPT_LIBS) \
diff --git a/src/tests/cmocka/dummy_child.c b/src/tests/cmocka/dummy_child.c
index 057fa6450b51cbac589e64553bb0692cbbfac655..e2c7fd30eff65c7cb17bd5cff429593f95ccc8a8 100644
--- a/src/tests/cmocka/dummy_child.c
+++ b/src/tests/cmocka/dummy_child.c
@@ -35,6 +35,10 @@ int main(int argc, const char *argv[])
     int opt;
     int debug_fd = -1;
     poptContext pc;
+    ssize_t len;
+    ssize_t written;
+    errno_t ret;
+    uint8_t buf[IN_BUF_SIZE];
     const char *action = NULL;
     const char *guitar;
     const char *drums;
@@ -80,6 +84,31 @@ int main(int argc, const char *argv[])
                 DEBUG(SSSDBG_CRIT_FAILURE, "This band sounds weird\n");
                 _exit(1);
             }
+        } else if (strcasecmp(action, "echo") == 0) {
+            errno = 0;
+            len = sss_atomic_read_s(STDIN_FILENO, buf, IN_BUF_SIZE);
+            if (len == -1) {
+                ret = errno;
+                DEBUG(SSSDBG_CRIT_FAILURE, "read failed [%d][%s].\n", ret, strerror(ret));
+                _exit(1);
+            }
+            close(STDIN_FILENO);
+
+            errno = 0;
+            written = sss_atomic_write_s(STDOUT_FILENO, buf, len);
+            if (written == -1) {
+                ret = errno;
+                DEBUG(SSSDBG_CRIT_FAILURE, "write failed [%d][%s].\n", ret,
+                            strerror(ret));
+                _exit(1);
+            }
+            close(STDOUT_FILENO);
+
+            if (written != len) {
+                DEBUG(SSSDBG_CRIT_FAILURE, "Expected to write %zu bytes, wrote %zu\n",
+                      len, written);
+                _exit(1);
+            }
         }
     }
 
diff --git a/src/tests/cmocka/test_child_common.c b/src/tests/cmocka/test_child_common.c
index ee6e297f58b0895b5401464bb3f9b5386a3e6411..cf64744269fa0da8f9acfc3ab26de13885d808d1 100644
--- a/src/tests/cmocka/test_child_common.c
+++ b/src/tests/cmocka/test_child_common.c
@@ -31,6 +31,8 @@
 #define TEST_BIN    "dummy-child"
 #define ECHO_STR    "Hello child"
 
+static int destructor_called;
+
 struct child_test_ctx {
     int pipefd_to_child[2];
     int pipefd_from_child[2];
@@ -153,6 +155,291 @@ void test_exec_child_extra_args(void **state)
     }
 }
 
+struct tevent_req *echo_child_write_send(TALLOC_CTX *mem_ctx,
+                                         struct child_test_ctx *child_tctx,
+                                         struct child_io_fds *io_fds,
+                                         const char *input);
+static void echo_child_write_done(struct tevent_req *subreq);
+static void echo_child_read_done(struct tevent_req *subreq);
+
+int __real_child_io_destructor(void *ptr);
+
+int __wrap_child_io_destructor(void *ptr)
+{
+    destructor_called = 1;
+    return __real_child_io_destructor(ptr);
+}
+
+/* Test that writing to the pipes works as expected */
+void test_exec_child_io_destruct(void **state)
+{
+    struct child_test_ctx *child_tctx = talloc_get_type(*state,
+                                                        struct child_test_ctx);
+    struct child_io_fds *io_fds;
+
+    io_fds = talloc(child_tctx, struct child_io_fds);
+    io_fds->read_from_child_fd = -1;
+    io_fds->write_to_child_fd = -1;
+    assert_non_null(io_fds);
+    talloc_set_destructor((void *) io_fds, child_io_destructor);
+
+    io_fds->read_from_child_fd = child_tctx->pipefd_from_child[0];
+    io_fds->write_to_child_fd = child_tctx->pipefd_to_child[1];
+
+    destructor_called = 0;
+    talloc_free(io_fds);
+    assert_int_equal(destructor_called, 1);
+
+    errno = 0;
+    close(child_tctx->pipefd_from_child[0]);
+    assert_int_equal(errno, EBADF);
+
+    errno = 0;
+    close(child_tctx->pipefd_from_child[1]);
+    assert_int_equal(errno, 0);
+
+    errno = 0;
+    close(child_tctx->pipefd_to_child[0]);
+    assert_int_equal(errno, 0);
+
+    errno = 0;
+    close(child_tctx->pipefd_to_child[1]);
+    assert_int_equal(errno, EBADF);
+}
+
+void test_child_cb(int child_status,
+                   struct tevent_signal *sige,
+                   void *pvt);
+
+/* Test that writing to the pipes works as expected */
+void test_exec_child_handler(void **state)
+{
+    errno_t ret;
+    pid_t child_pid;
+    struct child_test_ctx *child_tctx = talloc_get_type(*state,
+                                                        struct child_test_ctx);
+    struct sss_child_ctx_old *child_old_ctx;
+
+    ret = unsetenv("TEST_CHILD_ACTION");
+    assert_int_equal(ret, 0);
+
+    child_pid = fork();
+    assert_int_not_equal(child_pid, -1);
+    if (child_pid == 0) {
+        ret = exec_child(child_tctx,
+                         child_tctx->pipefd_to_child,
+                         child_tctx->pipefd_from_child,
+                         CHILD_DIR"/"TEST_BIN, 2, NULL);
+        assert_int_equal(ret, EOK);
+    }
+
+    ret = child_handler_setup(child_tctx->test_ctx->ev, child_pid,
+                              test_child_cb, child_tctx, &child_old_ctx);
+    assert_int_equal(ret, EOK);
+
+    ret = test_ev_loop(child_tctx->test_ctx);
+    assert_int_equal(ret, EOK);
+    assert_int_equal(child_tctx->test_ctx->error, 0);
+}
+
+void test_child_cb(int child_status,
+                   struct tevent_signal *sige,
+                   void *pvt)
+{
+    struct child_test_ctx *child_ctx = talloc_get_type(pvt, struct child_test_ctx);
+
+    child_ctx->test_ctx->error = EIO;
+    if (WIFEXITED(child_status) && WEXITSTATUS(child_status) == 0) {
+        child_ctx->test_ctx->error = 0;
+    }
+
+    child_ctx->test_ctx->done = true;
+}
+
+/* Test that writing to the pipes works as expected */
+void test_exec_child_echo(void **state)
+{
+    errno_t ret;
+    pid_t child_pid;
+    struct child_test_ctx *child_tctx = talloc_get_type(*state,
+                                                        struct child_test_ctx);
+    struct tevent_req *req;
+    struct child_io_fds *io_fds;
+
+    setenv("TEST_CHILD_ACTION", "echo", 1);
+
+    io_fds = talloc(child_tctx, struct child_io_fds);
+    assert_non_null(io_fds);
+    io_fds->read_from_child_fd = -1;
+    io_fds->write_to_child_fd = -1;
+    talloc_set_destructor((void *) io_fds, child_io_destructor);
+
+    child_pid = fork();
+    assert_int_not_equal(child_pid, -1);
+    if (child_pid == 0) {
+        ret = exec_child(child_tctx,
+                         child_tctx->pipefd_to_child,
+                         child_tctx->pipefd_from_child,
+                         CHILD_DIR"/"TEST_BIN, 2, NULL);
+        assert_int_equal(ret, EOK);
+    }
+
+    DEBUG(SSSDBG_FUNC_DATA, "Forked into %d\n", child_pid);
+
+    io_fds->read_from_child_fd = child_tctx->pipefd_from_child[0];
+    close(child_tctx->pipefd_from_child[1]);
+    io_fds->write_to_child_fd = child_tctx->pipefd_to_child[1];
+    close(child_tctx->pipefd_to_child[0]);
+
+    fd_nonblocking(io_fds->write_to_child_fd);
+    fd_nonblocking(io_fds->read_from_child_fd);
+
+    ret = child_handler_setup(child_tctx->test_ctx->ev, child_pid,
+                              NULL, NULL, NULL);
+    assert_int_equal(ret, EOK);
+
+    req = echo_child_write_send(child_tctx, child_tctx, io_fds, ECHO_STR);
+    assert_non_null(req);
+
+    ret = test_ev_loop(child_tctx->test_ctx);
+    talloc_free(io_fds);
+    assert_int_equal(ret, EOK);
+}
+
+struct test_exec_echo_state {
+    struct child_io_fds *io_fds;
+    struct io_buffer buf;
+    struct child_test_ctx *child_test_ctx;
+};
+
+struct tevent_req *echo_child_write_send(TALLOC_CTX *mem_ctx,
+                                         struct child_test_ctx *child_tctx,
+                                         struct child_io_fds *io_fds,
+                                         const char *input)
+{
+    struct tevent_req *req;
+    struct tevent_req *subreq;
+    struct test_exec_echo_state *echo_state;
+
+    req = tevent_req_create(mem_ctx, &echo_state, struct test_exec_echo_state);
+    assert_non_null(req);
+
+    echo_state->child_test_ctx = child_tctx;
+
+    echo_state->buf.data = (unsigned char *) talloc_strdup(echo_state, input);
+    assert_non_null(echo_state->buf.data);
+    echo_state->buf.size = strlen(input) + 1;
+    echo_state->io_fds = io_fds;
+
+    DEBUG(SSSDBG_TRACE_INTERNAL, "Writing..\n");
+    subreq = write_pipe_send(child_tctx, child_tctx->test_ctx->ev,
+                             echo_state->buf.data, echo_state->buf.size,
+                             echo_state->io_fds->write_to_child_fd);
+    assert_non_null(subreq);
+    tevent_req_set_callback(subreq, echo_child_write_done, req);
+
+    return req;
+}
+
+static void echo_child_write_done(struct tevent_req *subreq)
+{
+    struct tevent_req *req;
+    struct test_exec_echo_state *echo_state;
+    errno_t ret;
+
+    req = tevent_req_callback_data(subreq, struct tevent_req);
+    echo_state = tevent_req_data(req, struct test_exec_echo_state);
+
+    ret = write_pipe_recv(subreq);
+    DEBUG(SSSDBG_TRACE_INTERNAL, "Writing OK\n");
+    talloc_zfree(subreq);
+    assert_int_equal(ret, EOK);
+
+    close(echo_state->io_fds->write_to_child_fd);
+    echo_state->io_fds->write_to_child_fd = -1;
+
+    DEBUG(SSSDBG_TRACE_INTERNAL, "Reading..\n");
+    subreq = read_pipe_send(echo_state,
+                            echo_state->child_test_ctx->test_ctx->ev,
+                            echo_state->io_fds->read_from_child_fd);
+    assert_non_null(subreq);
+    tevent_req_set_callback(subreq, echo_child_read_done, req);
+}
+
+static void echo_child_read_done(struct tevent_req *subreq)
+{
+    struct tevent_req *req;
+    struct test_exec_echo_state *echo_state;
+    errno_t ret;
+    ssize_t len;
+    uint8_t *buf;
+
+    req = tevent_req_callback_data(subreq, struct tevent_req);
+    echo_state = tevent_req_data(req, struct test_exec_echo_state);
+
+    ret = read_pipe_recv(subreq, echo_state, &buf, &len);
+    talloc_zfree(subreq);
+    DEBUG(SSSDBG_TRACE_INTERNAL, "Reading OK\n");
+    assert_int_equal(ret, EOK);
+
+    close(echo_state->io_fds->read_from_child_fd);
+    echo_state->io_fds->read_from_child_fd = -1;
+
+    assert_string_equal(buf, echo_state->buf.data);
+    echo_state->child_test_ctx->test_ctx->done = true;
+}
+
+void sss_child_cb(int pid, int wait_status, void *pvt);
+
+/* Just make sure the exec works. The child does nothing but exits */
+void test_sss_child(void **state)
+{
+    errno_t ret;
+    pid_t child_pid;
+    struct child_test_ctx *child_tctx = talloc_get_type(*state,
+                                                        struct child_test_ctx);
+    struct sss_sigchild_ctx *sc_ctx;
+    struct sss_child_ctx *sss_child;
+
+    ret = unsetenv("TEST_CHILD_ACTION");
+    assert_int_equal(ret, 0);
+
+    ret = sss_sigchld_init(child_tctx, child_tctx->test_ctx->ev, &sc_ctx);
+    assert_int_equal(ret, EOK);
+
+    child_pid = fork();
+    assert_int_not_equal(child_pid, -1);
+    if (child_pid == 0) {
+        ret = exec_child(child_tctx,
+                         child_tctx->pipefd_to_child,
+                         child_tctx->pipefd_from_child,
+                         CHILD_DIR"/"TEST_BIN, 2, NULL);
+        assert_int_equal(ret, EOK);
+    }
+
+    ret = sss_child_register(child_tctx, sc_ctx,
+                             child_pid,
+                             sss_child_cb,
+                             child_tctx, &sss_child);
+    assert_int_equal(ret, EOK);
+
+    ret = test_ev_loop(child_tctx->test_ctx);
+    assert_int_equal(ret, EOK);
+    assert_int_equal(child_tctx->test_ctx->error, 0);
+}
+
+void sss_child_cb(int pid, int wait_status, void *pvt)
+{
+    struct child_test_ctx *child_ctx = talloc_get_type(pvt, struct child_test_ctx);
+
+    child_ctx->test_ctx->error = EIO;
+    if (WIFEXITED(wait_status) && WEXITSTATUS(wait_status) == 0) {
+        child_ctx->test_ctx->error = 0;
+    }
+
+    child_ctx->test_ctx->done = true;
+}
+
 int main(int argc, const char *argv[])
 {
     int rv;
@@ -171,6 +458,18 @@ int main(int argc, const char *argv[])
         unit_test_setup_teardown(test_exec_child_extra_args,
                                  child_test_setup,
                                  child_test_teardown),
+        unit_test_setup_teardown(test_exec_child_io_destruct,
+                                 child_test_setup,
+                                 child_test_teardown),
+        unit_test_setup_teardown(test_exec_child_handler,
+                                 child_test_setup,
+                                 child_test_teardown),
+        unit_test_setup_teardown(test_exec_child_echo,
+                                 child_test_setup,
+                                 child_test_teardown),
+        unit_test_setup_teardown(test_sss_child,
+                                 child_test_setup,
+                                 child_test_teardown),
     };
 
     /* Set debug level to invalid value so we can deside if -d 0 was used. */
diff --git a/src/util/child_common.c b/src/util/child_common.c
index 3b159f9d9a6144d348369ee39a5cca93fcd9c46e..ba63c48f73fa9b1153b5a1c729e1524b548dd4c4 100644
--- a/src/util/child_common.c
+++ b/src/util/child_common.c
@@ -46,6 +46,13 @@ struct sss_child_ctx {
     struct sss_sigchild_ctx *sigchld_ctx;
 };
 
+static void sss_child_handler(struct tevent_context *ev,
+                              struct tevent_signal *se,
+                              int signum,
+                              int count,
+                              void *siginfo,
+                              void *private_data);
+
 errno_t sss_sigchld_init(TALLOC_CTX *mem_ctx,
                          struct tevent_context *ev,
                          struct sss_sigchild_ctx **child_ctx)
@@ -177,12 +184,12 @@ static void sss_child_invoke_cb(struct tevent_context *ev,
     talloc_free(imm);
 }
 
-void sss_child_handler(struct tevent_context *ev,
-                       struct tevent_signal *se,
-                       int signum,
-                       int count,
-                       void *siginfo,
-                       void *private_data)
+static void sss_child_handler(struct tevent_context *ev,
+                              struct tevent_signal *se,
+                              int signum,
+                              int count,
+                              void *siginfo,
+                              void *private_data)
 {
     struct sss_sigchild_ctx *sigchld_ctx;
     struct tevent_immediate *imm;
diff --git a/src/util/child_common.h b/src/util/child_common.h
index 9124bc3fd602a9d8aa38cb31bf1feffa5989ff94..10753a1bbc882339fa9aec0f271c784a9cb0343c 100644
--- a/src/util/child_common.h
+++ b/src/util/child_common.h
@@ -68,13 +68,6 @@ errno_t sss_child_register(TALLOC_CTX *mem_ctx,
                            void *pvt,
                            struct sss_child_ctx **child_ctx);
 
-void sss_child_handler(struct tevent_context *ev,
-                       struct tevent_signal *se,
-                       int signum,
-                       int count,
-                       void *siginfo,
-                       void *private_data);
-
 /* Callback to be invoked when a sigchld handler is called.
  * The tevent_signal * associated with the handler will be
  * freed automatically when this function returns.
-- 
1.9.3

-------------- next part --------------
>From 5fa9a62a5f892233da4071147c5d28d3dfdb7781 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Mon, 8 Dec 2014 14:08:26 +0100
Subject: [PATCH 3/5] LDAP: Use child_io_destructor instead of child_cleanup in
 a custom desctructor

ldap_child was the only child process that used child_cleanup instead of
the common child_io_destructor. Unify the implementation to use the
common function instead.
---
 src/providers/ldap/sdap_child_helpers.c | 39 ++++++++++++++-------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/src/providers/ldap/sdap_child_helpers.c b/src/providers/ldap/sdap_child_helpers.c
index b60891d2b41f9a359856eb22174128d7f07559fb..58c951561d5d4ee00a1b8be82107d8228b54101d 100644
--- a/src/providers/ldap/sdap_child_helpers.c
+++ b/src/providers/ldap/sdap_child_helpers.c
@@ -47,8 +47,7 @@
 struct sdap_child {
     /* child info */
     pid_t pid;
-    int read_from_child_fd;
-    int write_to_child_fd;
+    struct child_io_fds *io;
 };
 
 static void sdap_close_fd(int *fd)
@@ -70,15 +69,6 @@ static void sdap_close_fd(int *fd)
     *fd = -1;
 }
 
-static int sdap_child_destructor(void *ptr)
-{
-    struct sdap_child *child = talloc_get_type(ptr, struct sdap_child);
-
-    child_cleanup(child->read_from_child_fd, child->write_to_child_fd);
-
-    return 0;
-}
-
 static errno_t sdap_fork_child(struct tevent_context *ev,
                                struct sdap_child *child)
 {
@@ -115,12 +105,12 @@ static errno_t sdap_fork_child(struct tevent_context *ev,
         return err;
     } else if (pid > 0) { /* parent */
         child->pid = pid;
-        child->read_from_child_fd = pipefd_from_child[0];
+        child->io->read_from_child_fd = pipefd_from_child[0];
         close(pipefd_from_child[1]);
-        child->write_to_child_fd = pipefd_to_child[1];
+        child->io->write_to_child_fd = pipefd_to_child[1];
         close(pipefd_to_child[0]);
-        fd_nonblocking(child->read_from_child_fd);
-        fd_nonblocking(child->write_to_child_fd);
+        fd_nonblocking(child->io->read_from_child_fd);
+        fd_nonblocking(child->io->write_to_child_fd);
 
         ret = child_handler_setup(ev, pid, NULL, NULL, NULL);
         if (ret != EOK) {
@@ -297,9 +287,14 @@ struct tevent_req *sdap_get_tgt_send(TALLOC_CTX *mem_ctx,
         goto fail;
     }
 
-    state->child->read_from_child_fd = -1;
-    state->child->write_to_child_fd = -1;
-    talloc_set_destructor((TALLOC_CTX *)state->child, sdap_child_destructor);
+    state->child->io = talloc(state, struct child_io_fds);
+    if (state->child->io == NULL) {
+        ret = ENOMEM;
+        goto fail;
+    }
+    state->child->io->read_from_child_fd = -1;
+    state->child->io->write_to_child_fd = -1;
+    talloc_set_destructor((TALLOC_CTX *) state->child->io, child_io_destructor);
 
     /* prepare the data to pass to child */
     ret = create_tgt_req_send_buffer(state,
@@ -323,7 +318,7 @@ struct tevent_req *sdap_get_tgt_send(TALLOC_CTX *mem_ctx,
     }
 
     subreq = write_pipe_send(state, ev, buf->data, buf->size,
-                             state->child->write_to_child_fd);
+                             state->child->io->write_to_child_fd);
     if (!subreq) {
         ret = ENOMEM;
         goto fail;
@@ -353,10 +348,10 @@ static void sdap_get_tgt_step(struct tevent_req *subreq)
         return;
     }
 
-    sdap_close_fd(&state->child->write_to_child_fd);
+    sdap_close_fd(&state->child->io->write_to_child_fd);
 
     subreq = read_pipe_send(state, state->ev,
-                            state->child->read_from_child_fd);
+                            state->child->io->read_from_child_fd);
     if (!subreq) {
         tevent_req_error(req, ENOMEM);
         return;
@@ -379,7 +374,7 @@ static void sdap_get_tgt_done(struct tevent_req *subreq)
         return;
     }
 
-    sdap_close_fd(&state->child->read_from_child_fd);
+    sdap_close_fd(&state->child->io->read_from_child_fd);
 
     tevent_req_done(req);
 }
-- 
1.9.3

-------------- next part --------------
>From 9933ae09e39aa6332112de6277d329c2487a491f Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Mon, 8 Dec 2014 14:09:12 +0100
Subject: [PATCH 4/5] UTIL: Remove child_cleanup

Not used anymore after previous patch.
---
 src/util/child_common.c | 22 ----------------------
 src/util/child_common.h |  2 --
 2 files changed, 24 deletions(-)

diff --git a/src/util/child_common.c b/src/util/child_common.c
index ba63c48f73fa9b1153b5a1c729e1524b548dd4c4..89cc07cbf26df7e5af59fc9054e83c4cd69501e0 100644
--- a/src/util/child_common.c
+++ b/src/util/child_common.c
@@ -781,28 +781,6 @@ errno_t exec_child(TALLOC_CTX *mem_ctx,
     return err;
 }
 
-void child_cleanup(int readfd, int writefd)
-{
-    int ret;
-
-    if (readfd != -1) {
-        ret = close(readfd);
-        if (ret != EOK) {
-            ret = errno;
-            DEBUG(SSSDBG_CRIT_FAILURE,
-                  "close failed [%d][%s].\n", ret, strerror(ret));
-        }
-    }
-    if (writefd != -1) {
-        ret = close(writefd);
-        if (ret != EOK) {
-            ret = errno;
-            DEBUG(SSSDBG_CRIT_FAILURE,
-                  "close failed [%d][%s].\n", ret, strerror(ret));
-        }
-    }
-}
-
 int child_io_destructor(void *ptr)
 {
     int ret;
diff --git a/src/util/child_common.h b/src/util/child_common.h
index 10753a1bbc882339fa9aec0f271c784a9cb0343c..7801d4a4eed3afe6b708fe6cb33c8723ea9adeff 100644
--- a/src/util/child_common.h
+++ b/src/util/child_common.h
@@ -106,8 +106,6 @@ errno_t exec_child(TALLOC_CTX *mem_ctx,
                    const char *binary, int debug_fd,
                    const char *extra_argv[]);
 
-void child_cleanup(int readfd, int writefd);
-
 int child_io_destructor(void *ptr);
 
 errno_t child_debug_init(const char *logfile, int *debug_fd);
-- 
1.9.3

-------------- next part --------------
>From 9fe6afd24d1b7fc10818a395c1c7f9cba2e155ae Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Mon, 8 Dec 2014 17:39:57 +0100
Subject: [PATCH 5/5] UTIL: Unify the fd_nonblocking implementation

The responder and child_common modules each had their own
implementation. Unify it instead and add a unit test.
---
 src/providers/ad/ad_gpo.c               |  4 ++--
 src/providers/ipa/ipa_selinux.c         |  4 ++--
 src/providers/krb5/krb5_child_handler.c |  4 ++--
 src/providers/ldap/sdap_child_helpers.c |  4 ++--
 src/responder/common/responder_common.c | 25 +------------------------
 src/tests/cmocka/test_child_common.c    |  4 ++--
 src/tests/util-tests.c                  | 21 +++++++++++++++++++++
 src/util/child_common.c                 | 23 -----------------------
 src/util/util.c                         | 24 ++++++++++++++++++++++++
 src/util/util.h                         | 12 ++++++++++++
 10 files changed, 68 insertions(+), 57 deletions(-)

diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index 62715861c91484fa2a57e7cc13ba403c9096d9a7..42938e719eb87c39d1e9c820dae3c00e07edea7f 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -3970,8 +3970,8 @@ gpo_fork_child(struct tevent_req *req)
         close(pipefd_from_child[1]);
         state->io->write_to_child_fd = pipefd_to_child[1];
         close(pipefd_to_child[0]);
-        fd_nonblocking(state->io->read_from_child_fd);
-        fd_nonblocking(state->io->write_to_child_fd);
+        sss_fd_nonblocking(state->io->read_from_child_fd);
+        sss_fd_nonblocking(state->io->write_to_child_fd);
 
         ret = child_handler_setup(state->ev, pid, NULL, NULL, NULL);
         if (ret != EOK) {
diff --git a/src/providers/ipa/ipa_selinux.c b/src/providers/ipa/ipa_selinux.c
index c4e70cfcb0748988d91fc1db57cf5a30d5365be4..0712247f261039191b17a6b5871f7fbdb0287fb6 100644
--- a/src/providers/ipa/ipa_selinux.c
+++ b/src/providers/ipa/ipa_selinux.c
@@ -1059,8 +1059,8 @@ static errno_t selinux_fork_child(struct selinux_child_state *state)
         close(pipefd_from_child[1]);
         state->io->write_to_child_fd = pipefd_to_child[1];
         close(pipefd_to_child[0]);
-        fd_nonblocking(state->io->read_from_child_fd);
-        fd_nonblocking(state->io->write_to_child_fd);
+        sss_fd_nonblocking(state->io->read_from_child_fd);
+        sss_fd_nonblocking(state->io->write_to_child_fd);
 
         ret = child_handler_setup(state->ev, pid, NULL, NULL, NULL);
         if (ret != EOK) {
diff --git a/src/providers/krb5/krb5_child_handler.c b/src/providers/krb5/krb5_child_handler.c
index 1454d220fb294abc339df6e862154012a03fdca0..36dca2a23599a741427432e69747241f8b178fa5 100644
--- a/src/providers/krb5/krb5_child_handler.c
+++ b/src/providers/krb5/krb5_child_handler.c
@@ -320,8 +320,8 @@ static errno_t fork_child(struct tevent_req *req)
         close(pipefd_from_child[1]);
         state->io->write_to_child_fd = pipefd_to_child[1];
         close(pipefd_to_child[0]);
-        fd_nonblocking(state->io->read_from_child_fd);
-        fd_nonblocking(state->io->write_to_child_fd);
+        sss_fd_nonblocking(state->io->read_from_child_fd);
+        sss_fd_nonblocking(state->io->write_to_child_fd);
 
         ret = child_handler_setup(state->ev, pid, NULL, NULL, NULL);
         if (ret != EOK) {
diff --git a/src/providers/ldap/sdap_child_helpers.c b/src/providers/ldap/sdap_child_helpers.c
index 58c951561d5d4ee00a1b8be82107d8228b54101d..3dc591f9fd0a31c56726ec9ed6e409218c3e6891 100644
--- a/src/providers/ldap/sdap_child_helpers.c
+++ b/src/providers/ldap/sdap_child_helpers.c
@@ -109,8 +109,8 @@ static errno_t sdap_fork_child(struct tevent_context *ev,
         close(pipefd_from_child[1]);
         child->io->write_to_child_fd = pipefd_to_child[1];
         close(pipefd_to_child[0]);
-        fd_nonblocking(child->io->read_from_child_fd);
-        fd_nonblocking(child->io->write_to_child_fd);
+        sss_fd_nonblocking(child->io->read_from_child_fd);
+        sss_fd_nonblocking(child->io->write_to_child_fd);
 
         ret = child_handler_setup(ev, pid, NULL, NULL, NULL);
         if (ret != EOK) {
diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c
index 6646fa2587a8299de40eaef35830351136b8149a..bc146203ce909f7cefbde134081bf0f40a71576f 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -22,8 +22,6 @@
 #include "config.h"
 
 #include <stdio.h>
-#include <unistd.h>
-#include <fcntl.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/socket.h>
@@ -45,27 +43,6 @@
 #include "monitor/monitor_interfaces.h"
 #include "sbus/sbus_client.h"
 
-static errno_t set_nonblocking(int fd)
-{
-    int v;
-    int ferr;
-    errno_t error;
-
-    /* Get the current flags for this file descriptor */
-    v = fcntl(fd, F_GETFL, 0);
-
-    errno = 0;
-    /* Set the non-blocking flag on this fd */
-    ferr = fcntl(fd, F_SETFL, v | O_NONBLOCK);
-    if (ferr < 0) {
-        error = errno;
-        DEBUG(SSSDBG_FATAL_FAILURE, "Unable to set fd non-blocking: [%d][%s]\n",
-                  error, strerror(error));
-        return error;
-    }
-    return EOK;
-}
-
 static errno_t set_close_on_exec(int fd)
 {
     int v;
@@ -598,7 +575,7 @@ int create_pipe_fd(const char *sock_name, int *_fd, mode_t umaskval)
 
     orig_umaskval = umask(umaskval);
 
-    ret = set_nonblocking(fd);
+    ret = sss_fd_nonblocking(fd);
     if (ret != EOK) {
         goto done;
     }
diff --git a/src/tests/cmocka/test_child_common.c b/src/tests/cmocka/test_child_common.c
index cf64744269fa0da8f9acfc3ab26de13885d808d1..c1a173175c109a04dbcbc80ec2fc81b678921676 100644
--- a/src/tests/cmocka/test_child_common.c
+++ b/src/tests/cmocka/test_child_common.c
@@ -291,8 +291,8 @@ void test_exec_child_echo(void **state)
     io_fds->write_to_child_fd = child_tctx->pipefd_to_child[1];
     close(child_tctx->pipefd_to_child[0]);
 
-    fd_nonblocking(io_fds->write_to_child_fd);
-    fd_nonblocking(io_fds->read_from_child_fd);
+    sss_fd_nonblocking(io_fds->write_to_child_fd);
+    sss_fd_nonblocking(io_fds->read_from_child_fd);
 
     ret = child_handler_setup(child_tctx->test_ctx->ev, child_pid,
                               NULL, NULL, NULL);
diff --git a/src/tests/util-tests.c b/src/tests/util-tests.c
index 08e8b8d263c46618a0cdfb2203684305fa6dddc6..94015d8e1e0efb143a4fea998f1b16db1e63365e 100644
--- a/src/tests/util-tests.c
+++ b/src/tests/util-tests.c
@@ -407,6 +407,26 @@ START_TEST(test_sss_filter_sanitize)
 }
 END_TEST
 
+START_TEST(test_fd_nonblocking)
+{
+    int fd;
+    int flags;
+    errno_t ret;
+
+    fd = open("/dev/null", O_RDONLY);
+    fail_unless(fd > 0);
+
+    flags = fcntl(fd, F_GETFL, 0);
+    fail_if(flags & O_NONBLOCK);
+
+    ret = sss_fd_nonblocking(fd);
+    fail_unless(ret == EOK);
+    flags = fcntl(fd, F_GETFL, 0);
+    fail_unless(flags & O_NONBLOCK);
+    close(fd);
+}
+END_TEST
+
 START_TEST(test_size_t_overflow)
 {
     fail_unless(!SIZE_T_OVERFLOW(1, 1), "unexpected overflow");
@@ -1020,6 +1040,7 @@ Suite *util_suite(void)
     tcase_add_test (tc_util, test_check_ipv6_addr);
     tcase_add_test (tc_util, test_is_host_in_domain);
     tcase_add_test (tc_util, test_known_service);
+    tcase_add_test (tc_util, test_fd_nonblocking);
     tcase_set_timeout(tc_util, 60);
 
     TCase *tc_utf8 = tcase_create("utf8");
diff --git a/src/util/child_common.c b/src/util/child_common.c
index 89cc07cbf26df7e5af59fc9054e83c4cd69501e0..56334c053e9008b895d625fc31a17ca44a0c0975 100644
--- a/src/util/child_common.c
+++ b/src/util/child_common.c
@@ -518,29 +518,6 @@ int read_pipe_recv(struct tevent_req *req, TALLOC_CTX *mem_ctx,
     return EOK;
 }
 
-/* The pipes to communicate with the child must be nonblocking */
-void fd_nonblocking(int fd)
-{
-    int flags;
-    int ret;
-
-    flags = fcntl(fd, F_GETFL, 0);
-    if (flags == -1) {
-        ret = errno;
-        DEBUG(SSSDBG_CRIT_FAILURE,
-              "F_GETFL failed [%d][%s].\n", ret, strerror(ret));
-        return;
-    }
-
-    if (fcntl(fd, F_SETFL, flags | O_NONBLOCK) == -1) {
-        ret = errno;
-        DEBUG(SSSDBG_CRIT_FAILURE,
-              "F_SETFL failed [%d][%s].\n", ret, strerror(ret));
-    }
-
-    return;
-}
-
 static void child_invoke_callback(struct tevent_context *ev,
                                   struct tevent_immediate *imm,
                                   void *pvt);
diff --git a/src/util/util.c b/src/util/util.c
index 2acb8604ac0c2bc7b83ee578c7bbead9a7fd44b3..613c559bb2002686c7833642d0946e46e5a9b5d6 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -880,3 +880,27 @@ done:
 
     return ret;
 }
+
+/* Set the nonblocking flag to the fd */
+errno_t sss_fd_nonblocking(int fd)
+{
+    int flags;
+    int ret;
+
+    flags = fcntl(fd, F_GETFL, 0);
+    if (flags == -1) {
+        ret = errno;
+        DEBUG(SSSDBG_CRIT_FAILURE,
+              "F_GETFL failed [%d][%s].\n", ret, strerror(ret));
+        return ret;
+    }
+
+    if (fcntl(fd, F_SETFL, flags | O_NONBLOCK) == -1) {
+        ret = errno;
+        DEBUG(SSSDBG_CRIT_FAILURE,
+              "F_SETFL failed [%d][%s].\n", ret, strerror(ret));
+        return ret;
+    }
+
+    return EOK;
+}
diff --git a/src/util/util.h b/src/util/util.h
index 45efd1aef94c2e058a435933e7c41adaecc676e2..60dbf9381c5fe26e7d42d51c6f9c7df6e3ebc4be 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -26,6 +26,7 @@
 #include <stdint.h>
 #include <stdbool.h>
 #include <unistd.h>
+#include <fcntl.h>
 #include <string.h>
 #include <strings.h>
 #include <ctype.h>
@@ -444,6 +445,17 @@ errno_t sss_hash_create_ex(TALLOC_CTX *mem_ctx,
 errno_t add_strings_lists(TALLOC_CTX *mem_ctx, const char **l1, const char **l2,
                           bool copy_strings, char ***_new_list);
 
+/**
+ * @brief set file descriptor as nonblocking
+ *
+ * Set the O_NONBLOCK flag for the input fd
+ *
+ * @param[in] fd            The file descriptor to set as nonblocking
+ *
+ * @return                  EOK on success, errno code otherwise
+ */
+errno_t sss_fd_nonblocking(int fd);
+
 /* Copy a NULL-terminated string list
  * Returns NULL on out of memory error or invalid input
  */
-- 
1.9.3



More information about the sssd-devel mailing list