[SSSD] [PATCH] IFP: touch config when changing debug level temporarily

Jakub Hrozek jhrozek at redhat.com
Wed Jul 9 08:41:13 UTC 2014


On Tue, Jul 08, 2014 at 10:23:23AM +0200, Pavel Březina wrote:
> On 07/07/2014 02:14 PM, Pavel Reichl wrote:
> >On Wed, 2014-07-02 at 13:10 +0200, Pavel Březina wrote:
> >>This fixes a bug found by Jan Šafránek. The debug level was not restored
> >>after sssd restart because the time of last modification of sssd.conf
> >>was older than config.ldb.
> >>_______________________________________________
> >>sssd-devel mailing list
> >>sssd-devel at lists.fedorahosted.org
> >>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> >Hi Pavel,
> >
> >
> >>      ret = change_debug_level_tmp(ctx->rctx->cdb, name, type, arg_new_level);
> >>+    if (ret != EOK) {
> >>+        goto done;
> >>+    }
> >>+
> >>+    errno = 0;
> >
> >I just wonder if there is any need to initialize errno as it is set by utime() in case of failure and we directly check return value of utime().
> >I'm not sure but I don't think it's a common practice in SSSD codebase to always initialize errno.
> 
> You are correct sir. I removed it also from sss_debuglevel tool.

Setting errno to 0 is a common practice if you rely on the value of
errno for checks. strtol is one example where you absolutely need to set
errno to 0. Looks like with utime you don't, though..

In general, errno is a poor way of communicating error codes..

> 
> >
> >I think it would be better to replace this line with comment like you put in commit message, something like
> >
> >/* Touch configuration file to make sure debug level is reloaded. */
> >
> 
> Done.
> 
> >>+    if (utime(CONFDB_DEFAULT_CONFIG_FILE, NULL) == -1) {
> >>+        ret = errno;
> >>+        goto done;
> >>+    }
> >>+
> >>+    ret = EOK;
> >
> >Thanks,
> >
> >PR
> >
> >
> >_______________________________________________
> >sssd-devel mailing list
> >sssd-devel at lists.fedorahosted.org
> >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> 
> Patches are attached.
> 

> From 39c19102d7e16a0b0a053c9f8913786533a90bcf Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> Date: Wed, 2 Jul 2014 11:14:42 +0200
> Subject: [PATCH 1/2] IFP: touch config when changing debug level temporarily
> 
> If we want the debug level to switch back to the value from
> configuration file we need to touch sssd.conf so it is reloaded.
> ---
>  src/responder/ifp/ifp_components.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/src/responder/ifp/ifp_components.c b/src/responder/ifp/ifp_components.c
> index e5fa06cdd23fa9dcb3e3601a670a73857e4fc33d..c8ad7673c290658e0c6c6273bce5e62bf88e8383 100644
> --- a/src/responder/ifp/ifp_components.c
> +++ b/src/responder/ifp/ifp_components.c
> @@ -21,6 +21,8 @@
>  #include <string.h>
>  #include <talloc.h>
>  #include <signal.h>
> +#include <errno.h>
> +#include <utime.h>
>  
>  #include "config.h"
>  #include "confdb/confdb.h"
> @@ -708,6 +710,17 @@ int ifp_component_change_debug_level_tmp(struct sbus_request *dbus_req,
>      }
>  
>      ret = change_debug_level_tmp(ctx->rctx->cdb, name, type, arg_new_level);
> +    if (ret != EOK) {
> +        goto done;
> +    }
> +
> +    /* Touch configuration file to make sure debug level is reloaded. */
> +    if (utime(CONFDB_DEFAULT_CONFIG_FILE, NULL) == -1) {
> +        ret = errno;
> +        goto done;
> +    }
> +
> +    ret = EOK;
>  
>  done:
>      if (ret != EOK) {
> -- 
> 1.7.11.7
> 

> From d15ad6028aae93560bdfc6e9a37a3af7edbb2431 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> Date: Tue, 8 Jul 2014 10:22:27 +0200
> Subject: [PATCH 2/2] sss_debuglevel: remove unnecessary errno
> 
> ---
>  src/tools/sss_debuglevel.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/src/tools/sss_debuglevel.c b/src/tools/sss_debuglevel.c
> index c38591cfaf66e67c198bf83ad431df5c3e11e74b..f423733d1134e166dca5cf57dd1dd1e1039deb82 100644
> --- a/src/tools/sss_debuglevel.c
> +++ b/src/tools/sss_debuglevel.c
> @@ -190,7 +190,6 @@ errno_t set_debug_level(struct debuglevel_tool_ctx *tool_ctx,
>       * Change atime and mtime of sssd.conf,
>       * so the configuration can be restored on next start.
>       */
> -    errno = 0;
>      if (utime(config_file, NULL) == -1 ) {
>          err = errno;
>          DEBUG(SSSDBG_MINOR_FAILURE, "Unable to change mtime of \"%s\": %s\n",
> -- 
> 1.7.11.7
> 

> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel




More information about the sssd-devel mailing list