On Fri, Mar 02, 2012 at 08:25:05AM -0500, Stephen Gallagher wrote:
> On Fri, 2012-03-02 at 14:16 +0100, Jakub Hrozek wrote:
> > Both krb5_child and ldap_child would emit a "child started" message and
> > only after that set up debugging to file. This might confuse users,
> > because unless there is an error, the krb5_child.log might actually be
> > empty.
>
> Nack. "[sssd[krb5_child[%d]]]", getpid() isn't dependent on anything
> that you don't know at this point. Just talloc_asprintf() it on NULL and
> then steal it onto pd later.
>
> Also, please add NULL-checks for the talloc_asprintf() calls. If they
> return NULL, just assign a static string "[sssd[ldap_child]]" or
> "[sssd[krb5_child]]" without the PID.
>
OK, new patch is attached. We won't be able to free debug_prg_name if
talloc_zero fails later, but that's not a big deal, the child process is
not a long-running one.
> >
> > I'm thinking we might also add a couple of "tracing" DEBUG messages so
> > that we can follow the flow in the subprocess more easily.
>
> Please open an RFE.
https://fedorahosted.org/sssd/ticket/1225
From ec30988eca461d4ead6b9f461ccdc0086fd95519 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Thu, 1 Mar 2012 16:41:14 +0100
Subject: [PATCH] krb5_child: set debugging sooner
---
src/providers/krb5/krb5_child.c | 24 ++++++++++++++----------
src/providers/ldap/ldap_child.c | 24 ++++++++++++++----------
2 files changed, 28 insertions(+), 20 deletions(-)
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index
cc185260ef95c275a97e7f21bddb0580bf6a36a5..5829c908621f13460d92b52e270889f4e202fe3a
100644
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -1582,7 +1582,7 @@ int main(int argc, const char *argv[])
POPT_TABLEEND
};
- /* Set debug level to invalid value so we can deside if -d 0 was used. */
+ /* Set debug level to invalid value so we can decide if -d 0 was used. */
debug_level = SSSDBG_INVALID;
pc = poptGetContext(argv[0], argc, argv, long_options, 0);
@@ -1600,23 +1600,27 @@ int main(int argc, const char *argv[])
CONVERT_AND_SET_DEBUG_LEVEL(debug_level);
- DEBUG(7, ("krb5_child started.\n"));
-
- pd = talloc_zero(NULL, struct pam_data);
- if (pd == NULL) {
- DEBUG(1, ("malloc failed.\n"));
- _exit(-1);
+ debug_prg_name = talloc_asprintf(NULL, "[sssd[krb5_child[%d]]]", getpid());
+ if (!debug_prg_name) {
+ debug_prg_name = "[sssd[krb5_child]]";
}
- debug_prg_name = talloc_asprintf(pd, "[sssd[krb5_child[%d]]]", getpid());
-
if (debug_fd != -1) {
ret = set_debug_file_from_fd(debug_fd);
if (ret != EOK) {
- DEBUG(1, ("set_debug_file_from_fd failed.\n"));
+ DEBUG(SSSDBG_CRIT_FAILURE, ("set_debug_file_from_fd failed.\n"));
}
}
+ DEBUG(SSSDBG_TRACE_FUNC, ("krb5_child started.\n"));
+
+ pd = talloc_zero(NULL, struct pam_data);
+ if (pd == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, ("malloc failed.\n"));
+ _exit(-1);
+ }
+ talloc_steal(pd, debug_prg_name);
+
buf = talloc_size(pd, sizeof(uint8_t)*IN_BUF_SIZE);
if (buf == NULL) {
DEBUG(1, ("malloc failed.\n"));
diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c
index
66ceb14e3d73d5af448029ef226bd97001fa008f..6c1646b5a28225520732e3057fd056f10427c700
100644
--- a/src/providers/ldap/ldap_child.c
+++ b/src/providers/ldap/ldap_child.c
@@ -388,7 +388,7 @@ int main(int argc, const char *argv[])
POPT_TABLEEND
};
- /* Set debug level to invalid value so we can deside if -d 0 was used. */
+ /* Set debug level to invalid value so we can decide if -d 0 was used. */
debug_level = SSSDBG_INVALID;
pc = poptGetContext(argv[0], argc, argv, long_options, 0);
@@ -406,23 +406,27 @@ int main(int argc, const char *argv[])
CONVERT_AND_SET_DEBUG_LEVEL(debug_level);
- DEBUG(7, ("ldap_child started.\n"));
-
- main_ctx = talloc_new(NULL);
- if (main_ctx == NULL) {
- DEBUG(1, ("talloc_new failed.\n"));
- _exit(-1);
+ debug_prg_name = talloc_asprintf(NULL, "[sssd[ldap_child[%d]]]", getpid());
+ if (!debug_prg_name) {
+ debug_prg_name = "[sssd[ldap_child]]";
}
- debug_prg_name = talloc_asprintf(main_ctx, "[sssd[ldap_child[%d]]]",
getpid());
-
if (debug_fd != -1) {
ret = set_debug_file_from_fd(debug_fd);
if (ret != EOK) {
- DEBUG(1, ("set_debug_file_from_fd failed.\n"));
+ DEBUG(SSSDBG_CRIT_FAILURE, ("set_debug_file_from_fd failed.\n"));
}
}
+ DEBUG(SSSDBG_TRACE_FUNC, ("ldap_child started.\n"));
+
+ main_ctx = talloc_new(NULL);
+ if (main_ctx == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_new failed.\n"));
+ _exit(-1);
+ }
+ talloc_steal(main_ctx, debug_prg_name);
+
buf = talloc_size(main_ctx, sizeof(uint8_t)*IN_BUF_SIZE);
if (buf == NULL) {
DEBUG(1, ("talloc_size failed.\n"));
--
1.7.7.6
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
