On Tue, 2012-03-06 at 17:36 +0100, Jakub Hrozek wrote:
> On Mon, Mar 05, 2012 at 08:42:42AM -0500, Stephen Gallagher wrote:
> > On Mon, 2012-03-05 at 13:08 +0100, Jakub Hrozek wrote:
> > > On Fri, Mar 02, 2012 at 09:39:18AM -0500, Stephen Gallagher wrote:
> > > > On Fri, 2012-03-02 at 15:25 +0100, Jakub Hrozek wrote:
> > > > > 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.
> > > > >
> > > >
> > > > Hmm, that's a good point. Coverity and valgrind will likely
complain
> > > > about the leak as well. On further thought, it's probably alright
to
> > > > just fail the krb5_child if that asprintf doesn't work, because
if we're
> > > > in that serious of an OOM situation, chances are high that other,
more
> > > > important things will be failing anyway.
> > > >
> > > > So let's do that. Sorry for the repeated revisions.
> > > >
> > > > > > >
> > > > > > > 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
> > > >
> > > > Thanks.
> > >
> > > A new patch is attached.
> >
> > Nack. With these new patches, you may jump to "done" before main_ctx
has
> > been set. You need to initialize main_ctx to NULL.
> >
> > Also, I'd prefer if we went to "done" instead of calling
_exit(-1); if
> > the allocation of main_ctx fails.
>
> Thank you, yet another patch attached.
Thanks for your patience; it has been rewarded.
Ack for master and sssd-1-8.