https://fedorahosted.org/sssd/ticket/1357
Neither systemd or our init script use pid file as a notification that sssd is finished initializing. They will continue starting up next service right after the original process is terminated.
On 10/18/2012 11:15 AM, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/1357
Neither systemd or our init script use pid file as a notification that sssd is finished initializing. They will continue starting up next service right after the original process is terminated.
Oops, I forgot to amend the patch with latest changes. The final patch is attached.
On Thu, Oct 18, 2012 at 11:28:10AM +0200, Pavel Březina wrote:
On 10/18/2012 11:15 AM, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/1357
Neither systemd or our init script use pid file as a notification that sssd is finished initializing. They will continue starting up next service right after the original process is terminated.
Oops, I forgot to amend the patch with latest changes. The final patch is attached.
I haven't really had time to read the patch too carefully yet, but my first thought was to always try to use tevent signals if you need signals at all.
On Thu, 2012-10-18 at 15:50 +0200, Jakub Hrozek wrote:
On Thu, Oct 18, 2012 at 11:28:10AM +0200, Pavel Březina wrote:
On 10/18/2012 11:15 AM, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/1357
Neither systemd or our init script use pid file as a notification that sssd is finished initializing. They will continue starting up next service right after the original process is terminated.
Oops, I forgot to amend the patch with latest changes. The final patch is attached.
I haven't really had time to read the patch too carefully yet, but my first thought was to always try to use tevent signals if you need signals at all.
It is generally mandatory to use tevent for signals, however if it is in the monitor it might be a special case, sorry I haven't looked closely at the patch either yet.
Simo.
On 10/18/2012 09:50 AM, Jakub Hrozek wrote:
On Thu, Oct 18, 2012 at 11:28:10AM +0200, Pavel Březina wrote:
On 10/18/2012 11:15 AM, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/1357
Neither systemd or our init script use pid file as a notification that sssd is finished initializing. They will continue starting up next service right after the original process is terminated.
Oops, I forgot to amend the patch with latest changes. The final patch is attached.
I haven't really had time to read the patch too carefully yet, but my first thought was to always try to use tevent signals if you need signals at all.
Yes, I went through a lot of effort a little over a year ago to get the monitor properly using tevent signals instead of managing its own directly. Please do not add manual handlers.
Nack.
On 10/18/2012 08:43 PM, Stephen Gallagher wrote:
On 10/18/2012 09:50 AM, Jakub Hrozek wrote:
On Thu, Oct 18, 2012 at 11:28:10AM +0200, Pavel Březina wrote:
On 10/18/2012 11:15 AM, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/1357
Neither systemd or our init script use pid file as a notification that sssd is finished initializing. They will continue starting up next service right after the original process is terminated.
Oops, I forgot to amend the patch with latest changes. The final patch is attached.
I haven't really had time to read the patch too carefully yet, but my first thought was to always try to use tevent signals if you need signals at all.
Yes, I went through a lot of effort a little over a year ago to get the monitor properly using tevent signals instead of managing its own directly. Please do not add manual handlers.
Nack.
Hi guys, I just want to make sure that we are on the same page here before I start modifying it to tevent.
I am not mixing tevent signals with posix signals. There is no existing tevent context available and there will never be (unless I create it).
I just need to catch SIGTERM in original process (that forks and become monitor), so that monitor can signal the original process to quit. Or wait for the monitor to exit (in case of error etc.) if the signal doesn't come.
Using tevent for this case seems like using a sledgehammer to crack a nut.
On Fri, 2012-10-19 at 09:52 +0200, Pavel Březina wrote:
On 10/18/2012 08:43 PM, Stephen Gallagher wrote:
On 10/18/2012 09:50 AM, Jakub Hrozek wrote:
On Thu, Oct 18, 2012 at 11:28:10AM +0200, Pavel Březina wrote:
On 10/18/2012 11:15 AM, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/1357
Neither systemd or our init script use pid file as a notification that sssd is finished initializing. They will continue starting up next service right after the original process is terminated.
Oops, I forgot to amend the patch with latest changes. The final patch is attached.
I haven't really had time to read the patch too carefully yet, but my first thought was to always try to use tevent signals if you need signals at all.
Yes, I went through a lot of effort a little over a year ago to get the monitor properly using tevent signals instead of managing its own directly. Please do not add manual handlers.
Nack.
Hi guys, I just want to make sure that we are on the same page here before I start modifying it to tevent.
I am not mixing tevent signals with posix signals. There is no existing tevent context available and there will never be (unless I create it).
I just need to catch SIGTERM in original process (that forks and become monitor), so that monitor can signal the original process to quit. Or wait for the monitor to exit (in case of error etc.) if the signal doesn't come.
Using tevent for this case seems like using a sledgehammer to crack a nut.
Pavel if this is before we create the tevent event context than it is ok to directly handle signals, however put a comment there taht says so. example: /* We use signals directly here because we don't have a tevent context yet */
Simo.
On 10/19/2012 02:20 PM, Simo Sorce wrote:
On Fri, 2012-10-19 at 09:52 +0200, Pavel Březina wrote:
On 10/18/2012 08:43 PM, Stephen Gallagher wrote:
On 10/18/2012 09:50 AM, Jakub Hrozek wrote:
On Thu, Oct 18, 2012 at 11:28:10AM +0200, Pavel Březina wrote:
On 10/18/2012 11:15 AM, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/1357
Neither systemd or our init script use pid file as a notification that sssd is finished initializing. They will continue starting up next service right after the original process is terminated.
Oops, I forgot to amend the patch with latest changes. The final patch is attached.
I haven't really had time to read the patch too carefully yet, but my first thought was to always try to use tevent signals if you need signals at all.
Yes, I went through a lot of effort a little over a year ago to get the monitor properly using tevent signals instead of managing its own directly. Please do not add manual handlers.
Nack.
Hi guys, I just want to make sure that we are on the same page here before I start modifying it to tevent.
I am not mixing tevent signals with posix signals. There is no existing tevent context available and there will never be (unless I create it).
I just need to catch SIGTERM in original process (that forks and become monitor), so that monitor can signal the original process to quit. Or wait for the monitor to exit (in case of error etc.) if the signal doesn't come.
Using tevent for this case seems like using a sledgehammer to crack a nut.
Pavel if this is before we create the tevent event context than it is ok to directly handle signals, however put a comment there taht says so. example: /* We use signals directly here because we don't have a tevent context yet */
Simo.
Very well. New patch is attached.
On 10/22/2012 01:49 PM, Pavel Březina wrote:
On 10/19/2012 02:20 PM, Simo Sorce wrote:
On Fri, 2012-10-19 at 09:52 +0200, Pavel Březina wrote:
On 10/18/2012 08:43 PM, Stephen Gallagher wrote:
On 10/18/2012 09:50 AM, Jakub Hrozek wrote:
On Thu, Oct 18, 2012 at 11:28:10AM +0200, Pavel Březina wrote:
On 10/18/2012 11:15 AM, Pavel Březina wrote: > https://fedorahosted.org/sssd/ticket/1357 > > Neither systemd or our init script use pid file as a notification > that sssd is finished initializing. They will continue starting up > next service right after the original process is terminated.
Oops, I forgot to amend the patch with latest changes. The final patch is attached.
I haven't really had time to read the patch too carefully yet, but my first thought was to always try to use tevent signals if you need signals at all.
Yes, I went through a lot of effort a little over a year ago to get the monitor properly using tevent signals instead of managing its own directly. Please do not add manual handlers.
Nack.
Hi guys, I just want to make sure that we are on the same page here before I start modifying it to tevent.
I am not mixing tevent signals with posix signals. There is no existing tevent context available and there will never be (unless I create it).
I just need to catch SIGTERM in original process (that forks and become monitor), so that monitor can signal the original process to quit. Or wait for the monitor to exit (in case of error etc.) if the signal doesn't come.
Using tevent for this case seems like using a sledgehammer to crack a nut.
Pavel if this is before we create the tevent event context than it is ok to directly handle signals, however put a comment there taht says so. example: /* We use signals directly here because we don't have a tevent context yet */
Simo.
Very well. New patch is attached.
Hi, I'm sending new patch set.
[PATCH 1/2] exit original process after sssd is initialized ...unchanged.
[PATCH 2/2] create pid file immediately after fork again We realized that sysv and systemd does not use pid file existence as a notification of finished initialization. Therefore, we create the pid file immediately after we fork to become daemon.
On Fri, Oct 26, 2012 at 01:10:38PM +0200, Pavel Březina wrote:
On 10/22/2012 01:49 PM, Pavel Březina wrote:
On 10/19/2012 02:20 PM, Simo Sorce wrote:
On Fri, 2012-10-19 at 09:52 +0200, Pavel Březina wrote:
On 10/18/2012 08:43 PM, Stephen Gallagher wrote:
On 10/18/2012 09:50 AM, Jakub Hrozek wrote:
On Thu, Oct 18, 2012 at 11:28:10AM +0200, Pavel Březina wrote: >On 10/18/2012 11:15 AM, Pavel Březina wrote: >>https://fedorahosted.org/sssd/ticket/1357 >> >>Neither systemd or our init script use pid file as a notification >>that sssd is finished initializing. They will continue starting up >>next service right after the original process is terminated. > >Oops, I forgot to amend the patch with latest changes. The final >patch is attached.
I haven't really had time to read the patch too carefully yet, but my first thought was to always try to use tevent signals if you need signals at all.
Yes, I went through a lot of effort a little over a year ago to get the monitor properly using tevent signals instead of managing its own directly. Please do not add manual handlers.
Nack.
Hi guys, I just want to make sure that we are on the same page here before I start modifying it to tevent.
I am not mixing tevent signals with posix signals. There is no existing tevent context available and there will never be (unless I create it).
I just need to catch SIGTERM in original process (that forks and become monitor), so that monitor can signal the original process to quit. Or wait for the monitor to exit (in case of error etc.) if the signal doesn't come.
Using tevent for this case seems like using a sledgehammer to crack a nut.
Pavel if this is before we create the tevent event context than it is ok to directly handle signals, however put a comment there taht says so. example: /* We use signals directly here because we don't have a tevent context yet */
Simo.
Very well. New patch is attached.
Hi, I'm sending new patch set.
[PATCH 1/2] exit original process after sssd is initialized ...unchanged.
[PATCH 2/2] create pid file immediately after fork again We realized that sysv and systemd does not use pid file existence as a notification of finished initialization. Therefore, we create the pid file immediately after we fork to become daemon.
Positive testing went fine. So far I only have one remark for the code -- is there a reason to keep the parent pid around at all? Why not simply call getppid()?
Also, starting the service doesn't work if any provider is misconfigured, it seems that the parent never exits. I tested by simply putting an invalid ldap URI (ldap=someserver instead of ldap://someserver).
On 10/30/2012 01:51 PM, Jakub Hrozek wrote:
On Fri, Oct 26, 2012 at 01:10:38PM +0200, Pavel Březina wrote:
On 10/22/2012 01:49 PM, Pavel Březina wrote:
On 10/19/2012 02:20 PM, Simo Sorce wrote:
On Fri, 2012-10-19 at 09:52 +0200, Pavel Březina wrote:
On 10/18/2012 08:43 PM, Stephen Gallagher wrote:
On 10/18/2012 09:50 AM, Jakub Hrozek wrote: > On Thu, Oct 18, 2012 at 11:28:10AM +0200, Pavel Březina wrote: >> On 10/18/2012 11:15 AM, Pavel Březina wrote: >>> https://fedorahosted.org/sssd/ticket/1357 >>> >>> Neither systemd or our init script use pid file as a notification >>> that sssd is finished initializing. They will continue starting up >>> next service right after the original process is terminated. >> >> Oops, I forgot to amend the patch with latest changes. The final >> patch is attached. > > I haven't really had time to read the patch too carefully yet, but my > first thought was to always try to use tevent signals if you need > signals at all.
Yes, I went through a lot of effort a little over a year ago to get the monitor properly using tevent signals instead of managing its own directly. Please do not add manual handlers.
Nack.
Hi guys, I just want to make sure that we are on the same page here before I start modifying it to tevent.
I am not mixing tevent signals with posix signals. There is no existing tevent context available and there will never be (unless I create it).
I just need to catch SIGTERM in original process (that forks and become monitor), so that monitor can signal the original process to quit. Or wait for the monitor to exit (in case of error etc.) if the signal doesn't come.
Using tevent for this case seems like using a sledgehammer to crack a nut.
Pavel if this is before we create the tevent event context than it is ok to directly handle signals, however put a comment there taht says so. example: /* We use signals directly here because we don't have a tevent context yet */
Simo.
Very well. New patch is attached.
Hi, I'm sending new patch set.
[PATCH 1/2] exit original process after sssd is initialized ...unchanged.
[PATCH 2/2] create pid file immediately after fork again We realized that sysv and systemd does not use pid file existence as a notification of finished initialization. Therefore, we create the pid file immediately after we fork to become daemon.
Positive testing went fine. So far I only have one remark for the code -- is there a reason to keep the parent pid around at all? Why not simply call getppid()?
Also, starting the service doesn't work if any provider is misconfigured, it seems that the parent never exits. I tested by simply putting an invalid ldap URI (ldap=someserver instead of ldap://someserver).
I'm sending another set of patches after our offline discussion.
On Thu, Nov 01, 2012 at 05:36:10PM +0100, Pavel Březina wrote:
On 10/30/2012 01:51 PM, Jakub Hrozek wrote:
On Fri, Oct 26, 2012 at 01:10:38PM +0200, Pavel Březina wrote:
On 10/22/2012 01:49 PM, Pavel Březina wrote:
On 10/19/2012 02:20 PM, Simo Sorce wrote:
On Fri, 2012-10-19 at 09:52 +0200, Pavel Březina wrote:
On 10/18/2012 08:43 PM, Stephen Gallagher wrote: >On 10/18/2012 09:50 AM, Jakub Hrozek wrote: >>On Thu, Oct 18, 2012 at 11:28:10AM +0200, Pavel Březina wrote: >>>On 10/18/2012 11:15 AM, Pavel Březina wrote: >>>>https://fedorahosted.org/sssd/ticket/1357 >>>> >>>>Neither systemd or our init script use pid file as a notification >>>>that sssd is finished initializing. They will continue starting up >>>>next service right after the original process is terminated. >>> >>>Oops, I forgot to amend the patch with latest changes. The final >>>patch is attached. >> >>I haven't really had time to read the patch too carefully yet, but my >>first thought was to always try to use tevent signals if you need >>signals at all. > >Yes, I went through a lot of effort a little over a year ago to get the >monitor properly using tevent signals instead of managing its own >directly. Please do not add manual handlers. > >Nack.
Hi guys, I just want to make sure that we are on the same page here before I start modifying it to tevent.
I am not mixing tevent signals with posix signals. There is no existing tevent context available and there will never be (unless I create it).
I just need to catch SIGTERM in original process (that forks and become monitor), so that monitor can signal the original process to quit. Or wait for the monitor to exit (in case of error etc.) if the signal doesn't come.
Using tevent for this case seems like using a sledgehammer to crack a nut.
Pavel if this is before we create the tevent event context than it is ok to directly handle signals, however put a comment there taht says so. example: /* We use signals directly here because we don't have a tevent context yet */
Simo.
Very well. New patch is attached.
Hi, I'm sending new patch set.
[PATCH 1/2] exit original process after sssd is initialized ...unchanged.
[PATCH 2/2] create pid file immediately after fork again We realized that sysv and systemd does not use pid file existence as a notification of finished initialization. Therefore, we create the pid file immediately after we fork to become daemon.
Positive testing went fine. So far I only have one remark for the code -- is there a reason to keep the parent pid around at all? Why not simply call getppid()?
Also, starting the service doesn't work if any provider is misconfigured, it seems that the parent never exits. I tested by simply putting an invalid ldap URI (ldap=someserver instead of ldap://someserver).
I'm sending another set of patches after our offline discussion.
With these set of patches both positive and negative (breaking configuration, injecting kill(getpid(), SIGSEGV), etc) went fine.
I only have a couple of nitpicks about the code: * We should use new DEBUG macros in monitor_quit_signal() * DEBUG(SSSDBG_FATAL_FAILURE, ("Shutting down with: %d\n", ret)); should say returned with
I would also like to ask Simo for a second look as the startup issues are delicate.
On Thu, 2012-11-01 at 17:36 +0100, Pavel Březina wrote:
if (ctx->is_daemon && ctx->parent_pid > 0&& ctx->parent_pid == getppid()) {if (ctx->parent_pid <= 0 || ctx->parent_pid != getppid()){
/* the parent process was already terminated */DEBUG(SSSDBG_MINOR_FAILURE, ("Invalid parent pid\n"));
goto done;}
Isn't the second if condition redundant here ? we can't even test it unless it is already true according to the previous if () or am I missing something ? Did you intend to use some || in the previous condition ?
Simo.
On Fri, Nov 02, 2012 at 01:53:52PM -0400, Simo Sorce wrote:
On Thu, 2012-11-01 at 17:36 +0100, Pavel Březina wrote:
if (ctx->is_daemon && ctx->parent_pid > 0&& ctx->parent_pid == getppid()) {if (ctx->parent_pid <= 0 || ctx->parent_pid != getppid()){
/* the parent process was already terminated */DEBUG(SSSDBG_MINOR_FAILURE, ("Invalid parent pid\n"));
goto done;}Isn't the second if condition redundant here ? we can't even test it unless it is already true according to the previous if () or am I missing something ? Did you intend to use some || in the previous condition ?
Oops, I think you're right, thank you Simo.
To reiterate the discussion we had with Pavel in person[1], we wanted to make sure we're really signaling the original parent and not e.g. init in the odd case the parent died before we got to this point.
[1] This is a little off-topic, but as many of the SSSD developers are now in the same office, it is very convenient for us to talk in person. Obviously this has the drawback of excluding the other developers and the community from the discussion. We should try harder to document the thoughts in more detail even though it might seem redundant.
On Fri, 2012-11-02 at 19:11 +0100, Jakub Hrozek wrote:
On Fri, Nov 02, 2012 at 01:53:52PM -0400, Simo Sorce wrote:
On Thu, 2012-11-01 at 17:36 +0100, Pavel Březina wrote:
if (ctx->is_daemon && ctx->parent_pid > 0&& ctx->parent_pid == getppid()) {if (ctx->parent_pid <= 0 || ctx->parent_pid != getppid()){
/* the parent process was already terminated */DEBUG(SSSDBG_MINOR_FAILURE, ("Invalid parent pid\n"));
goto done;}Isn't the second if condition redundant here ? we can't even test it unless it is already true according to the previous if () or am I missing something ? Did you intend to use some || in the previous condition ?
Oops, I think you're right, thank you Simo.
To reiterate the discussion we had with Pavel in person[1], we wanted to make sure we're really signaling the original parent and not e.g. init in the odd case the parent died before we got to this point.
init has pid 1 iirc, should you check for > 1 ?
[1] This is a little off-topic, but as many of the SSSD developers are now in the same office, it is very convenient for us to talk in person. Obviously this has the drawback of excluding the other developers and the community from the discussion. We should try harder to document the thoughts in more detail even though it might seem redundant.
Yes please, those comments should go either in the commit message or comments in the code, or if they are just additional considerations in the mail message that comes with the patch.
Simo.
On 11/02/2012 06:53 PM, Simo Sorce wrote:
On Thu, 2012-11-01 at 17:36 +0100, Pavel Březina wrote:
if (ctx->is_daemon && ctx->parent_pid > 0&& ctx->parent_pid == getppid()) {if (ctx->parent_pid <= 0 || ctx->parent_pid != getppid()){
/* the parent process was already terminated */DEBUG(SSSDBG_MINOR_FAILURE, ("Invalid parent pid\n"));
goto done;}Isn't the second if condition redundant here ?
Yes, definitely. This is a rebase bug :/
we can't even test it unless it is already true according to the previous if () or am I missing something ? Did you intend to use some || in the previous condition ?
Simo.
On 11/02/2012 06:53 PM, Simo Sorce wrote:
On Thu, 2012-11-01 at 17:36 +0100, Pavel Březina wrote:
if (ctx->is_daemon && ctx->parent_pid > 0&& ctx->parent_pid == getppid()) {if (ctx->parent_pid <= 0 || ctx->parent_pid != getppid()){
/* the parent process was already terminated */DEBUG(SSSDBG_MINOR_FAILURE, ("Invalid parent pid\n"));
goto done;}Isn't the second if condition redundant here ? we can't even test it unless it is already true according to the previous if () or am I missing something ? Did you intend to use some || in the previous condition ?
Simo.
I'm sending a new set of patches. There are the three original and two new.
[1/5] fixes indentation in the whole server.c [2/5] introduces a new debug level macro SSSDBG_IMPORTANT_INFO that should be used instead of SSSDBG_FATAL_FAILURE on places where the original level was 0, but it is not an error.
On Fri, 2012-11-02 at 22:58 +0100, Pavel Březina wrote:
Ok if you are fixing indent also fix style while there please so we do this once.
- /* try and use up these file descriptors, so silly
library routines writing to stdout etc won't cause havoc */- for (i=0;i<3;i++) {
should be for (i = 0; i < 3; i++) {
fd = open("/dev/null",O_RDWR,0);
space after ,
if (fd < 0)fd = open("/dev/null",O_WRONLY,0);
space after ,
if (fd < 0) {DEBUG(0,("Can't open /dev/null\n"));
space after ,
return;}if (fd != i) {DEBUG(0,("Didn't get file descriptor %d\n",i));
space after ,
return;}- }
#endif }
Also you can change the debug level from number to macro here, as it is not a functional change IMO.
Also later there is at least one line longer than 80, would be nice to fold that.
Simo.
On 11/02/2012 11:58 PM, Simo Sorce wrote:
On Fri, 2012-11-02 at 22:58 +0100, Pavel Březina wrote:
Ok if you are fixing indent also fix style while there please so we do this once.
- /* try and use up these file descriptors, so silly
library routines writing to stdout etc won't cause havoc */- for (i=0;i<3;i++) {
should be for (i = 0; i < 3; i++) {
fd = open("/dev/null",O_RDWR,0);space after ,
if (fd < 0)fd = open("/dev/null",O_WRONLY,0);space after ,
if (fd < 0) {DEBUG(0,("Can't open /dev/null\n"));space after ,
return;}if (fd != i) {DEBUG(0,("Didn't get file descriptor %d\n",i));space after ,
return;}- } #endif }
Also you can change the debug level from number to macro here, as it is not a functional change IMO.
Also later there is at least one line longer than 80, would be nice to fold that.
Simo.
Hi, there were some more coding style issues (mostly missing space after comma). When I was in it I changed all debug levels there to macros. There was actually a place where I could use SSSDBG_IMPORTANT_INFO so it made me happy :-)
New patches are attached. The order of the first two patches is switched.
Thank you for review.
On Sat, 2012-11-03 at 12:37 +0100, Pavel Březina wrote:
Hi, there were some more coding style issues (mostly missing space after comma). When I was in it I changed all debug levels there to macros. There was actually a place where I could use SSSDBG_IMPORTANT_INFO so it made me happy :-)
New patches are attached. The order of the first two patches is switched.
Thank you for review.
Very good!
ACK x 5
Simo.
On Mon, Nov 05, 2012 at 08:48:19AM -0500, Simo Sorce wrote:
On Sat, 2012-11-03 at 12:37 +0100, Pavel Březina wrote:
Hi, there were some more coding style issues (mostly missing space after comma). When I was in it I changed all debug levels there to macros. There was actually a place where I could use SSSDBG_IMPORTANT_INFO so it made me happy :-)
New patches are attached. The order of the first two patches is switched.
Thank you for review.
Very good!
ACK x 5
Pushed to master and sssd-1-9
On Fri, 2012-11-02 at 22:58 +0100, Pavel Březina wrote:
+#define SSSDBG_IMPORTANT_INFO 0x0010 /* level 0 */ #define SSSDBG_FATAL_FAILURE 0x0010 /* level 0 */ #define SSSDBG_CRIT_FAILURE 0x0020 /* level 1 */ #define SSSDBG_OP_FAILURE 0x0040 /* level 2 */
Uhm I am not particularly happy with proliferating macros, but I see why you would like a better name, however if it isn't a fatal failure shouldn;t we use a higher debug level ?
Also do not define numbers just define an alias to a macro so we do not risk changing one and not the other.
I propose: #define SSSDBG_IMPORTANT_INFO SSSDBG_OP_FAILURE
Simo.
On 11/03/2012 12:01 AM, Simo Sorce wrote:
On Fri, 2012-11-02 at 22:58 +0100, Pavel Březina wrote:
+#define SSSDBG_IMPORTANT_INFO 0x0010 /* level 0 */ #define SSSDBG_FATAL_FAILURE 0x0010 /* level 0 */ #define SSSDBG_CRIT_FAILURE 0x0020 /* level 1 */ #define SSSDBG_OP_FAILURE 0x0040 /* level 2 */
Uhm I am not particularly happy with proliferating macros, but I see why you would like a better name, however if it isn't a fatal failure shouldn;t we use a higher debug level ?
Also do not define numbers just define an alias to a macro so we do not risk changing one and not the other.
Well, in this particular case I didn't consider it as an alias to SSSDBG_FATAL_FAILURE, but as a separate level, which by coincidence happened to be an existing number (because I didn't want to change its value). In general, I would prefer a different number for this category.
But I think we should not mess with existing debug levels until a complete refactoring of their usage is done (I think there is a ticket for that).
I propose: #define SSSDBG_IMPORTANT_INFO SSSDBG_OP_FAILURE
Simo.
Thank you for the review, I'll send new patches tomorrow.
On Thu, 2012-11-01 at 17:36 +0100, Pavel Březina wrote:
I'm sending another set of patches after our offline discussion.
For patch 2 it looks like you did a re-indent of become_daemon(), but didn't fully re-indent it.
Maybe it would be better to send a re-indent patch first and then on top the patch that changes the code ?
Simo.
On Thu, 2012-11-01 at 17:36 +0100, Pavel Březina wrote:
+void become_daemon(bool Fork, pid_t *ppid) {
int ret;
- pid_t pid;
- int status;
- int ret;
if (Fork) {if (fork()) {_exit(0);}}
- if (Fork) {
pid = fork();if (pid != 0) {*ppid = 0;
Why care about ppid at all here? you never return from this branch as you call _exit();
/* Terminate parent process on demand so we can holdsystemd
* or initd from starting next service until sssd ininitialized.
* We use signals directly here because we don't have atevent
* context yet. */CatchSignal(SIGTERM, deamon_parent_sigterm);/* or exit when sssd monitor is terminated */waitpid(pid, &status, 0);/* return error if we didn't exited normally */ret = 1;if (WIFEXITED(status)) {/* but return our exit code otherwise */ret = WEXITSTATUS(status);}_exit(ret);}- }
- *ppid = getppid();
What's the point of making become_daemon() return a ppid if you assign it unconditionally after the fork() ?
Isn't it the same than calling getppid() in the caller function ? What am I missing ?
/* detach from the terminal */
setsid();
setsid();
/* chdir to / to be sure we're not on a remote filesystem */ errno = 0;@@ -93,8 +123,8 @@ void become_daemon(bool Fork) return; }
This ^^ remains indented odly.
/* Close fd's 0,1,2. Needed if started by rsh */close_low_fds();
- /* Close fd's 0,1,2. Needed if started by rsh */
- close_low_fds();
}
Simo.
On Thu, 2012-11-01 at 17:36 +0100, Pavel Březina wrote:
From 99c4a968889c4ee6d0f3015b160152da8c733960 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 26 Oct 2012 12:46:48 +0200 Subject: [PATCH 3/3] create pid file immediately after fork again
Related to https://fedorahosted.org/sssd/ticket/1357
We realized that sysv and systemd does not use pid file existence as a notification of finished initialization. Therefore, we create the pid file immediately after we fork to become daemon.
The comment would be clearer if it said the pidfile is created in server_setup() Also it doesn't say why you remove check_file on the pidfile ? Can you explain ?
Simo.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Pavel Březina -
Simo Sorce -
Stephen Gallagher