[PATCH]-1414-Improve syslog message when configuration cannot be loaded

[Ariel Barria]

https://fedorahosted.org/sssd/ticket/1414

I found a similar topic with ./SSSD -i.

[Jakub Hrozek]

On Mon, Jul 16, 2012 at 12:39:06AM -0500, Ariel Barria wrote:
> https://fedorahosted.org/sssd/ticket/1414
>
> I found a similar topic with ./SSSD -i.

There is at least one more place that should have been changed by adding
strerror which the original reporter complained about, which is the sss_log
call at src/monitor/monitor.c:2527

[Ariel Barria]

New patch attached.
> Date: Mon, 16 Jul 2012 12:54:25 +0200
> From: [e-mail]
> To: [e-mail]
> Subject: Re: [SSSD] [PATCH]-1414-Improve syslog message when configuration cannot be loaded
>
> On Mon, Jul 16, 2012 at 12:39:06AM -0500, Ariel Barria wrote:
> > https://fedorahosted.org/sssd/ticket/1414
> >
> > I found a similar topic with ./SSSD -i.
>
> There is at least one more place that should have been changed by adding
> strerror which the original reporter complained about, which is the sss_log
> call at src/monitor/monitor.c:2527
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Mon, Jul 16, 2012 at 09:48:42PM -0500, Ariel Barria wrote:
> New patch attached.
> > Date: Mon, 16 Jul 2012 12:54:25 +0200
> > From: [e-mail]
> > To: [e-mail]
> > Subject: Re: [SSSD] [PATCH]-1414-Improve syslog message when configuration cannot be loaded
> >
> > On Mon, Jul 16, 2012 at 12:39:06AM -0500, Ariel Barria wrote:
> > > https://fedorahosted.org/sssd/ticket/1414
> > >
> > > I found a similar topic with ./SSSD -i.
> >
> > There is at least one more place that should have been changed by adding
> > strerror which the original reporter complained about, which is the sss_log
> > call at src/monitor/monitor.c:2527

Thank you, Ariel, this is almost what I had in mind. Can you add two more
minor changes? It's mostly coding style, not functional changes. See below.
>    DEBUG(0,("Could not set up debug fn.\n"));
>    DEBUG(SSSDBG_FATAL_FAILURE,
>         ("Could not set up debug fn.\n"));

