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.
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.
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.
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.
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.
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.
Thanks.
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.
Thanks.
A new patch is attached.
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.
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.
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.
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.
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.
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.
On Tue, 2012-03-06 at 11:46 -0500, Stephen Gallagher wrote:
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.
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.
Pushed to master and sssd-1-8.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Stephen Gallagher