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.