8:28 a.m.
Base on the second proposal:
https://fedorahosted.org/sssd/wiki/DesignDocs/SigChld
There is some old SIGCHLD handling code in src/providers/child_common.[ch], that
should probably go away if this gets accepted. There was also a naming conflict
with the sss_child_ctx structure. This structure is only used internally by
functions defined src/providers/child_common.c. I renamed the original structure
to sss_child_ctx_old for now.
Pavel
11:44 a.m.
On Fri, 2011-10-21 at 15:28 +0200, Pavel Zuna wrote:
Base on the second proposal:
https://fedorahosted.org/sssd/wiki/DesignDocs/SigChld
There is some old SIGCHLD handling code in src/providers/child_common.[ch], that
should probably go away if this gets accepted. There was also a naming conflict
with the sss_child_ctx structure. This structure is only used internally by
functions defined src/providers/child_common.c. I renamed the original structure
to sss_child_ctx_old for now.
Nack (but a good start)
sss_child_destructor() cannot return the result of hash_delete(). Talloc
destructors may only return 0 on success or -1 on failure. However,
because destructor failure results in cancelling the free(), we should
just log an error here and return 0.
sss_child_register:
Don't initialize sss_child_ctx *tmp to the child_ctx passed in. We want
to leave child_ctx untouched until successful completion. Also, please
don't use the variable name "tmp". It's not descriptive. Call it
"child".
Instead of allocating *child_ctx at the beginning, allocate "child" on
the mem_ctx.
child = talloc_zero(mem_ctx, struct sss_child_ctx);
Then, only at the end of execution, right before returning EOK, do
*child_ctx = child;
sss_child_invoke_cb():
I'm wary of having the callback invocation free the child_ctx. It needs
to be documented well (so that consumers know they must not free it once
the callback has fired or they'll get a double-free).
sss_child_handler():
You need to check whether waitpid() returned an error and handle that
appropriately. waitpid() will return -1 on error and set errno. If the
errno value is EINTR, you should retry (because waitpid() was
interrupted by a signal at the time). Other errors should generate a
DEBUG message.
Please separate any changes you make to the existing sss_child_ctx into
its own patch. I don't like just renaming it to _old, either. Please
find a more appropriate way to replace this old object (preferably by
converting it to use your new mechanism, also in a new patch).
6:08 a.m.
On 10/21/2011 06:44 PM, Stephen Gallagher wrote:
On Fri, 2011-10-21 at 15:28 +0200, Pavel Zuna wrote:
> Base on the second proposal:
>
> https://fedorahosted.org/sssd/wiki/DesignDocs/SigChld
>
> There is some old SIGCHLD handling code in src/providers/child_common.[ch], that
> should probably go away if this gets accepted. There was also a naming conflict
> with the sss_child_ctx structure. This structure is only used internally by
> functions defined src/providers/child_common.c. I renamed the original structure
> to sss_child_ctx_old for now.
Nack (but a good start)
sss_child_destructor() cannot return the result of hash_delete(). Talloc
destructors may only return 0 on success or -1 on failure. However,
because destructor failure results in cancelling the free(), we should
just log an error here and return 0.
sss_child_register:
Don't initialize sss_child_ctx *tmp to the child_ctx passed in. We want
to leave child_ctx untouched until successful completion. Also, please
don't use the variable name "tmp". It's not descriptive. Call it
"child".
Instead of allocating *child_ctx at the beginning, allocate "child" on
the mem_ctx.
child = talloc_zero(mem_ctx, struct sss_child_ctx);
Then, only at the end of execution, right before returning EOK, do
*child_ctx = child;
sss_child_invoke_cb():
I'm wary of having the callback invocation free the child_ctx. It needs
to be documented well (so that consumers know they must not free it once
the callback has fired or they'll get a double-free).
sss_child_handler():
You need to check whether waitpid() returned an error and handle that
appropriately. waitpid() will return -1 on error and set errno. If the
errno value is EINTR, you should retry (because waitpid() was
interrupted by a signal at the time). Other errors should generate a
DEBUG message.
Ok, I'm done with all of this. It was pretty straight forward, but...
Please separate any changes you make to the existing sss_child_ctx into
its own patch. I don't like just renaming it to _old, either. Please
find a more appropriate way to replace this old object (preferably by
converting it to use your new mechanism, also in a new patch).
... this, I'm trying to find a way to make the old code use the new mechanism. I
think it would be better to remove the old one completely and replace it with
the new one everywhere where it's used.
Pavel
7:03 a.m.
On 10/25/2011 01:08 PM, Pavel Zuna wrote:
On 10/21/2011 06:44 PM, Stephen Gallagher wrote:
> On Fri, 2011-10-21 at 15:28 +0200, Pavel Zuna wrote:
>> Base on the second proposal:
>>
>> https://fedorahosted.org/sssd/wiki/DesignDocs/SigChld
>>
>> There is some old SIGCHLD handling code in src/providers/child_common.[ch], that
>> should probably go away if this gets accepted. There was also a naming conflict
>> with the sss_child_ctx structure. This structure is only used internally by
>> functions defined src/providers/child_common.c. I renamed the original structure
>> to sss_child_ctx_old for now.
>
>
> Nack (but a good start)
>
> sss_child_destructor() cannot return the result of hash_delete(). Talloc
> destructors may only return 0 on success or -1 on failure. However,
> because destructor failure results in cancelling the free(), we should
> just log an error here and return 0.
>
> sss_child_register:
> Don't initialize sss_child_ctx *tmp to the child_ctx passed in. We want
> to leave child_ctx untouched until successful completion. Also, please
> don't use the variable name "tmp". It's not descriptive. Call it
> "child".
>
> Instead of allocating *child_ctx at the beginning, allocate "child" on
> the mem_ctx.
>
> child = talloc_zero(mem_ctx, struct sss_child_ctx);
>
> Then, only at the end of execution, right before returning EOK, do
> *child_ctx = child;
>
>
> sss_child_invoke_cb():
> I'm wary of having the callback invocation free the child_ctx. It needs
> to be documented well (so that consumers know they must not free it once
> the callback has fired or they'll get a double-free).
>
> sss_child_handler():
> You need to check whether waitpid() returned an error and handle that
> appropriately. waitpid() will return -1 on error and set errno. If the
> errno value is EINTR, you should retry (because waitpid() was
> interrupted by a signal at the time). Other errors should generate a
> DEBUG message.
>
Ok, I'm done with all of this. It was pretty straight forward, but...
>
> Please separate any changes you make to the existing sss_child_ctx into
> its own patch. I don't like just renaming it to _old, either. Please
> find a more appropriate way to replace this old object (preferably by
> converting it to use your new mechanism, also in a new patch).
>
... this, I'm trying to find a way to make the old code use the new mechanism. I
think it would be better to remove the old one completely and replace it with
the new one everywhere where it's used.
Pavel
New patch with the above mentioned issues fixed is attached to this email.
We agreed that we're going to replace all uses of the old system by the new.
Expect about 3 patches to follow on this one - one for each provider using the
old system.
Pavel
10:29 a.m.
On Thu, Oct 27, 2011 at 02:03:25PM +0200, Pavel Zuna wrote:
On 10/25/2011 01:08 PM, Pavel Zuna wrote:
>On 10/21/2011 06:44 PM, Stephen Gallagher wrote:
>>On Fri, 2011-10-21 at 15:28 +0200, Pavel Zuna wrote:
>>>Base on the second proposal:
>>>
>>>https://fedorahosted.org/sssd/wiki/DesignDocs/SigChld
>>>
>>>There is some old SIGCHLD handling code in src/providers/child_common.[ch],
that
>>>should probably go away if this gets accepted. There was also a naming
conflict
>>>with the sss_child_ctx structure. This structure is only used internally by
>>>functions defined src/providers/child_common.c. I renamed the original
structure
>>>to sss_child_ctx_old for now.
>>
>>
>>Nack (but a good start)
>>
>>sss_child_destructor() cannot return the result of hash_delete(). Talloc
>>destructors may only return 0 on success or -1 on failure. However,
>>because destructor failure results in cancelling the free(), we should
>>just log an error here and return 0.
>>
>>sss_child_register:
>>Don't initialize sss_child_ctx *tmp to the child_ctx passed in. We want
>>to leave child_ctx untouched until successful completion. Also, please
>>don't use the variable name "tmp". It's not descriptive. Call
it
>>"child".
>>
>>Instead of allocating *child_ctx at the beginning, allocate "child" on
>>the mem_ctx.
>>
>>child = talloc_zero(mem_ctx, struct sss_child_ctx);
>>
>>Then, only at the end of execution, right before returning EOK, do
>>*child_ctx = child;
>>
In sss_child_register():
+ error = hash_enter(sigchld_ctx->children, &key, &value);
+ if (error != HASH_SUCCESS && error != HASH_ERROR_KEY_NOT_FOUND) {
I don't think hash_enter can return HASH_ERROR_KEY_NOT_FOUND (what would
it mean?) and even if it did, why ignore it?
>>
>>sss_child_invoke_cb():
>>I'm wary of having the callback invocation free the child_ctx. It needs
>>to be documented well (so that consumers know they must not free it once
>>the callback has fired or they'll get a double-free).
This still needs fixing.
>>
>>sss_child_handler():
>>You need to check whether waitpid() returned an error and handle that
>>appropriately. waitpid() will return -1 on error and set errno. If the
>>errno value is EINTR, you should retry (because waitpid() was
>>interrupted by a signal at the time). Other errors should generate a
>>DEBUG message.
I think you misunderstood how the error handling should be done:
+ if (pid == -1) {
+ if (errno == ECHILD) {
+ DEBUG(0, ("waitpid failed with ECHILD\n"));
+ } else if (errno == EINVAL) {
+ DEBUG(0, ("waitpid failed with EINVAL\n"));
+ }
+ return;
+ }
Instead of having a DEBUG() call per retval, it would be better to inform user
on why the child process exited, similarly to how child_sig_handler() does it
now.
>>
>
>Ok, I'm done with all of this. It was pretty straight forward, but...
>
>>
>>Please separate any changes you make to the existing sss_child_ctx into
>>its own patch. I don't like just renaming it to _old, either. Please
>>find a more appropriate way to replace this old object (preferably by
>>converting it to use your new mechanism, also in a new patch).
>>
>
>... this, I'm trying to find a way to make the old code use the new mechanism. I
>think it would be better to remove the old one completely and replace it with
>the new one everywhere where it's used.
>
>Pavel
New patch with the above mentioned issues fixed is attached to this email.
We agreed that we're going to replace all uses of the old system by
the new. Expect about 3 patches to follow on this one - one for each
provider using the old system.
Pavel
5:34 a.m.
On 10/31/2011 04:29 PM, Jakub Hrozek wrote:
On Thu, Oct 27, 2011 at 02:03:25PM +0200, Pavel Zuna wrote:
> On 10/25/2011 01:08 PM, Pavel Zuna wrote:
>> On 10/21/2011 06:44 PM, Stephen Gallagher wrote:
>>> On Fri, 2011-10-21 at 15:28 +0200, Pavel Zuna wrote:
>>>> Base on the second proposal:
>>>>
>>>> https://fedorahosted.org/sssd/wiki/DesignDocs/SigChld
>>>>
>>>> There is some old SIGCHLD handling code in
src/providers/child_common.[ch], that
>>>> should probably go away if this gets accepted. There was also a naming
conflict
>>>> with the sss_child_ctx structure. This structure is only used internally
by
>>>> functions defined src/providers/child_common.c. I renamed the original
structure
>>>> to sss_child_ctx_old for now.
>>>
>>>
>>> Nack (but a good start)
>>>
>>> sss_child_destructor() cannot return the result of hash_delete(). Talloc
>>> destructors may only return 0 on success or -1 on failure. However,
>>> because destructor failure results in cancelling the free(), we should
>>> just log an error here and return 0.
>>>
>>> sss_child_register:
>>> Don't initialize sss_child_ctx *tmp to the child_ctx passed in. We want
>>> to leave child_ctx untouched until successful completion. Also, please
>>> don't use the variable name "tmp". It's not descriptive.
Call it
>>> "child".
>>>
>>> Instead of allocating *child_ctx at the beginning, allocate "child"
on
>>> the mem_ctx.
>>>
>>> child = talloc_zero(mem_ctx, struct sss_child_ctx);
>>>
>>> Then, only at the end of execution, right before returning EOK, do
>>> *child_ctx = child;
>>>
In sss_child_register():
+ error = hash_enter(sigchld_ctx->children,&key,&value);
+ if (error != HASH_SUCCESS&& error != HASH_ERROR_KEY_NOT_FOUND) {
I don't think hash_enter can return HASH_ERROR_KEY_NOT_FOUND (what would
it mean?) and even if it did, why ignore it?
>>>
>>> sss_child_invoke_cb():
>>> I'm wary of having the callback invocation free the child_ctx. It needs
>>> to be documented well (so that consumers know they must not free it once
>>> the callback has fired or they'll get a double-free).
This still needs fixing.
Forgot about that, sorry. Replaced the talloc_free with removing child_ctx from
the hash table only.
>>>
>>> sss_child_handler():
>>> You need to check whether waitpid() returned an error and handle that
>>> appropriately. waitpid() will return -1 on error and set errno. If the
>>> errno value is EINTR, you should retry (because waitpid() was
>>> interrupted by a signal at the time). Other errors should generate a
>>> DEBUG message.
I think you misunderstood how the error handling should be done:
+ if (pid == -1) {
+ if (errno == ECHILD) {
+ DEBUG(0, ("waitpid failed with ECHILD\n"));
+ } else if (errno == EINVAL) {
+ DEBUG(0, ("waitpid failed with EINVAL\n"));
+ }
+ return;
+ }
Instead of having a DEBUG() call per retval, it would be better to inform user
on why the child process exited, similarly to how child_sig_handler() does it
now.
I don't agree. The system is designed to inform providers that a child has
exited by invoking their callbacks. It's up to the provider to decide what to do
with the result and inform the user if appropriate. This would also make sense
only in the case of a successful call (pid >= 0).
In case of EINTR, we need to repeat the call to waitpid, because it was
interrupted by a signal. In other cases (ECHILD and EINVAL) the call is going to
always fail, so there's won't be any callbacks to invoke.
>>>
>>
>> Ok, I'm done with all of this. It was pretty straight forward, but...
>>
>>>
>>> Please separate any changes you make to the existing sss_child_ctx into
>>> its own patch. I don't like just renaming it to _old, either. Please
>>> find a more appropriate way to replace this old object (preferably by
>>> converting it to use your new mechanism, also in a new patch).
>>>
>>
>> ... this, I'm trying to find a way to make the old code use the new
mechanism. I
>> think it would be better to remove the old one completely and replace it with
>> the new one everywhere where it's used.
>>
>> Pavel
>
> New patch with the above mentioned issues fixed is attached to this email.
>
> We agreed that we're going to replace all uses of the old system by
> the new. Expect about 3 patches to follow on this one - one for each
> provider using the old system.
>
> Pavel
Thanks for taking the time to review this. New patch attached.
Pavel
8:58 a.m.
On Tue, Nov 01, 2011 at 11:34:27AM +0100, Pavel Zuna wrote:
On 10/31/2011 04:29 PM, Jakub Hrozek wrote:
>On Thu, Oct 27, 2011 at 02:03:25PM +0200, Pavel Zuna wrote:
>>On 10/25/2011 01:08 PM, Pavel Zuna wrote:
>>>On 10/21/2011 06:44 PM, Stephen Gallagher wrote:
>>>>On Fri, 2011-10-21 at 15:28 +0200, Pavel Zuna wrote:
>>>>>Base on the second proposal:
>>>>>
>>>>>https://fedorahosted.org/sssd/wiki/DesignDocs/SigChld
>>>>>
>>>>>There is some old SIGCHLD handling code in
src/providers/child_common.[ch], that
>>>>>should probably go away if this gets accepted. There was also a
naming conflict
>>>>>with the sss_child_ctx structure. This structure is only used
internally by
>>>>>functions defined src/providers/child_common.c. I renamed the
original structure
>>>>>to sss_child_ctx_old for now.
>>>>
>>>>
>>>>Nack (but a good start)
>>>>
>>>>sss_child_destructor() cannot return the result of hash_delete(). Talloc
>>>>destructors may only return 0 on success or -1 on failure. However,
>>>>because destructor failure results in cancelling the free(), we should
>>>>just log an error here and return 0.
>>>>
>>>>sss_child_register:
>>>>Don't initialize sss_child_ctx *tmp to the child_ctx passed in. We
want
>>>>to leave child_ctx untouched until successful completion. Also, please
>>>>don't use the variable name "tmp". It's not
descriptive. Call it
>>>>"child".
>>>>
>>>>Instead of allocating *child_ctx at the beginning, allocate
"child" on
>>>>the mem_ctx.
>>>>
>>>>child = talloc_zero(mem_ctx, struct sss_child_ctx);
>>>>
>>>>Then, only at the end of execution, right before returning EOK, do
>>>>*child_ctx = child;
>>>>
>
>In sss_child_register():
>
>+ error = hash_enter(sigchld_ctx->children,&key,&value);
>+ if (error != HASH_SUCCESS&& error != HASH_ERROR_KEY_NOT_FOUND) {
>
>I don't think hash_enter can return HASH_ERROR_KEY_NOT_FOUND (what would
>it mean?) and even if it did, why ignore it?
>
>>>>
>>>>sss_child_invoke_cb():
>>>>I'm wary of having the callback invocation free the child_ctx. It
needs
>>>>to be documented well (so that consumers know they must not free it once
>>>>the callback has fired or they'll get a double-free).
>
>This still needs fixing.
>
Forgot about that, sorry. Replaced the talloc_free with removing
child_ctx from the hash table only.
>>>>
>>>>sss_child_handler():
>>>>You need to check whether waitpid() returned an error and handle that
>>>>appropriately. waitpid() will return -1 on error and set errno. If the
>>>>errno value is EINTR, you should retry (because waitpid() was
>>>>interrupted by a signal at the time). Other errors should generate a
>>>>DEBUG message.
>
>I think you misunderstood how the error handling should be done:
>
>+ if (pid == -1) {
>+ if (errno == ECHILD) {
>+ DEBUG(0, ("waitpid failed with ECHILD\n"));
>+ } else if (errno == EINVAL) {
>+ DEBUG(0, ("waitpid failed with EINVAL\n"));
>+ }
>+ return;
>+ }
>
>Instead of having a DEBUG() call per retval, it would be better to inform user
>on why the child process exited, similarly to how child_sig_handler() does it
>now.
>
I don't agree. The system is designed to inform providers that a
child has exited by invoking their callbacks. It's up to the
provider to decide what to do with the result and inform the user if
appropriate. This would also make sense only in the case of a
successful call (pid >= 0).
OK, then why the separate branches for ECHILD and EINVAL? Isn't it
easier to just print errno and strerror(errno) ?
In case of EINTR, we need to repeat the call to waitpid, because it
was interrupted by a signal. In other cases (ECHILD and EINVAL) the
call is going to always fail, so there's won't be any callbacks to
invoke.
Yup.
>>>>
>>>
>>>Ok, I'm done with all of this. It was pretty straight forward, but...
>>>
>>>>
>>>>Please separate any changes you make to the existing sss_child_ctx into
>>>>its own patch. I don't like just renaming it to _old, either. Please
>>>>find a more appropriate way to replace this old object (preferably by
>>>>converting it to use your new mechanism, also in a new patch).
>>>>
>>>
>>>... this, I'm trying to find a way to make the old code use the new
mechanism. I
>>>think it would be better to remove the old one completely and replace it
with
>>>the new one everywhere where it's used.
>>>
>>>Pavel
>>
>>New patch with the above mentioned issues fixed is attached to this email.
>>
>>We agreed that we're going to replace all uses of the old system by
>>the new. Expect about 3 patches to follow on this one - one for each
>>provider using the old system.
>>
>>Pavel
>
Thanks for taking the time to review this. New patch attached.
Pavel
Two more issues (sorry for not catching all this at once):
Please don't just use DEBUG(0) for all the messages. DEBUG(0) is
supposed to be used for fatal errors that cause the provider to quit
completely.
See https://fedorahosted.org/pipermail/sssd-devel/2011-June/006407.html
for explanation of the log levels. In general, this patch is going to be
included in 1.7 and onwards -- please use the new debug levels (see
src/util/util.h) defined as macros instead of integers.
The second issue is that I'm getting a segfault when logging in via
Kerberos, the backtrace is pointing to tevent_common_check_signal().
8:31 a.m.
On 11/01/2011 02:58 PM, Jakub Hrozek wrote:
On Tue, Nov 01, 2011 at 11:34:27AM +0100, Pavel Zuna wrote:
> On 10/31/2011 04:29 PM, Jakub Hrozek wrote:
>> On Thu, Oct 27, 2011 at 02:03:25PM +0200, Pavel Zuna wrote:
>>> On 10/25/2011 01:08 PM, Pavel Zuna wrote:
>>>> On 10/21/2011 06:44 PM, Stephen Gallagher wrote:
>>>>> On Fri, 2011-10-21 at 15:28 +0200, Pavel Zuna wrote:
>>>>>> Base on the second proposal:
>>>>>>
>>>>>> https://fedorahosted.org/sssd/wiki/DesignDocs/SigChld
>>>>>>
>>>>>> There is some old SIGCHLD handling code in
src/providers/child_common.[ch], that
>>>>>> should probably go away if this gets accepted. There was also a
naming conflict
>>>>>> with the sss_child_ctx structure. This structure is only used
internally by
>>>>>> functions defined src/providers/child_common.c. I renamed the
original structure
>>>>>> to sss_child_ctx_old for now.
>>>>>
>>>>>
>>>>> Nack (but a good start)
>>>>>
>>>>> sss_child_destructor() cannot return the result of hash_delete().
Talloc
>>>>> destructors may only return 0 on success or -1 on failure. However,
>>>>> because destructor failure results in cancelling the free(), we
should
>>>>> just log an error here and return 0.
>>>>>
>>>>> sss_child_register:
>>>>> Don't initialize sss_child_ctx *tmp to the child_ctx passed in.
We want
>>>>> to leave child_ctx untouched until successful completion. Also,
please
>>>>> don't use the variable name "tmp". It's not
descriptive. Call it
>>>>> "child".
>>>>>
>>>>> Instead of allocating *child_ctx at the beginning, allocate
"child" on
>>>>> the mem_ctx.
>>>>>
>>>>> child = talloc_zero(mem_ctx, struct sss_child_ctx);
>>>>>
>>>>> Then, only at the end of execution, right before returning EOK, do
>>>>> *child_ctx = child;
>>>>>
>>
>> In sss_child_register():
>>
>> + error = hash_enter(sigchld_ctx->children,&key,&value);
>> + if (error != HASH_SUCCESS&& error != HASH_ERROR_KEY_NOT_FOUND) {
>>
>> I don't think hash_enter can return HASH_ERROR_KEY_NOT_FOUND (what would
>> it mean?) and even if it did, why ignore it?
>>
>>>>>
>>>>> sss_child_invoke_cb():
>>>>> I'm wary of having the callback invocation free the child_ctx. It
needs
>>>>> to be documented well (so that consumers know they must not free it
once
>>>>> the callback has fired or they'll get a double-free).
>>
>> This still needs fixing.
>>
>
> Forgot about that, sorry. Replaced the talloc_free with removing
> child_ctx from the hash table only.
>
>>>>>
>>>>> sss_child_handler():
>>>>> You need to check whether waitpid() returned an error and handle
that
>>>>> appropriately. waitpid() will return -1 on error and set errno. If
the
>>>>> errno value is EINTR, you should retry (because waitpid() was
>>>>> interrupted by a signal at the time). Other errors should generate a
>>>>> DEBUG message.
>>
>> I think you misunderstood how the error handling should be done:
>>
>> + if (pid == -1) {
>> + if (errno == ECHILD) {
>> + DEBUG(0, ("waitpid failed with ECHILD\n"));
>> + } else if (errno == EINVAL) {
>> + DEBUG(0, ("waitpid failed with EINVAL\n"));
>> + }
>> + return;
>> + }
>>
>> Instead of having a DEBUG() call per retval, it would be better to inform user
>> on why the child process exited, similarly to how child_sig_handler() does it
>> now.
>>
>
> I don't agree. The system is designed to inform providers that a
> child has exited by invoking their callbacks. It's up to the
> provider to decide what to do with the result and inform the user if
> appropriate. This would also make sense only in the case of a
> successful call (pid>= 0).
OK, then why the separate branches for ECHILD and EINVAL? Isn't it
easier to just print errno and strerror(errno) ?
Yeah, it's definitely better. Improved in current patch. :)
>
> In case of EINTR, we need to repeat the call to waitpid, because it
> was interrupted by a signal. In other cases (ECHILD and EINVAL) the
> call is going to always fail, so there's won't be any callbacks to
> invoke.
Yup.
>
>>>>>
>>>>
>>>> Ok, I'm done with all of this. It was pretty straight forward,
but...
>>>>
>>>>>
>>>>> Please separate any changes you make to the existing sss_child_ctx
into
>>>>> its own patch. I don't like just renaming it to _old, either.
Please
>>>>> find a more appropriate way to replace this old object (preferably
by
>>>>> converting it to use your new mechanism, also in a new patch).
>>>>>
>>>>
>>>> ... this, I'm trying to find a way to make the old code use the new
mechanism. I
>>>> think it would be better to remove the old one completely and replace it
with
>>>> the new one everywhere where it's used.
>>>>
>>>> Pavel
>>>
>>> New patch with the above mentioned issues fixed is attached to this email.
>>>
>>> We agreed that we're going to replace all uses of the old system by
>>> the new. Expect about 3 patches to follow on this one - one for each
>>> provider using the old system.
>>>
>>> Pavel
>>
>
> Thanks for taking the time to review this. New patch attached.
>
> Pavel
Two more issues (sorry for not catching all this at once):
Please don't just use DEBUG(0) for all the messages. DEBUG(0) is
supposed to be used for fatal errors that cause the provider to quit
completely.
See https://fedorahosted.org/pipermail/sssd-devel/2011-June/006407.html
for explanation of the log levels. In general, this patch is going to be
included in 1.7 and onwards -- please use the new debug levels (see
src/util/util.h) defined as macros instead of integers.
Ok.
The second issue is that I'm getting a segfault when logging in
via
Kerberos, the backtrace is pointing to tevent_common_check_signal().
Finally found what the problem was. I suspect there might be a bug in tevent.
The SEGFAULT was generated from tevent_common_check_signal as you pointed out.
It happened when copying a portion of siginfo that my signal handler wasn't using.
Simply adding the SA_SIGINFO flag to tevent_add_signal solved the issue.
Updated patch attached.
Pavel
7:58 a.m.
On Wed, 2011-11-09 at 15:31 +0100, Pavel Zuna wrote:
Updated patch attached.
Nack.
Please make "struct sss_sigchild_ctx" an opaque structure and provide an
init routine to create the hash and set up the signal handler. This way
we only have to call
sss_sigchild_init(ctx, &ctx->sigchld_ctx);
in be_process_init() (or any other executable that needs child control).
In sss_child_handler(), don't consider it fatal if the PID isn't found
in the hash. It could mean that something we're linked against did a
fork and was trying to maintain itself. This is bad, but not fatal.
Lower the debug level a bit (I think SSSDBG_OP_FAILURE is fine) and
allow the waitpid() loop to continue, rather than exiting immediately.
sss_child_ctx should be moved to child_common.c. Its internals should
not be visible elsewhere.
Please reorder sss_child_invoke_cb() to remove the entry from the hash
before invoking the callback (you cannot assume that the callback won't
do something funny to the hash structure or child_ctx, though it should
be safe). Also, please raise the debug level for this failure to
SSSDBG_OP_FAILURE.
10:14 a.m.
On Tue, 2011-11-22 at 08:58 -0500, Stephen Gallagher wrote:
On Wed, 2011-11-09 at 15:31 +0100, Pavel Zuna wrote:
> Updated patch attached.
Nack.
Please make "struct sss_sigchild_ctx" an opaque structure and provide an
init routine to create the hash and set up the signal handler. This way
we only have to call
sss_sigchild_init(ctx, &ctx->sigchld_ctx);
in be_process_init() (or any other executable that needs child control).
In sss_child_handler(), don't consider it fatal if the PID isn't found
in the hash. It could mean that something we're linked against did a
fork and was trying to maintain itself. This is bad, but not fatal.
Lower the debug level a bit (I think SSSDBG_OP_FAILURE is fine) and
allow the waitpid() loop to continue, rather than exiting immediately.
sss_child_ctx should be moved to child_common.c. Its internals should
not be visible elsewhere.
Please reorder sss_child_invoke_cb() to remove the entry from the hash
before invoking the callback (you cannot assume that the callback won't
do something funny to the hash structure or child_ctx, though it should
be safe). Also, please raise the debug level for this failure to
SSSDBG_OP_FAILURE.
I've taken over the development of this patch. I'm sending my changes to
address the above issues except for a slight modification to my comments
about the sss_child_handler loop - it was already ignoring unknown
errors, but it was exiting on unexpected failures. I added a comment and
will allow it to try to continue looping through in the hopes that it
will minimize the potential zombies.
This patch does not currently consume the functions that it creates. I
am working on converting the existing child handlers to use this code,
but I want to get this patch looked at and acked in the meantime.
8:09 a.m.
On Tue, Dec 13, 2011 at 11:14:31AM -0500, Stephen Gallagher wrote:
On Tue, 2011-11-22 at 08:58 -0500, Stephen Gallagher wrote:
> On Wed, 2011-11-09 at 15:31 +0100, Pavel Zuna wrote:
> > Updated patch attached.
>
>
>
> Nack.
>
> Please make "struct sss_sigchild_ctx" an opaque structure and provide an
> init routine to create the hash and set up the signal handler. This way
> we only have to call
> sss_sigchild_init(ctx, &ctx->sigchld_ctx);
> in be_process_init() (or any other executable that needs child control).
>
> In sss_child_handler(), don't consider it fatal if the PID isn't found
> in the hash. It could mean that something we're linked against did a
> fork and was trying to maintain itself. This is bad, but not fatal.
> Lower the debug level a bit (I think SSSDBG_OP_FAILURE is fine) and
> allow the waitpid() loop to continue, rather than exiting immediately.
>
> sss_child_ctx should be moved to child_common.c. Its internals should
> not be visible elsewhere.
>
> Please reorder sss_child_invoke_cb() to remove the entry from the hash
> before invoking the callback (you cannot assume that the callback won't
> do something funny to the hash structure or child_ctx, though it should
> be safe). Also, please raise the debug level for this failure to
> SSSDBG_OP_FAILURE.
I've taken over the development of this patch. I'm sending my changes to
address the above issues except for a slight modification to my comments
about the sss_child_handler loop - it was already ignoring unknown
errors, but it was exiting on unexpected failures. I added a comment and
will allow it to try to continue looping through in the hopes that it
will minimize the potential zombies.
This patch does not currently consume the functions that it creates. I
am working on converting the existing child handlers to use this code,
but I want to get this patch looked at and acked in the meantime.
Are you going to fix the two comments you had originally in the later
patch? Those were don't consider it fatal if the PID isn't found
in the hash in sss_child_handler() and reordering sss_child_invoke_cb()
If so, then ack
8:15 a.m.
On Dec 14, 2011, at 9:09 AM, Jakub Hrozek <jhrozek(a)redhat.com> wrote:
On Tue, Dec 13, 2011 at 11:14:31AM -0500, Stephen Gallagher wrote:
> On Tue, 2011-11-22 at 08:58 -0500, Stephen Gallagher wrote:
>> On Wed, 2011-11-09 at 15:31 +0100, Pavel Zuna wrote:
>>> Updated patch attached.
>>
>>
>>
>> Nack.
>>
>> Please make "struct sss_sigchild_ctx" an opaque structure and provide
an
>> init routine to create the hash and set up the signal handler. This way
>> we only have to call
>> sss_sigchild_init(ctx, &ctx->sigchld_ctx);
>> in be_process_init() (or any other executable that needs child control).
>>
>> In sss_child_handler(), don't consider it fatal if the PID isn't found
>> in the hash. It could mean that something we're linked against did a
>> fork and was trying to maintain itself. This is bad, but not fatal.
>> Lower the debug level a bit (I think SSSDBG_OP_FAILURE is fine) and
>> allow the waitpid() loop to continue, rather than exiting immediately.
>>
>> sss_child_ctx should be moved to child_common.c. Its internals should
>> not be visible elsewhere.
>>
>> Please reorder sss_child_invoke_cb() to remove the entry from the hash
>> before invoking the callback (you cannot assume that the callback won't
>> do something funny to the hash structure or child_ctx, though it should
>> be safe). Also, please raise the debug level for this failure to
>> SSSDBG_OP_FAILURE.
>
> I've taken over the development of this patch. I'm sending my changes to
> address the above issues except for a slight modification to my comments
> about the sss_child_handler loop - it was already ignoring unknown
> errors, but it was exiting on unexpected failures. I added a comment and
> will allow it to try to continue looping through in the hopes that it
> will minimize the potential zombies.
>
> This patch does not currently consume the functions that it creates. I
> am working on converting the existing child handlers to use this code,
> but I want to get this patch looked at and acked in the meantime.
Are you going to fix the two comments you had originally in the later
patch? Those were don't consider it fatal if the PID isn't found
in the hash in sss_child_handler() and reordering sss_child_invoke_cb()
If so, then ack
The first part of that I already described above as being an incorrect reading of the
source. I missed the second part and I'll fix that shortly.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
12:03 p.m.
On Wed, 2011-12-14 at 09:15 -0500, Stephen Gallagher wrote:
On Dec 14, 2011, at 9:09 AM, Jakub Hrozek <jhrozek(a)redhat.com> wrote:
>
> Are you going to fix the two comments you had originally in the later
> patch? Those were don't consider it fatal if the PID isn't found
> in the hash in sss_child_handler() and reordering sss_child_invoke_cb()
>
> If so, then ack
The first part of that I already described above as being an incorrect
reading of the source. I missed the second part and I'll fix that
shortly.
New patch attached fixes the missing reordering of the
sssd_child_invoke_cb() and adds more explicit handling of unknown PIDs
(with a new DEBUG log message).
8:32 p.m.
On Wed, 2011-12-14 at 13:03 -0500, Stephen Gallagher wrote:
On Wed, 2011-12-14 at 09:15 -0500, Stephen Gallagher wrote:
>
> On Dec 14, 2011, at 9:09 AM, Jakub Hrozek <jhrozek(a)redhat.com> wrote:
> >
> > Are you going to fix the two comments you had originally in the later
> > patch? Those were don't consider it fatal if the PID isn't found
> > in the hash in sss_child_handler() and reordering sss_child_invoke_cb()
> >
> > If so, then ack
>
>
> The first part of that I already described above as being an incorrect
> reading of the source. I missed the second part and I'll fix that
> shortly.
New patch attached fixes the missing reordering of the
sssd_child_invoke_cb() and adds more explicit handling of unknown PIDs
(with a new DEBUG log message).
I located one very serious bug while working on some dependent patches.
The waitpid() in this patch was using a bitwise AND with the WNOHANG
instead of a bitwise OR (resulting in the WNOHANG being disabled and
causing waitpid() to spin forever in some situations). New patch
attached.
I also moved the child_common routines out of the provider directory and
into util. I did this in a second patch so it's easier to follow the
changes.
9 p.m.
On Wed, 2011-12-14 at 13:03 -0500, Stephen Gallagher wrote:
On Wed, 2011-12-14 at 09:15 -0500, Stephen Gallagher wrote:
>
> On Dec 14, 2011, at 9:09 AM, Jakub Hrozek <jhrozek(a)redhat.com> wrote:
> >
> > Are you going to fix the two comments you had originally in the later
> > patch? Those were don't consider it fatal if the PID isn't found
> > in the hash in sss_child_handler() and reordering sss_child_invoke_cb()
> >
> > If so, then ack
>
>
> The first part of that I already described above as being an incorrect
> reading of the source. I missed the second part and I'll fix that
> shortly.
New patch attached fixes the missing reordering of the
sssd_child_invoke_cb() and adds more explicit handling of unknown PIDs
(with a new DEBUG log message).
I discovered two more issues with this patch, one serious, one cosmetic
but annoying.
1) sss_child_register() wasn't assigning child->sigchld_ctx, so if the
child was freed, the destructor would segfault trying to remove this
child from the hash table.
2) sss_child_handler() needed to ignore a return of pid=0 (which would
happen every time as the last pass through) or else it would print an
error trying to find pid 0 in the hash table (which it had better not
be. I don't want SSSD monitoring init!)
New patches attached.
5:59 a.m.
On Wed, Dec 14, 2011 at 10:00:58PM -0500, Stephen Gallagher wrote:
On Wed, 2011-12-14 at 13:03 -0500, Stephen Gallagher wrote:
> On Wed, 2011-12-14 at 09:15 -0500, Stephen Gallagher wrote:
> >
> > On Dec 14, 2011, at 9:09 AM, Jakub Hrozek <jhrozek(a)redhat.com> wrote:
> > >
> > > Are you going to fix the two comments you had originally in the later
> > > patch? Those were don't consider it fatal if the PID isn't found
> > > in the hash in sss_child_handler() and reordering sss_child_invoke_cb()
> > >
> > > If so, then ack
> >
> >
> > The first part of that I already described above as being an incorrect
> > reading of the source. I missed the second part and I'll fix that
> > shortly.
>
>
> New patch attached fixes the missing reordering of the
> sssd_child_invoke_cb() and adds more explicit handling of unknown PIDs
> (with a new DEBUG log message).
I discovered two more issues with this patch, one serious, one cosmetic
but annoying.
1) sss_child_register() wasn't assigning child->sigchld_ctx, so if the
child was freed, the destructor would segfault trying to remove this
child from the hash table.
2) sss_child_handler() needed to ignore a return of pid=0 (which would
happen every time as the last pass through) or else it would print an
error trying to find pid 0 in the hash table (which it had better not
be. I don't want SSSD monitoring init!)
init is pid=1 :-)
New patches attached.
Ack to this version.
10:38 a.m.
On Mon, 2011-12-19 at 12:59 +0100, Jakub Hrozek wrote:
On Wed, Dec 14, 2011 at 10:00:58PM -0500, Stephen Gallagher wrote:
> On Wed, 2011-12-14 at 13:03 -0500, Stephen Gallagher wrote:
> > On Wed, 2011-12-14 at 09:15 -0500, Stephen Gallagher wrote:
> > >
> > > On Dec 14, 2011, at 9:09 AM, Jakub Hrozek <jhrozek(a)redhat.com>
wrote:
> > > >
> > > > Are you going to fix the two comments you had originally in the
later
> > > > patch? Those were don't consider it fatal if the PID isn't
found
> > > > in the hash in sss_child_handler() and reordering
sss_child_invoke_cb()
> > > >
> > > > If so, then ack
> > >
> > >
> > > The first part of that I already described above as being an incorrect
> > > reading of the source. I missed the second part and I'll fix that
> > > shortly.
> >
> >
> > New patch attached fixes the missing reordering of the
> > sssd_child_invoke_cb() and adds more explicit handling of unknown PIDs
> > (with a new DEBUG log message).
>
> I discovered two more issues with this patch, one serious, one cosmetic
> but annoying.
>
> 1) sss_child_register() wasn't assigning child->sigchld_ctx, so if the
> child was freed, the destructor would segfault trying to remove this
> child from the hash table.
>
> 2) sss_child_handler() needed to ignore a return of pid=0 (which would
> happen every time as the last pass through) or else it would print an
> error trying to find pid 0 in the hash table (which it had better not
> be. I don't want SSSD monitoring init!)
init is pid=1 :-)
>
> New patches attached.
Ack to this version.
Pushed to master.
8:03 a.m.
On Wed, Nov 09, 2011 at 03:31:42PM +0100, Pavel Zuna wrote:
On 11/01/2011 02:58 PM, Jakub Hrozek wrote:
>On Tue, Nov 01, 2011 at 11:34:27AM +0100, Pavel Zuna wrote:
>>On 10/31/2011 04:29 PM, Jakub Hrozek wrote:
>>>On Thu, Oct 27, 2011 at 02:03:25PM +0200, Pavel Zuna wrote:
>>>>On 10/25/2011 01:08 PM, Pavel Zuna wrote:
>>>>>On 10/21/2011 06:44 PM, Stephen Gallagher wrote:
>>>>>>On Fri, 2011-10-21 at 15:28 +0200, Pavel Zuna wrote:
>>>>>>>Base on the second proposal:
>>>>>>>
>>>>>>>https://fedorahosted.org/sssd/wiki/DesignDocs/SigChld
>>>>>>>
>>>>>>>There is some old SIGCHLD handling code in
src/providers/child_common.[ch], that
>>>>>>>should probably go away if this gets accepted. There was also
a naming conflict
>>>>>>>with the sss_child_ctx structure. This structure is only used
internally by
>>>>>>>functions defined src/providers/child_common.c. I renamed the
original structure
>>>>>>>to sss_child_ctx_old for now.
>>>>>>
>>>>>>
>>>>>>Nack (but a good start)
>>>>>>
>>>>>>sss_child_destructor() cannot return the result of hash_delete().
Talloc
>>>>>>destructors may only return 0 on success or -1 on failure.
However,
>>>>>>because destructor failure results in cancelling the free(), we
should
>>>>>>just log an error here and return 0.
>>>>>>
>>>>>>sss_child_register:
>>>>>>Don't initialize sss_child_ctx *tmp to the child_ctx passed
in. We want
>>>>>>to leave child_ctx untouched until successful completion. Also,
please
>>>>>>don't use the variable name "tmp". It's not
descriptive. Call it
>>>>>>"child".
>>>>>>
>>>>>>Instead of allocating *child_ctx at the beginning, allocate
"child" on
>>>>>>the mem_ctx.
>>>>>>
>>>>>>child = talloc_zero(mem_ctx, struct sss_child_ctx);
>>>>>>
>>>>>>Then, only at the end of execution, right before returning EOK,
do
>>>>>>*child_ctx = child;
>>>>>>
>>>
>>>In sss_child_register():
>>>
>>>+ error = hash_enter(sigchld_ctx->children,&key,&value);
>>>+ if (error != HASH_SUCCESS&& error != HASH_ERROR_KEY_NOT_FOUND)
{
>>>
>>>I don't think hash_enter can return HASH_ERROR_KEY_NOT_FOUND (what would
>>>it mean?) and even if it did, why ignore it?
>>>
>>>>>>
>>>>>>sss_child_invoke_cb():
>>>>>>I'm wary of having the callback invocation free the
child_ctx. It needs
>>>>>>to be documented well (so that consumers know they must not free
it once
>>>>>>the callback has fired or they'll get a double-free).
>>>
>>>This still needs fixing.
>>>
>>
>>Forgot about that, sorry. Replaced the talloc_free with removing
>>child_ctx from the hash table only.
>>
>>>>>>
>>>>>>sss_child_handler():
>>>>>>You need to check whether waitpid() returned an error and handle
that
>>>>>>appropriately. waitpid() will return -1 on error and set errno.
If the
>>>>>>errno value is EINTR, you should retry (because waitpid() was
>>>>>>interrupted by a signal at the time). Other errors should
generate a
>>>>>>DEBUG message.
>>>
>>>I think you misunderstood how the error handling should be done:
>>>
>>>+ if (pid == -1) {
>>>+ if (errno == ECHILD) {
>>>+ DEBUG(0, ("waitpid failed with ECHILD\n"));
>>>+ } else if (errno == EINVAL) {
>>>+ DEBUG(0, ("waitpid failed with EINVAL\n"));
>>>+ }
>>>+ return;
>>>+ }
>>>
>>>Instead of having a DEBUG() call per retval, it would be better to inform
user
>>>on why the child process exited, similarly to how child_sig_handler() does
it
>>>now.
>>>
>>
>>I don't agree. The system is designed to inform providers that a
>>child has exited by invoking their callbacks. It's up to the
>>provider to decide what to do with the result and inform the user if
>>appropriate. This would also make sense only in the case of a
>>successful call (pid>= 0).
>
>OK, then why the separate branches for ECHILD and EINVAL? Isn't it
>easier to just print errno and strerror(errno) ?
>
Yeah, it's definitely better. Improved in current patch. :)
>>
>>In case of EINTR, we need to repeat the call to waitpid, because it
>>was interrupted by a signal. In other cases (ECHILD and EINVAL) the
>>call is going to always fail, so there's won't be any callbacks to
>>invoke.
>
>Yup.
>
>>
>>>>>>
>>>>>
>>>>>Ok, I'm done with all of this. It was pretty straight forward,
but...
>>>>>
>>>>>>
>>>>>>Please separate any changes you make to the existing
sss_child_ctx into
>>>>>>its own patch. I don't like just renaming it to _old, either.
Please
>>>>>>find a more appropriate way to replace this old object
(preferably by
>>>>>>converting it to use your new mechanism, also in a new patch).
>>>>>>
>>>>>
>>>>>... this, I'm trying to find a way to make the old code use the
new mechanism. I
>>>>>think it would be better to remove the old one completely and replace
it with
>>>>>the new one everywhere where it's used.
>>>>>
>>>>>Pavel
>>>>
>>>>New patch with the above mentioned issues fixed is attached to this
email.
>>>>
>>>>We agreed that we're going to replace all uses of the old system by
>>>>the new. Expect about 3 patches to follow on this one - one for each
>>>>provider using the old system.
>>>>
>>>>Pavel
>>>
>>
>>Thanks for taking the time to review this. New patch attached.
>>
>>Pavel
>
>Two more issues (sorry for not catching all this at once):
>
>Please don't just use DEBUG(0) for all the messages. DEBUG(0) is
>supposed to be used for fatal errors that cause the provider to quit
>completely.
>
>See https://fedorahosted.org/pipermail/sssd-devel/2011-June/006407.html
>for explanation of the log levels. In general, this patch is going to be
>included in 1.7 and onwards -- please use the new debug levels (see
>src/util/util.h) defined as macros instead of integers.
>
Ok.
>The second issue is that I'm getting a segfault when logging in via
>Kerberos, the backtrace is pointing to tevent_common_check_signal().
Finally found what the problem was. I suspect there might be a bug in tevent.
The SEGFAULT was generated from tevent_common_check_signal as you
pointed out. It happened when copying a portion of siginfo that my
signal handler wasn't using.
Simply adding the SA_SIGINFO flag to tevent_add_signal solved the issue.
Updated patch attached.
Pavel
The patch is OK now, I just needed to use a 3-way merge to get it applied -
probably some patches landed in master since it was submitted for review.
Please ask Simo about the flag, this is too low-level tevent knowledge
for me.
I'll ack the patch once you know what was the tevent problem.
5:06 a.m.
On Thu, Oct 27, 2011 at 02:03:25PM +0200, Pavel Zuna wrote:
On 10/25/2011 01:08 PM, Pavel Zuna wrote:
>On 10/21/2011 06:44 PM, Stephen Gallagher wrote:
>>On Fri, 2011-10-21 at 15:28 +0200, Pavel Zuna wrote:
>>>Base on the second proposal:
>>>
>>>https://fedorahosted.org/sssd/wiki/DesignDocs/SigChld
>>>
>>>There is some old SIGCHLD handling code in src/providers/child_common.[ch],
that
>>>should probably go away if this gets accepted. There was also a naming
conflict
>>>with the sss_child_ctx structure. This structure is only used internally by
>>>functions defined src/providers/child_common.c. I renamed the original
structure
>>>to sss_child_ctx_old for now.
>>
>>
>>Nack (but a good start)
>>
>>sss_child_destructor() cannot return the result of hash_delete(). Talloc
>>destructors may only return 0 on success or -1 on failure. However,
>>because destructor failure results in cancelling the free(), we should
>>just log an error here and return 0.
>>
>>sss_child_register:
>>Don't initialize sss_child_ctx *tmp to the child_ctx passed in. We want
>>to leave child_ctx untouched until successful completion. Also, please
>>don't use the variable name "tmp". It's not descriptive. Call
it
>>"child".
>>
>>Instead of allocating *child_ctx at the beginning, allocate "child" on
>>the mem_ctx.
>>
>>child = talloc_zero(mem_ctx, struct sss_child_ctx);
>>
>>Then, only at the end of execution, right before returning EOK, do
>>*child_ctx = child;
>>
>>
>>sss_child_invoke_cb():
>>I'm wary of having the callback invocation free the child_ctx. It needs
>>to be documented well (so that consumers know they must not free it once
>>the callback has fired or they'll get a double-free).
>>
>>sss_child_handler():
>>You need to check whether waitpid() returned an error and handle that
>>appropriately. waitpid() will return -1 on error and set errno. If the
>>errno value is EINTR, you should retry (because waitpid() was
>>interrupted by a signal at the time). Other errors should generate a
>>DEBUG message.
>>
>
>Ok, I'm done with all of this. It was pretty straight forward, but...
>
>>
>>Please separate any changes you make to the existing sss_child_ctx into
>>its own patch. I don't like just renaming it to _old, either. Please
>>find a more appropriate way to replace this old object (preferably by
>>converting it to use your new mechanism, also in a new patch).
>>
>
>... this, I'm trying to find a way to make the old code use the new mechanism. I
>think it would be better to remove the old one completely and replace it with
>the new one everywhere where it's used.
>
>Pavel
New patch with the above mentioned issues fixed is attached to this email.
We agreed that we're going to replace all uses of the old system by
the new. Expect about 3 patches to follow on this one - one for each
provider using the old system.
Pavel
One more thing..this check is wrong:
+ sigchld_ctx = talloc_zero(ctx, struct sss_sigchild_ctx);
+ if (!ctx->sigchld_ctx) {
4511
days inactive
4570
days old
sssd-devel@lists.fedorahosted.org
18 comments
3 participants
participants (3)
-
Jakub Hrozek -
Pavel Zuna -
Stephen Gallagher