[SSSD] [PATCH] SSSD: Add a new command diag_cmd

Petr Cech pcech at redhat.com
Thu Nov 12 09:49:33 UTC 2015


On 11/11/2015 12:25 PM, Jakub Hrozek wrote:
> On Wed, Nov 11, 2015 at 11:07:46AM +0100, Petr Cech wrote:
>> >On 11/11/2015 09:32 AM, Jakub Hrozek wrote:
>>>>> > >>>Hi Jakub,
>>>>> > >>>
>>>>> > >>>I just sent the patch to the CI tests and they passed
>>>>> > >>>http://sssd-ci.duckdns.org/logs/job/32/63/summary.html
>>>>> > >>>
>>>>>>> > >>>> >Then I would prefer undocumented. It matches how we (don't) document the
>>>>>>> > >>>> >"command" option.
>>>>> > >>>I agree with little exception. I think it could be more clear if we write
>>>>> > >>>little documentation to commit message or to the code near the new option.
>>>>> > >>>But, how everybody wrote, we could leave man page without documentation.
>>> > >Good idea, I can move the documentation that was previously in the man
>>> > >page to the commit message, would that work for you?
>> >
>> >Yes, it works for me.
>> >
>> >I am going to 1/2 PTO for now.
>> >
>> >The last two things are that I would like to run it due to your reproducer.
>> >And there is a remark from Stephen Gallagher which we need resolve.
>> >
>> >However, CI tests passed. So if you're in a hurry with code review, please
>> >ask someone else.
>> >
>> >I can continue with review tomorrow.
> I'm not in hurry at all. Attached is a patch that adds a better commit
> message. We can discuss any details related to testing over IRC if you
> prefer.
Hi Jakub,

it works due to your reproducer. It is really need to have
setenforce == 1

CI tests passed:
http://sssd-ci.duckdns.org/logs/job/32/25/summary.html

Stephen Gallagher wrote (2015-09-11 11:32 AM):
There are problems inherent with passing the PID to the child process.
There's no guarantee that the process still exists. In the worst-case,
the PID could actually be reassigned to a new process and the output
you got back from something like pstack could be reading from a
different executable entirely.
---
I understand, it could be dangerous. But, this option is a little bit 
secret, we don't write about it in our man pages and so on. I hope it 
will be used only for debuging some hot cases.

There are only little remarks in patch.

=> ACK

Regrds

Petr

PS: I accepted that we have
# p = copy;
not something like:
# copy_ptr = copy;
How I suggested previous mail.

> 0001-SSSD-Add-a-new-command-diag_cmd.patch
>
>
>  From ee4135adf6669221de575ebc92e7b3aabba55ba9 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek<jhrozek at redhat.com>
> Date: Mon, 2 Nov 2015 11:41:31 +0100
> Subject: [PATCH] SSSD: Add a new command diag_cmd
>
> This command is an optional one that is run when a sbus ping times out
> and before a SIGKILL commans is sent.
                           ---^--- s -> d
>
> diag_cmd (string):
> A command that should be run for diagnostic purpose when an sbus timeout
> fails. The option value may contain %p which would be expanded for the
> process ID of the process that timed out
>
> Example:
>          pstack %p
> This setting would print the stackstrace of the command whose ping timed out.
>
> Default: not set.
> ---
>   src/confdb/confdb.h                  |   1 +
>   src/config/SSSDConfig/__init__.py.in |   1 +
>   src/config/SSSDConfigTest.py         |   1 +
>   src/config/etc/sssd.api.conf         |   1 +
>   src/monitor/monitor.c                | 215 +++++++++++++++++++++++++++++++----
>   5 files changed, 197 insertions(+), 22 deletions(-)
>
> diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
> index 37b5fd7c7629e2618a1699e3ffd58110171db605..0ef7268f9cdc2c18482bbf7b8dbe19d3ef6b7bbf 100644
> --- a/src/confdb/confdb.h
> +++ b/src/confdb/confdb.h
> @@ -71,6 +71,7 @@
>   #define CONFDB_MONITOR_DEFAULT_DOMAIN "default_domain_suffix"
>   #define CONFDB_MONITOR_OVERRIDE_SPACE "override_space"
>   #define CONFDB_MONITOR_USER_RUNAS "user"
> +#define CONFDB_MONITOR_PRE_KILL_CMD "diag_cmd"
>
>   /* Both monitor and domains */
>   #define CONFDB_NAME_REGEX   "re_expression"
> diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in
> index bf61c402796122050fa43cf41128faec4771c5d2..60129e6e7fbc96d11c539323346c22a7db6d7f23 100644
> --- a/src/config/SSSDConfig/__init__.py.in
> +++ b/src/config/SSSDConfig/__init__.py.in
> @@ -50,6 +50,7 @@ option_strings = {
>       'reconnection_retries' : _('Number of times to attempt connection to Data Providers'),
>       'fd_limit' : _('The number of file descriptors that may be opened by this responder'),
>       'client_idle_timeout' : _('Idle time before automatic disconnection of a client'),
> +    'diag_cmd' : _('The command to run when a service ping times out'),
>
>       # [sssd]
>       'services' : _('SSSD Services to start'),
> diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py
> index 45562214da5d227b45914abbcb298e043048adf5..abd4a39258e060f27db62eb2352450b6c405930c 100755
> --- a/src/config/SSSDConfigTest.py
> +++ b/src/config/SSSDConfigTest.py
> @@ -307,6 +307,7 @@ class SSSDConfigTestSSSDService(unittest.TestCase):
>               'reconnection_retries',
>               'fd_limit',
>               'client_idle_timeout',
> +            'diag_cmd',
>               'description']
>
>           self.assertTrue(type(options) == dict,
> diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf
> index 72abb8b3f427e9bcf3cf3dcaa67aaf4e3e50226c..0c03625bd5f0d3fbf8948971b015d4e8255f0009 100644
> --- a/src/config/etc/sssd.api.conf
> +++ b/src/config/etc/sssd.api.conf
> @@ -13,6 +13,7 @@ fd_limit = int, None, false
>   client_idle_timeout = int, None, false
>   force_timeout = int, None, false
>   description = str, None, false
> +diag_cmd = str, None, false
>
>   [sssd]
>   # Monitor service
> diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> index 89ac882d38652e071fdc619495c3252807527da0..5ff5f85527a6aa91e220ddd07e8c8738dba0e829 100644
> --- a/src/monitor/monitor.c
> +++ b/src/monitor/monitor.c
> @@ -116,6 +116,7 @@ struct mt_svc {
>       char *identity;
>       pid_t pid;
>
> +    char *diag_cmd;
>       int ping_time;
>       int kill_time;
>
> @@ -383,6 +384,176 @@ static int add_svc_conn_spy(struct mt_svc *svc)
>       return EOK;
>   }
>
> +static char *expand_diag_cmd(struct mt_svc *svc,
> +                             const char *template)
> +{
> +    TALLOC_CTX *tmp_ctx = NULL;
> +    char *copy;
> +    char *p;
> +    char *n;
> +    char *result = NULL;
> +    char action;
> +    char *res = NULL;
> +
> +    if (template == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Missing template.\n");
> +        return NULL;
> +    }
> +
> +    tmp_ctx = talloc_new(NULL);
> +    if (!tmp_ctx) return NULL;
> +
> +    copy = talloc_strdup(tmp_ctx, template);
> +    if (copy == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_strdup failed.\n");
> +        goto done;
> +    }
> +
> +    result = talloc_strdup(tmp_ctx, "");
> +    if (result == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_strdup failed.\n");
> +        goto done;
> +    }
> +
> +    p = copy;
> +    while ((n = strchr(p, '%')) != NULL) {
> +        *n = '\0';
> +        n++;
> +        if ( *n == '\0' ) {
> +            DEBUG(SSSDBG_CRIT_FAILURE,
> +                  "format error, single %% at the end of the template.\n");
> +            goto done;
> +        }
> +
> +        action = *n;
> +        switch (action) {
> +        case 'p':
> +            result = talloc_asprintf_append(result, "%s%d", p, svc->pid);
> +            break;
> +        default:
> +            DEBUG(SSSDBG_CRIT_FAILURE,
> +                  "format error, unknown template [%%%c].\n", *n);
> +            goto done;
> +        }
> +
> +        if (result == NULL) {
> +            DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf_append failed.\n");
> +            goto done;
> +        }
> +
> +        p = n + 1;
> +    }
> +
> +    result = talloc_asprintf_append(result, "%s", p);
> +    if (result == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf_append failed.\n");
> +        goto done;
> +    }
> +
> +    res = talloc_move(svc, &result);
> +done:
> +    talloc_zfree(tmp_ctx);
> +    return res;
> +}
> +
> +static void svc_child_info(struct mt_svc *svc, int wait_status)
> +{
> +    if (WIFEXITED(wait_status)) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "Child [%d] exited with code [%d]\n",
> +               svc->pid, WEXITSTATUS(wait_status));
> +    } else if (WIFSIGNALED(wait_status)) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "Child [%d] terminated with signal [%d]\n",
> +               svc->pid, WTERMSIG(wait_status));
> +    } else {
> +        DEBUG(SSSDBG_FATAL_FAILURE,
> +              "Child [%d] did not exit cleanly\n", svc->pid);
> +        /* Forcibly kill this child, just in case */
> +        kill(svc->pid, SIGKILL);
> +
> +        /* Let us get caught by another
> +         * call to the SIGCHLD handler
> +         */
> +    }
> +}
> +
> +static void svc_diag_cmd_exit_handler(int pid, int wait_status, void *pvt)
> +{
> +    struct mt_svc *svc = talloc_get_type(pvt, struct mt_svc);
> +
> +    svc_child_info(svc, wait_status);
> +}
> +
> +static void svc_run_diag_cmd(struct mt_svc *svc)
> +{
> +    pid_t pkc_pid;
> +    char **args;
> +    int ret;
> +    int debug_fd;
> +    char *diag_cmd;
> +    struct sss_child_ctx *diag_child_ctx;
> +
> +    if (svc->diag_cmd == NULL) {
> +        return;
> +    }
> +
> +    pkc_pid = fork();
> +    if (pkc_pid != 0) {
> +        /* parent, schedule SIGKILL */
> +
> +        ret = sss_child_register(svc,
> +                                 svc->mt_ctx->sigchld_ctx,
> +                                 pkc_pid,
> +                                 svc_diag_cmd_exit_handler,
> +                                 svc,
> +                                 &diag_child_ctx);
> +        if (ret != EOK) {
> +            DEBUG(SSSDBG_CRIT_FAILURE, "Cannot register child %d\n", pkc_pid);
> +            /* Try to go on ... */
> +        }
> +
> +        return;
> +    }
> +
> +    /* child, execute diagnostics */
> +    diag_cmd = expand_diag_cmd(svc, svc->diag_cmd);
> +    if (diag_cmd == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE,
> +              "Failed to expand [%s]\n", svc->diag_cmd);
> +        _exit(1);
> +    }
> +
> +    if (debug_level >= SSSDBG_TRACE_LIBS) {
> +        debug_fd = get_fd_from_debug_file();
> +        ret = dup2(debug_fd, STDERR_FILENO);
> +        if (ret == -1) {
> +            ret = errno;
> +            DEBUG(SSSDBG_MINOR_FAILURE,
> +                "dup2 failed for stderr [%d][%s].\n", ret, sss_strerror(ret));
> +            /* failure to redirect stderr is not fatal */
> +        }
> +
> +        ret = dup2(debug_fd, STDOUT_FILENO);
> +        if (ret == -1) {
> +            ret = errno;
> +            DEBUG(SSSDBG_MINOR_FAILURE,
> +                "dup2 failed for stdout [%d][%s].\n", ret, sss_strerror(ret));
> +            /* failure to redirect stdout is not fatal */
> +        }
> +    }
> +
> +    args = parse_args(diag_cmd);
> +    execvp(args[0], args);
> +
> +    /* If we are here, exec() has failed
> +     * Print errno and abort quickly */
> +    ret = errno;
> +    DEBUG(SSSDBG_FATAL_FAILURE,
> +          "Could not exec %s, reason: %s\n", svc->diag_cmd, strerror(ret));
> +    _exit(1);
> +}
> +
>   static int mark_service_as_started(struct mt_svc *svc)
>   {
>       struct mt_ctx *ctx = svc->mt_ctx;
> @@ -613,8 +784,10 @@ static int monitor_kill_service (struct mt_svc *svc)
>           return EOK;
>       }
>
> +    svc_run_diag_cmd(svc);
> +
>       /* Set up a timer to send SIGKILL if this process
> -     * doesn't exit within sixty seconds
> +     * doesn't exit within the configured interval
>        */
>       tv = tevent_timeval_current_ofs(svc->kill_time, 0);
>       svc->kill_timer = tevent_add_timer(svc->mt_ctx->ev,
> @@ -628,7 +801,6 @@ static int monitor_kill_service (struct mt_svc *svc)
>                 "Failed to allocate timed event: mt_svc_sigkill.\n");
>           /* We'll just have to hope that the SIGTERM succeeds */
>       }
> -
>       return EOK;
>   }
>
> @@ -1065,6 +1237,18 @@ static errno_t get_ping_config(struct mt_ctx *ctx, const char *path,
>             "Time between service pings for [%s]: [%d]\n",
>              svc->name, svc->ping_time);
>
> +    ret = confdb_get_string(ctx->cdb, svc, path,
> +                            CONFDB_MONITOR_PRE_KILL_CMD,
> +                            NULL, &svc->diag_cmd);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_CRIT_FAILURE,
> +              "Failed to get diagnostics command for %s\n", svc->name);
> +        return ret;
> +    }
> +    if (svc->diag_cmd) {
> +        DEBUG(SSSDBG_CONF_SETTINGS, "Diagnostics command: [%s]\n", svc->diag_cmd);
There are 83 characters.

> +    }
> +
>       ret = confdb_get_int(ctx->cdb, path,
>                            CONFDB_SERVICE_FORCE_TIMEOUT,
>                            MONITOR_DEF_FORCE_TIME, &svc->kill_time);
> @@ -2615,6 +2799,10 @@ static void ping_check(DBusPendingCall *pending, void *data)
>                      "Attempt [%d]\n",
>                      svc->name, svc->failed_pongs);
>               svc->failed_pongs++;
> +
> +            if (debug_level & SSSDBG_TRACE_LIBS) {
> +                svc_run_diag_cmd(svc);
> +            }
>               break;
>           }
>
> @@ -2765,26 +2953,9 @@ static void mt_svc_exit_handler(int pid, int wait_status, void *pvt)
>   {
>       struct mt_svc *svc = talloc_get_type(pvt, struct mt_svc);
>
> -
> -    if (WIFEXITED(wait_status)) {
> -        DEBUG(SSSDBG_OP_FAILURE,
> -              "Child [%s] exited with code [%d]\n",
> -               svc->name, WEXITSTATUS(wait_status));
> -    } else if (WIFSIGNALED(wait_status)) {
> -        DEBUG(SSSDBG_OP_FAILURE,
> -              "Child [%s] terminated with signal [%d]\n",
> -               svc->name, WTERMSIG(wait_status));
> -    } else {
> -        DEBUG(SSSDBG_FATAL_FAILURE,
> -              "Child [%s] did not exit cleanly\n", svc->name);
> -        /* Forcibly kill this child, just in case */
> -        kill(svc->pid, SIGKILL);
> -
> -        /* Return and let us get caught by another
> -         * call to the SIGCHLD handler
> -         */
> -        return;
> -    }
> +    DEBUG(SSSDBG_TRACE_LIBS,
> +          "SIGCHLD handler of service %s called\n", svc->name);
> +    svc_child_info(svc, wait_status);
>
>       /* Clear the kill_timer so we don't try to SIGKILL it after it's
>        * already gone.
> -- 2.4.3


More information about the sssd-devel mailing list