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.