On Tue, 2011-11-22 at 08:58 -0500, Stephen Gallagher wrote: > On Wed, 2011-11-09 at 15:31 +0100, Pavel Zuna wrote: > > Updated patch attached. > > > > Nack. > > Please make "struct sss_sigchild_ctx" an opaque structure and provide an > init routine to create the hash and set up the signal handler. This way > we only have to call > sss_sigchild_init(ctx, &ctx->sigchld_ctx); > in be_process_init() (or any other executable that needs child control). > > In sss_child_handler(), don't consider it fatal if the PID isn't found > in the hash. It could mean that something we're linked against did a > fork and was trying to maintain itself. This is bad, but not fatal. > Lower the debug level a bit (I think SSSDBG_OP_FAILURE is fine) and > allow the waitpid() loop to continue, rather than exiting immediately. > > sss_child_ctx should be moved to child_common.c. Its internals should > not be visible elsewhere. > > Please reorder sss_child_invoke_cb() to remove the entry from the hash > before invoking the callback (you cannot assume that the callback won't > do something funny to the hash structure or child_ctx, though it should > be safe). Also, please raise the debug level for this failure to > SSSDBG_OP_FAILURE.
I've taken over the development of this patch. I'm sending my changes to address the above issues except for a slight modification to my comments about the sss_child_handler loop - it was already ignoring unknown errors, but it was exiting on unexpected failures. I added a comment and will allow it to try to continue looping through in the hopes that it will minimize the potential zombies. This patch does not currently consume the functions that it creates. I am working on converting the existing child handlers to use this code, but I want to get this patch looked at and acked in the meantime.
From bfe098b42fb2e04d6e2d00302e9a7c77db5ad9f3 Mon Sep 17 00:00:00 2001
From: Pavel Zuna <pz...@redhat.com>
Date: Thu, 20 Oct 2011 13:26:18 -0400
Subject: [PATCH] Add common SIGCHLD handling for providers.
---
Makefile.am | 11 ++-
src/providers/child_common.c | 218 ++++++++++++++++++++++++++++++++++++-
src/providers/child_common.h | 25 +++++
src/providers/data_provider_be.c | 9 ++
src/providers/dp_backend.h | 2 +
5 files changed, 257 insertions(+), 8 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 8d75b8767bd30532f9a241ca1518545d602beffb..fe62a3eba6caff340e92b521a930406e603b593a 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -436,6 +436,7 @@ sssd_pam_LDADD = \
libsss_util.la
sssd_be_SOURCES = \
+ src/providers/child_common.c \
src/providers/data_provider_be.c \
src/providers/data_provider_fo.c \
src/providers/data_provider_opts.c \
@@ -984,7 +985,9 @@ krb5_child_SOURCES = \
src/providers/child_common.c \
src/providers/dp_pam_data_util.c \
src/util/user_info_msg.c \
- src/util/sss_krb5.c
+ src/util/sss_krb5.c \
+ src/util/util.c \
+ src/util/signal.c
krb5_child_CFLAGS = \
$(AM_CFLAGS) \
$(POPT_CFLAGS) \
@@ -994,12 +997,15 @@ krb5_child_LDADD = \
$(TALLOC_LIBS) \
$(TEVENT_LIBS) \
$(POPT_LIBS) \
+ $(DHASH_LIBS) \
$(KRB5_LIBS)
ldap_child_SOURCES = \
src/providers/ldap/ldap_child.c \
src/providers/child_common.c \
- src/util/sss_krb5.c
+ src/util/sss_krb5.c \
+ src/util/util.c \
+ src/util/signal.c
ldap_child_CFLAGS = \
$(AM_CFLAGS) \
$(POPT_CFLAGS) \
@@ -1010,6 +1016,7 @@ ldap_child_LDADD = \
$(TEVENT_LIBS) \
$(POPT_LIBS) \
$(OPENLDAP_LIBS) \
+ $(DHASH_LIBS) \
$(KRB5_LIBS)
proxy_child_SOURCES = \
diff --git a/src/providers/child_common.c b/src/providers/child_common.c
index 5920ebc7423ddce81dbf4d8f2767cab201852236..1ecde9d4b5c62a705a49154ff584b7324dad607b 100644
--- a/src/providers/child_common.c
+++ b/src/providers/child_common.c
@@ -33,7 +33,213 @@
#include "db/sysdb.h"
#include "providers/child_common.h"
+struct sss_sigchild_ctx {
+ struct tevent_context *ev;
+ hash_table_t *children;
+ int options;
+};
+
struct sss_child_ctx {
+ pid_t pid;
+ sss_child_fn_t cb;
+ void *pvt;
+ struct sss_sigchild_ctx *sigchld_ctx;
+};
+
+errno_t sss_sigchld_init(TALLOC_CTX *mem_ctx,
+ struct tevent_context *ev,
+ struct sss_sigchild_ctx **child_ctx)
+{
+ errno_t ret;
+ struct sss_sigchild_ctx *sigchld_ctx;
+ struct tevent_signal *tes;
+
+ sigchld_ctx = talloc_zero(mem_ctx, struct sss_sigchild_ctx);
+ if (!sigchld_ctx) {
+ DEBUG(0, ("fatal error initializing sss_sigchild_ctx\n"));
+ return ENOMEM;
+ }
+ sigchld_ctx->ev = ev;
+
+ ret = sss_hash_create(sigchld_ctx, 10, &sigchld_ctx->children);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_FATAL_FAILURE,
+ ("fatal error initializing children hash table: [%s]\n",
+ strerror(ret)));
+ talloc_free(sigchld_ctx);
+ return ret;
+ }
+
+ BlockSignals(false, SIGCHLD);
+ tes = tevent_add_signal(ev, sigchld_ctx, SIGCHLD, SA_SIGINFO,
+ sss_child_handler, sigchld_ctx);
+ if (tes == NULL) {
+ talloc_free(sigchld_ctx);
+ return EIO;
+ }
+
+ *child_ctx = sigchld_ctx;
+ return EOK;
+}
+
+static int sss_child_destructor(void *ptr)
+{
+ struct sss_child_ctx *child_ctx;
+ hash_key_t key;
+ int error;
+
+ child_ctx = talloc_get_type(ptr, struct sss_child_ctx);
+ key.type = HASH_KEY_ULONG;
+ key.ul = child_ctx->pid;
+
+ error = hash_delete(child_ctx->sigchld_ctx->children, &key);
+ if (error != HASH_SUCCESS && error != HASH_ERROR_KEY_NOT_FOUND) {
+ DEBUG(SSSDBG_TRACE_INTERNAL,
+ ("failed to delete child_ctx from hash table [%d]: %s\n",
+ error, hash_error_string(error)));
+ }
+
+ return 0;
+}
+
+errno_t sss_child_register(TALLOC_CTX *mem_ctx,
+ struct sss_sigchild_ctx *sigchld_ctx,
+ pid_t pid,
+ sss_child_fn_t cb,
+ void *pvt,
+ struct sss_child_ctx **child_ctx)
+{
+ struct sss_child_ctx *child;
+ hash_key_t key;
+ hash_value_t value;
+ int error;
+
+ child = talloc_zero(mem_ctx, struct sss_child_ctx);
+ if (child == NULL) {
+ return ENOMEM;
+ }
+
+ child->pid = pid;
+ child->cb = cb;
+ child->pvt = pvt;
+
+ key.type = HASH_KEY_ULONG;
+ key.ul = pid;
+
+ value.type = HASH_VALUE_PTR;
+ value.ptr = child;
+
+ error = hash_enter(sigchld_ctx->children, &key, &value);
+ if (error != HASH_SUCCESS) {
+ talloc_free(child);
+ return ENOMEM;
+ }
+
+ talloc_set_destructor((TALLOC_CTX *) child, sss_child_destructor);
+
+ *child_ctx = child;
+ return EOK;
+}
+
+struct sss_child_cb_pvt {
+ struct sss_child_ctx *child_ctx;
+ int wait_status;
+};
+
+static void sss_child_invoke_cb(struct tevent_context *ev,
+ struct tevent_immediate *imm,
+ void *pvt)
+{
+ struct sss_child_cb_pvt *cb_pvt;
+ struct sss_child_ctx *child_ctx;
+ hash_key_t key;
+ int error;
+
+ cb_pvt = talloc_get_type(pvt, struct sss_child_cb_pvt);
+ child_ctx = cb_pvt->child_ctx;
+
+ if (child_ctx->cb) {
+ child_ctx->cb(child_ctx->pid, cb_pvt->wait_status, child_ctx->pvt);
+ }
+
+ key.type = HASH_KEY_ULONG;
+ key.ul = child_ctx->pid;
+
+ error = hash_delete(child_ctx->sigchld_ctx->children, &key);
+ if (error != HASH_SUCCESS && error != HASH_ERROR_KEY_NOT_FOUND) {
+ DEBUG(SSSDBG_TRACE_INTERNAL,
+ ("failed to delete child_ctx from hash table [%d]: %s\n",
+ error, hash_error_string(error)));
+ }
+}
+
+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;
+ struct sss_child_cb_pvt *invoke_pvt;
+ struct sss_child_ctx *child_ctx;
+ hash_key_t key;
+ hash_value_t value;
+ int error;
+ int wait_status;
+ pid_t pid;
+
+ sigchld_ctx = talloc_get_type(private_data, struct sss_sigchild_ctx);
+ key.type = HASH_KEY_ULONG;
+
+ do {
+ do {
+ errno = 0;
+ pid = waitpid(-1, &wait_status, WNOHANG & sigchld_ctx->options);
+ } while (pid == -1 && errno == EINTR);
+
+ if (pid == -1) {
+ DEBUG(SSSDBG_TRACE_INTERNAL,
+ ("waitpid failed [%d]: %s\n", errno, strerror(errno)));
+ return;
+ }
+
+ key.ul = pid;
+ error = hash_lookup(sigchld_ctx->children, &key, &value);
+ if (error == HASH_SUCCESS) {
+ child_ctx = talloc_get_type(value.ptr, struct sss_child_ctx);
+
+ imm = tevent_create_immediate(sigchld_ctx->ev);
+ if (imm == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("Out of memory invoking SIGCHLD callback\n"));
+ return;
+ }
+
+ invoke_pvt = talloc_zero(child_ctx, struct sss_child_cb_pvt);
+ if (invoke_pvt == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("out of memory invoking SIGCHLD callback\n"));
+ return;
+ }
+ invoke_pvt->child_ctx = child_ctx;
+ invoke_pvt->wait_status = wait_status;
+
+ tevent_schedule_immediate(imm, sigchld_ctx->ev,
+ sss_child_invoke_cb, invoke_pvt);
+ } else if (error != HASH_ERROR_KEY_NOT_FOUND) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("SIGCHLD hash table error [%d]: %s\n",
+ error, hash_error_string(error)));
+ /* This is bad, but we should try to check for other
+ * children anyway, to avoid potential zombies.
+ */
+ }
+ } while (pid != 0);
+}
+
+struct sss_child_ctx_old {
struct tevent_signal *sige;
pid_t pid;
int child_status;
@@ -44,11 +250,11 @@ struct sss_child_ctx {
int child_handler_setup(struct tevent_context *ev, int pid,
sss_child_callback_t cb, void *pvt)
{
- struct sss_child_ctx *child_ctx;
+ struct sss_child_ctx_old *child_ctx;
DEBUG(8, ("Setting up signal handler up for pid [%d]\n", pid));
- child_ctx = talloc_zero(ev, struct sss_child_ctx);
+ child_ctx = talloc_zero(ev, struct sss_child_ctx_old);
if (child_ctx == NULL) {
return ENOMEM;
}
@@ -300,7 +506,7 @@ void child_sig_handler(struct tevent_context *ev,
int count, void *__siginfo, void *pvt)
{
int ret, err;
- struct sss_child_ctx *child_ctx;
+ struct sss_child_ctx_old *child_ctx;
struct tevent_immediate *imm;
if (count <= 0) {
@@ -308,7 +514,7 @@ void child_sig_handler(struct tevent_context *ev,
return;
}
- child_ctx = talloc_get_type(pvt, struct sss_child_ctx);
+ child_ctx = talloc_get_type(pvt, struct sss_child_ctx_old);
DEBUG(7, ("Waiting for child [%d].\n", child_ctx->pid));
errno = 0;
@@ -363,8 +569,8 @@ static void child_invoke_callback(struct tevent_context *ev,
struct tevent_immediate *imm,
void *pvt)
{
- struct sss_child_ctx *child_ctx =
- talloc_get_type(pvt, struct sss_child_ctx);
+ struct sss_child_ctx_old *child_ctx =
+ talloc_get_type(pvt, struct sss_child_ctx_old);
if (child_ctx->cb) {
child_ctx->cb(child_ctx->child_status, child_ctx->sige, child_ctx->pvt);
}
diff --git a/src/providers/child_common.h b/src/providers/child_common.h
index 22a77dbbe28c385c7c0e49ce8a339a68028b3658..1e9f1b6c152686ba75d5207ba48f0f131a4a2fff 100644
--- a/src/providers/child_common.h
+++ b/src/providers/child_common.h
@@ -45,6 +45,31 @@ struct io_buffer {
size_t size;
};
+/* COMMON SIGCHLD HANDLING */
+typedef void (*sss_child_fn_t)(int pid, int wait_status, void *pvt);
+
+struct sss_sigchild_ctx;
+struct sss_child_ctx;
+
+/* Create a new child context to manage callbacks */
+errno_t sss_sigchld_init(TALLOC_CTX *mem_ctx,
+ struct tevent_context *ev,
+ struct sss_sigchild_ctx **child_ctx);
+
+errno_t sss_child_register(TALLOC_CTX *mem_ctx,
+ struct sss_sigchild_ctx *sigchld_ctx,
+ pid_t pid,
+ sss_child_fn_t cb,
+ 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.
diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
index 6557f9bca25865e34afea5aebc71954587f50977..4910aa6fc50429bf2a6367ce9ffa90e5689d5c91 100644
--- a/src/providers/data_provider_be.c
+++ b/src/providers/data_provider_be.c
@@ -42,6 +42,7 @@
#include "sbus/sssd_dbus.h"
#include "providers/dp_backend.h"
#include "providers/fail_over.h"
+#include "providers/child_common.h"
#include "resolv/async_resolv.h"
#include "monitor/monitor_interfaces.h"
@@ -1176,6 +1177,14 @@ int be_process_init(TALLOC_CTX *mem_ctx,
return EIO;
}
+ ret = sss_sigchld_init(ctx, ctx->ev, &ctx->sigchld_ctx);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_FATAL_FAILURE,
+ ("Could not initialize sigchld context: [%s]\n",
+ strerror(ret)));
+ return ret;
+ }
+
return EOK;
}
diff --git a/src/providers/dp_backend.h b/src/providers/dp_backend.h
index 3d5e40bae43b326c00ce9464abfca1e5918ced01..0c24b82c9687b75a69f1559a16647fb0fb86c00d 100644
--- a/src/providers/dp_backend.h
+++ b/src/providers/dp_backend.h
@@ -24,6 +24,7 @@
#include "providers/data_provider.h"
#include "providers/fail_over.h"
+#include "providers/child_common.h"
#include "db/sysdb.h"
/* a special token, if used in place of the hostname, denotes that real
@@ -93,6 +94,7 @@ struct be_ctx {
const char *identity;
const char *conf_path;
struct be_failover_ctx *be_fo;
+ struct sss_sigchild_ctx *sigchld_ctx;
/* Functions to be invoked when the
* backend goes online or offline
--
1.7.7.3
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