Statements like this don't have to be wrapped into two lines, if they
fit into 78 characters, it's be OK to leave it on one line.
>      talloc_free(cdb);
>      return EIO;
>  }
>
> @@ -653,8 +654,10 @@ int confdb_init(TALLOC_CTX *mem_ctx,
>      ret = ldb_connect(cdb->ldb, confdb_location, 0, NULL);
>      umask(old_umask);
>      if (ret != LDB_SUCCESS) {
>    DEBUG(0, ("Unable to open config database [%s]\n",
>              confdb_location));
>    DEBUG(SSSDBG_FATAL_FAILURE,
>         ("Unable to open config database"
>         " [%s], [%d] [%s].\n",
>          confdb_location, ret, strerror(ret)));

Same here.
>      talloc_free(cdb);
>      return EIO;
>  }
>
> diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> index 201d9fa8dd01977ff5455da053fff4c2bd4fcec5..91603310ab060181eb727bc05be199345f9d1b4f 100644
> --- a/src/monitor/monitor.c
> +++ b/src/monitor/monitor.c
> @@ -1381,14 +1381,17 @@ static errno_t load_configuration(TALLOC_CTX *mem_ctx,
>  cdb_file = talloc_asprintf(ctx, "%s/%s", DB_PATH, CONFDB_FILE);
>  if (cdb_file == NULL) {
>    DEBUG(0,("Out of memory, aborting!\n"));
>    DEBUG(SSSDBG_FATAL_FAILURE,
>         ("Out of memory, aborting!.\n"));

Same here.
>      ret = ENOMEM;
>      goto done;
>  }
>
>  ret = confdb_init(ctx, &ctx->cdb, cdb_file);
>  if (ret != EOK) {
>    DEBUG(0,("The confdb initialization failed\n"));
>    DEBUG(SSSDBG_FATAL_FAILURE,
>         ("The confdb initialization failed, [%d] [%s].\n",
>         ret,strerror(ret)));

Please put a space after a comma (",").
>      goto done;
>  }
>
> @@ -1405,25 +1408,31 @@ static errno_t load_configuration(TALLOC_CTX *mem_ctx,
>      ret = confdb_init(ctx, &ctx->cdb, cdb_file);
>      if (ret != EOK) {
>        DEBUG(0,("The confdb initialization failed\n"));
>        DEBUG(SSSDBG_FATAL_FAILURE,
>             ("The confdb initialization failed, [%d] [%s].\n",
>             ret,strerror(ret)));

Space again here.
>          goto done;
>      }
>
>      /* Load special entries */
>      ret = confdb_create_base(ctx->cdb);
>      if (ret != EOK) {
>        DEBUG(0, ("Unable to load special entries into confdb\n"));
>        DEBUG(SSSDBG_FATAL_FAILURE,
>             ("Unable to load special entries into confdb.\n"));
>        goto done;
>    }
> } else if (ret != EOK) {
>    DEBUG(0, ("Fatal error initializing confdb\n"));
>    DEBUG(SSSDBG_FATAL_FAILURE,
>         ("Fatal error initializing confdb, [%d] [%s].\n",
>         ret,strerror(ret)));

Space again.
>      goto done;
>  }
>  talloc_zfree(cdb_file);
>
>  ret = confdb_init_db(config_file, ctx->cdb);
>  if (ret != EOK) {
>    DEBUG(0, ("ConfDB initialization has failed [%s]\n",
>    DEBUG(SSSDBG_FATAL_FAILURE,
>         ("ConfDB initialization has failed [%s].\n",
>          strerror(ret)));
>    goto done;
> }
>
> @@ -2522,9 +2531,12 @@ int main(int argc, const char *argv[])
>                      "Cannot read config file %s, please check if permissions "
>                      "are 0600 and the file is owned by root.root", config_file);
>          } else {
>        DEBUG(1, ("Error loading configuration database: [%d]: %s",
>                  ret, strerror(ret)));
>        sss_log(SSS_LOG_ALERT, "Cannot load configuration database");
>        DEBUG(SSSDBG_FATAL_FAILURE,
>             ("Error loading configuration database: [%d]: %s",
>             ret, strerror(ret)));
>        sss_log(SSS_LOG_ALERT,
>               "Cannot load configuration database: [%d]: %s",
>               ret, strerror(ret));
>    }
>    return 4;
> }
>
> -- 
> 1.7.7.6

[Ariel Barria]

done, thanks for comments.
> Date: Tue, 17 Jul 2012 22:46:07 +0200
> From: [e-mail]
> To: [e-mail]
> Subject: Re: [SSSD] [PATCH]-1414-Improve syslog message when configuration cannot be loaded
>
> On Mon, Jul 16, 2012 at 09:48:42PM -0500, Ariel Barria wrote:
> > New patch attached.
> > > Date: Mon, 16 Jul 2012 12:54:25 +0200
> > > From: [e-mail]
> > > To: [e-mail]
> > > Subject: Re: [SSSD] [PATCH]-1414-Improve syslog message when configuration cannot be loaded
> > >
> > > On Mon, Jul 16, 2012 at 12:39:06AM -0500, Ariel Barria wrote:
> > > > https://fedorahosted.org/sssd/ticket/1414
> > > >
> > > > I found a similar topic with ./SSSD -i.
> > >
> > > There is at least one more place that should have been changed by adding
> > > strerror which the original reporter complained about, which is the sss_log
> > > call at src/monitor/monitor.c:2527
>
> Thank you, Ariel, this is almost what I had in mind. Can you add two more
> minor changes? It's mostly coding style, not functional changes. See below.
> >    DEBUG(0,("Could not set up debug fn.\n"));
> >    DEBUG(SSSDBG_FATAL_FAILURE,
> >         ("Could not set up debug fn.\n"));
>
> Statements like this don't have to be wrapped into two lines, if they
> fit into 78 characters, it's be OK to leave it on one line.
> >      talloc_free(cdb);
> >      return EIO;
> >  }
> >
> > @@ -653,8 +654,10 @@ int confdb_init(TALLOC_CTX *mem_ctx,
> >      ret = ldb_connect(cdb->ldb, confdb_location, 0, NULL);
> >      umask(old_umask);
> >      if (ret != LDB_SUCCESS) {
> >    DEBUG(0, ("Unable to open config database [%s]\n",
> >              confdb_location));
> >    DEBUG(SSSDBG_FATAL_FAILURE,
> >         ("Unable to open config database"
> >         " [%s], [%d] [%s].\n",
> >          confdb_location, ret, strerror(ret)));
>
> Same here.
> >      talloc_free(cdb);
> >      return EIO;
> >  }
> >
> > diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> > index 201d9fa8dd01977ff5455da053fff4c2bd4fcec5..91603310ab060181eb727bc05be199345f9d1b4f 100644
> > --- a/src/monitor/monitor.c
> > +++ b/src/monitor/monitor.c
> > @@ -1381,14 +1381,17 @@ static errno_t load_configuration(TALLOC_CTX *mem_ctx,
> >  cdb_file = talloc_asprintf(ctx, "%s/%s", DB_PATH, CONFDB_FILE);
> >  if (cdb_file == NULL) {
> >    DEBUG(0,("Out of memory, aborting!\n"));
> >    DEBUG(SSSDBG_FATAL_FAILURE,
> >         ("Out of memory, aborting!.\n"));
>
> Same here.
> >      ret = ENOMEM;
> >      goto done;
> >  }
> >
> >  ret = confdb_init(ctx, &ctx->cdb, cdb_file);
> >  if (ret != EOK) {
> >    DEBUG(0,("The confdb initialization failed\n"));
> >    DEBUG(SSSDBG_FATAL_FAILURE,
> >         ("The confdb initialization failed, [%d] [%s].\n",
> >         ret,strerror(ret)));
>
> Please put a space after a comma (",").
> >      goto done;
> >  }
> >
> > @@ -1405,25 +1408,31 @@ static errno_t load_configuration(TALLOC_CTX *mem_ctx,
> >      ret = confdb_init(ctx, &ctx->cdb, cdb_file);
> >      if (ret != EOK) {
> >        DEBUG(0,("The confdb initialization failed\n"));
> >        DEBUG(SSSDBG_FATAL_FAILURE,
> >             ("The confdb initialization failed, [%d] [%s].\n",
> >             ret,strerror(ret)));
>
> Space again here.
> >          goto done;
> >      }
> >
> >      /* Load special entries */
> >      ret = confdb_create_base(ctx->cdb);
> >      if (ret != EOK) {
> >        DEBUG(0, ("Unable to load special entries into confdb\n"));
> >        DEBUG(SSSDBG_FATAL_FAILURE,
> >             ("Unable to load special entries into confdb.\n"));
> >        goto done;
> >    }
> > } else if (ret != EOK) {
> >    DEBUG(0, ("Fatal error initializing confdb\n"));
> >    DEBUG(SSSDBG_FATAL_FAILURE,
> >         ("Fatal error initializing confdb, [%d] [%s].\n",
> >         ret,strerror(ret)));
>
> Space again.
> >      goto done;
> >  }
> >  talloc_zfree(cdb_file);
> >
> >  ret = confdb_init_db(config_file, ctx->cdb);
> >  if (ret != EOK) {
> >    DEBUG(0, ("ConfDB initialization has failed [%s]\n",
> >    DEBUG(SSSDBG_FATAL_FAILURE,
> >         ("ConfDB initialization has failed [%s].\n",
> >          strerror(ret)));
> >    goto done;
> > }
> >
> > @@ -2522,9 +2531,12 @@ int main(int argc, const char *argv[])
> >                      "Cannot read config file %s, please check if permissions "
> >                      "are 0600 and the file is owned by root.root", config_file);
> >          } else {
> >        DEBUG(1, ("Error loading configuration database: [%d]: %s",
> >                  ret, strerror(ret)));
> >        sss_log(SSS_LOG_ALERT, "Cannot load configuration database");
> >        DEBUG(SSSDBG_FATAL_FAILURE,
> >             ("Error loading configuration database: [%d]: %s",
> >             ret, strerror(ret)));
> >        sss_log(SSS_LOG_ALERT,
> >               "Cannot load configuration database: [%d]: %s",
> >               ret, strerror(ret));
> >    }
> >    return 4;
> > }
> >
> > -- 
> > 1.7.7.6
>
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Tue, Jul 17, 2012 at 10:36:17PM -0500, Ariel Barria wrote:
> done, thanks for comments.

This patch was left behind for unknown reason, I'm sorry. It looks good
to me now, but needs rebasing. Can you rebase, please, Ariel?

[Ariel Barria]

no problem, patch left alone the change with  requesting in the ticket.
> Date: Wed, 20 Mar 2013 14:21:46 +0100
> From: [e-mail]
> To: [e-mail]
> Subject: Re: [SSSD] [PATCH]-1414-Improve syslog message when configuration cannot be loaded
>
> On Tue, Jul 17, 2012 at 10:36:17PM -0500, Ariel Barria wrote:
> > done, thanks for comments.
>
> This patch was left behind for unknown reason, I'm sorry. It looks good
> to me now, but needs rebasing. Can you rebase, please, Ariel?
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Wed, Mar 27, 2013 at 04:15:52PM -0500, Ariel Barria wrote:
> no problem, patch left alone the change with  requesting in the ticket.

Thank you!

Ack.

[Jakub Hrozek]

On Tue, Apr 02, 2013 at 05:19:03PM +0200, Jakub Hrozek wrote:
> On Wed, Mar 27, 2013 at 04:15:52PM -0500, Ariel Barria wrote:
> > no problem, patch left alone the change with  requesting in the ticket.
>
> Thank you!
>
> Ack.

Pushed to master.