[PATCHES] support local overrides

[Pavel Březina]

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

If you have any idea how to improve manual page, please, share it.

[Jakub Hrozek]

On Fri, Jul 24, 2015 at 01:08:17PM +0200, Pavel Březina wrote:
> https://fedorahosted.org/sssd/ticket/2584
>
> If you have any idea how to improve manual page, please, share it.

Hi,

please see my comments inline.
> From 58e7ac9f61c9dcc33e14a255daf026b563f06a8c Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Fri, 24 Jul 2015 09:55:28 +0200
> Subject: [PATCH 1/3] SYSDB: prepare for LOCAL view
>
> Objects doesn't have to have overrideDN specified when using LOCAL view.
> Since the view is not stored on the server we do not want to contact
> LDAP therefore we special case LOCAL view saying that it is OK that
> this attribute is missing.
>
> Preparation for:
> https://fedorahosted.org/sssd/ticket/2584
>
> src/db/sysdb.h                      |  1 +
>  src/db/sysdb_views.c                |  7 +++++++
>  src/tests/cmocka/test_sysdb_views.c | 29 +++++++++++++++++++++++++++++
>  3 files changed, 37 insertions(+)
>
> diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> index 0f745ccb1a646d77ba4ad3d714d5f4dce0a51211..7b18bbbe3f9249e59c4ffc6ee6ddcacebddf7e9f 100644
> --- a/src/db/sysdb.h
> +++ b/src/db/sysdb.h
> @@ -160,6 +160,7 @@
>  #define SYSDB_VIEW_CLASS "view"
>  #define SYSDB_VIEW_NAME "viewName"
>  #define SYSDB_DEFAULT_VIEW_NAME "default"
> +#define SYSDB_LOCAL_VIEW_NAME "LOCAL"

Please put a comment here that this name is reserved for client-side
overrides. You can also move the definition to the top or bottom of the
view-related options, perhaps also with DEFAULT_VIEW_NAME, but that's
less important to me.
> #define SYSDB_OVERRIDE_CLASS "overrride"
>  #define SYSDB_OVERRIDE_ANCHOR_UUID "overrideAnchorUUID"
>  #define SYSDB_OVERRIDE_USER_CLASS "userOverride"

[...]
> diff --git a/src/tests/cmocka/test_sysdb_views.c b/src/tests/cmocka/test_sysdb_views.c
> index 1fb598219e9ee581e465ddbb32ba9f2544600c26..19c04d58e47aa400aee4745dc40f937497081d10 100644
> --- a/src/tests/cmocka/test_sysdb_views.c
> +++ b/src/tests/cmocka/test_sysdb_views.c
> @@ -275,6 +275,33 @@ void test_sysdb_add_overrides_to_object(void **state)
>      assert_int_equal(ldb_val_string_cmp(&el->values[1], "OVERRIDEKEY2"), 0);
>  }
>
> +void test_sysdb_add_overrides_to_object_local(void **state)
> +{
> int ret;
> struct ldb_message *orig;
> struct ldb_message_element *el;
> char *tmp_str;
> struct sysdb_test_ctx *test_ctx = talloc_get_type_abort(*state,
>                                                     struct sysdb_test_ctx);
> orig = ldb_msg_new(test_ctx);
> assert_non_null(orig);
> tmp_str = talloc_strdup(orig,  "ORIGNAME");
> ret = ldb_msg_add_string(orig, SYSDB_NAME, tmp_str);
> assert_int_equal(ret, EOK);
> tmp_str = talloc_strdup(orig,  "ORIGGECOS");
> ret = ldb_msg_add_string(orig, SYSDB_GECOS, tmp_str);
> assert_int_equal(ret, EOK);
> test_ctx->domain->has_views = true;
> test_ctx->domain->view_name = "LOCAL";
> ret = sysdb_add_overrides_to_object(test_ctx->domain, orig, NULL, NULL);
> assert_int_equal(ret, EOK);
>
> +}

It would also be nice to add a negative test to check that a non-local
override requires overrideDN ?
> From 5f18058a302c5ac3901dd5694724cc9a38adef36 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Wed, 22 Jul 2015 10:02:02 +0200
> Subject: [PATCH 2/3] TOOLS: add common command framework
>
> Add general framework to simplify creating "cmd COMMAND [OPTIONS...]"
> style tools.
>
> Preparation for:
> https://fedorahosted.org/sssd/ticket/2584
>
> Makefile.am                  |   2 +
>  src/tools/common/sss_tools.c | 407 +++++++++++++++++++++++++++++++++++++++++++
>  src/tools/common/sss_tools.h | 103 +++++++++++
>  3 files changed, 512 insertions(+)
>  create mode 100644 src/tools/common/sss_tools.c
>  create mode 100644 src/tools/common/sss_tools.h
>
> diff --git a/Makefile.am b/Makefile.am
> index b8cbc6df23ded1edb945a709b6dbe1c44eb54017..e2c919b9adaf69f1ea6fde7e4a3e1746ccdcbedf 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -445,6 +445,7 @@ SSSD_TOOLS_OBJ = \
>      src/tools/tools_util.c \
>      src/tools/files.c \
>      src/tools/selinux.c \
> src/tools/common/sss_tools.c \
> src/util/nscd.c

Please add $(NULL) to the end of the list.
> SSSD_LCL_TOOLS_OBJ = \
> @@ -641,6 +642,7 @@ dist_noinst_HEADERS = \
>      src/lib/idmap/sss_idmap_private.h \
>      src/lib/sifp/sss_sifp_private.h \
>      src/tests/cmocka/test_utils.h \
> src/tools/common/sss_tools.h \
> $(NULL)
>
> diff --git a/src/tools/common/sss_tools.c b/src/tools/common/sss_tools.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..1c26e3b520743f41ed9376c52e8ac18736d73daa
> --- /dev/null
> +++ b/src/tools/common/sss_tools.c
> @@ -0,0 +1,407 @@
> +/*
> Authors:
>    Pavel Březina <[e-mail]>
> Copyright (C) 2015 Red Hat
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 3 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program.  If not, see http://www.gnu.org/licenses/.
>
> +*/
>
> +#include <talloc.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <popt.h>
>
> +#include "config.h"
> +#include "util/util.h"
> +#include "confdb/confdb.h"
> +#include "db/sysdb.h"
> +#include "tools/common/sss_tools.h"
>
> +static void sss_tool_common_opts(struct sss_tool_ctx *tool_ctx,
>                             int *argc, const char **argv)
>
> +{
> poptContext pc;
> int debug = SSSDBG_DEFAULT;
> int orig_argc = *argc;
> int opt;
> struct poptOption options[] = {
>    {"debug", '\0', POPT_ARG_INT | POPT_ARGFLAG_STRIP, &debug,
>        0, _("The debug level to run with"), NULL },
>    POPT_TABLEEND
> };
> pc = poptGetContext(argv[0], orig_argc, argv, options, 0);
> while((opt = poptGetNextOpt(pc)) != -1) {

Put a space between while and "("
>    /* do nothing */
> }
> /* Strip --debug and --config from arguments. We will discard_const here,

~~~~~~~~
                There is no --config anymore
> * since it is not worth the trouble to convert it back and forth. */
> *argc = poptStrippedArgv(pc, orig_argc, discard_const_p(char *, argv));
> DEBUG_CLI_INIT(debug);
> poptFreeContext(pc);
>
> +}
>
> +static errno_t sss_tool_confdb_init(TALLOC_CTX *mem_ctx,
>                                struct confdb_ctx **_confdb,
>                                char **_path)

~~~~~~~~~~~~~
I don't think returning path should be part of the API. It's really an
internal detail of the confdb.
> +{
> struct confdb_ctx *confdb;
> char *path;
> errno_t ret;
> path = talloc_asprintf(mem_ctx, "%s/%s", DB_PATH, CONFDB_FILE);
> if (path == NULL) {
>    return ENOMEM;
> }
> ret = confdb_init(mem_ctx, &confdb, path);
> if (ret != EOK) {
>    DEBUG(SSSDBG_CRIT_FAILURE,
>          "Could not initialize connection to the confdb\n");
>    talloc_free(path);
>    return ret;
> }
> if (_confdb != NULL) {
>    *_confdb = confdb;
> }
> if (_path != NULL) {
>    *_path = path;
> }
> return EOK;
>
> +}

[...]
> +static errno_t sss_tool_domains_init(TALLOC_CTX *mem_ctx,
>                                 struct confdb_ctx *confdb,
>                                 struct sss_domain_info **_domains)
>
> +{
> struct sss_domain_info *domains;
> struct sss_domain_info *dom;
> errno_t ret;
> ret = confdb_get_domains(confdb, &domains);
> if (ret != EOK) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to setup domains [%d]: %s\n",
>                               ret, sss_strerror(ret));
>    return ret;
> }
> ret = sysdb_init(mem_ctx, domains, false);
> SYSDB_VERSION_ERROR(ret);
> if (ret != EOK) {
>    DEBUG(SSSDBG_CRIT_FAILURE,
>          "Could not initialize connection to the sysdb\n");
>    return ret;
> }
> for (dom = domains; dom != NULL; dom = get_next_domain(dom, false)) {

~~~~~~~
      This looks like a bug, you can never descend into a subdomain with
      descend=false

btw (unrelated to these patches) it seems strange to require the user of
the sysdb database to connect to the db and then separately initialize
subdomains...I would vote for merging that code..
>    if (!IS_SUBDOMAIN(dom)) {
>        /* Update list of subdomains for this domain */
>        ret = sysdb_update_subdomains(dom);
>        if (ret != EOK) {
>            DEBUG(SSSDBG_MINOR_FAILURE,
>                  "Failed to update subdomains for domain %s.\n",
>                  dom->name);
>        }
>    }
> }
> for (dom = domains; dom != NULL; dom = get_next_domain(dom, false)) {

~~~~~
        I think you should use descend="true" here as well.
>    ret = sss_names_init(mem_ctx, confdb, dom->name, &dom->names);
>    if (ret != EOK) {
>        DEBUG(SSSDBG_CRIT_FAILURE, "sss_names_init() failed\n");
>        return ret;
>    }
> }
> *_domains = domains;
> return ret;
>
> +}
>
> +struct sss_tool_ctx * sss_tool_init(TALLOC_CTX *mem_ctx,

~~
We don't normally put a space between asterisk and identifier.
>                                int *argc, const char **argv)
>
> +{
> struct sss_tool_ctx *tool_ctx;
> errno_t ret;

[...]
> +int sss_tool_usage(const char *tool_name,
>               struct sss_route_cmd *commands)
>
> +{
> int i;
> fprintf(stderr, "Usage:\n%s COMMAND COMMAND-ARGS\n\n", tool_name);
> fprintf(stderr, "Available commands:\n");

Can you make the strings translatable with _(foo) ?
> for (i = 0; commands[i].command != NULL; i++) {
>    fprintf(stderr, "* %s\n", commands[i].command);
> }
> return EXIT_FAILURE;
>
> +}

[...]
> +int sss_tool_popt_ex(struct sss_cmdline *cmdline,
>                 struct poptOption *options,
>                 enum sss_tool_opt require_option,
>                 sss_popt_fn popt_fn,
>                 void *popt_fn_pvt,
>                 const char *fopt_name,
>                 const char *fopt_help,
>                 const char **_fopt)
>
> +{
> char *help;
> poptContext pc;
> int ret;
> /* Create help option string. We always need to append command name since
> * we use POPT_CONTEXT_KEEP_FIRST. */
> if (fopt_name == NULL) {
>    help = talloc_asprintf(NULL, "%s %s [OPTIONS...]",
>                           cmdline->exec, cmdline->command);

I think the OPTIONS string should also be made translatable...I guess
you need to add another format expanstion instead:
        help = talloc_asprintf(NULL, "%s %s %s",
                               cmdline->exec, cmdline->command,
                               _("[OPTIONS...]"));
> } else {
>    help = talloc_asprintf(NULL, "%s %s %s [OPTIONS...]",
>                           cmdline->exec, cmdline->command, fopt_name);

Same here.
> }
> if (help == NULL) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "tallos_asprintf() failed\n");

typo: tallos.

Also please set ret here or maybe even better, just return, because "pc"
is not defined at this point in the done handler yet.
>    goto done;
> }
> /* Create popt context. */
> pc = poptGetContext(cmdline->exec, cmdline->argc, cmdline->argv,
>                    options, POPT_CONTEXT_KEEP_FIRST);

~~~~~~~~~~~~~~~~~~~~~~~

What does this flag do? popt.h is not exactly explanatory..
> poptSetOtherOptionHelp(pc, help);
> /* Parse options. Invoke custom function if provided. If no parsing
> * function is provided, print error on unknown option. */
> while((ret = poptGetNextOpt(pc)) != -1) {

~~~~
Missing space.
>    if (popt_fn != NULL) {
>        ret = popt_fn(pc, ret, popt_fn_pvt);
>        if (ret != EOK) {
>            ret = EXIT_FAILURE;
>            goto done;
>        }
>    } else {
>        fprintf(stderr, "Invalid option %s: %s\n\n",
>                poptBadOption(pc, 0), poptStrerror(ret));

Translation again..
>        poptPrintHelp(pc, stderr, 0);
>        ret = EXIT_FAILURE;
>        goto done;
>    }
> }
> /* Parse free option which is always required if requested. */
> if (_fopt != NULL) {
>    *_fopt = poptGetArg(pc);
>    if (*_fopt == NULL) {
>        fprintf(stderr, "Missing option: %s\n\n", fopt_help);

Missing translation.
>        poptPrintHelp(pc, stderr, 0);
>        ret = EXIT_FAILURE;
>        goto done;
>    }
>    /* No more arguments expected. If something follows it is an error. */
>    if (poptGetArg(pc)) {
>        fprintf(stderr, "Only one free argument is expected!\n\n");

Missing translation.
>        poptPrintHelp(pc, stderr, 0);
>        ret = EXIT_FAILURE;
>        goto done;
>    }
> }
> /* If at least one option is required and not provided, print error. */
> if (require_option == SSS_TOOL_OPT_REQUIRED
>        && ((_fopt != NULL && cmdline->argc < 2) || cmdline->argc < 1)) {
>    fprintf(stderr, "At least one option is required!\n\n");

Missing translation.
>    poptPrintHelp(pc, stderr, 0);
>    ret = EXIT_FAILURE;
>    goto done;
> }
> ret = EXIT_SUCCESS;
>
> +done:
> poptFreeContext(pc);
> talloc_free(help);
> return ret;
>
> +}
>
> +int sss_tool_popt(struct sss_cmdline *cmdline,
>              struct poptOption *options,
>              enum sss_tool_opt require_option,
>              sss_popt_fn popt_fn,
>              void *popt_fn_pvt)
>
> +{
> return sss_tool_popt_ex(cmdline, options, require_option,
>                        popt_fn, popt_fn_pvt, NULL, NULL, NULL);
>
> +}
>
> +int sss_tool_main(int argc, const char **argv,
>              struct sss_route_cmd *commands,
>              void *pvt)
>
> +{
> struct sss_tool_ctx *tool_ctx;
> uid_t uid;
> int ret;
> uid = getuid();
> if (uid != 0) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "Running under %d, must be root\n", uid);

I think here it should be either root or sssd user. Even better might be
to try and gracefully fail if opening the databases fail -- I don't mind
which solution you choose.
>    ERROR("%1$s must be run as root\n", argv[0]);
>    return EXIT_FAILURE;
> }
> tool_ctx = sss_tool_init(NULL, &argc, argv);
> if (tool_ctx == NULL) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to create tool context\n");
>    return EXIT_FAILURE;
> }
> ret = sss_tool_route(argc, argv, tool_ctx, commands, pvt);
> talloc_free(tool_ctx);
> return ret;
>
> +}

[...]
> --- /dev/null
> +++ b/src/tools/common/sss_tools.h
> @@ -0,0 +1,103 @@
> +/*
> Authors:
>    Pavel Březina <[e-mail]>
> Copyright (C) 2015 Red Hat
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 3 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program.  If not, see http://www.gnu.org/licenses/.
>
> +*/
>
> +#ifndef SRC_TOOLS_COMMON_SSS_TOOLS_H_
> +#define SRC_TOOLS_COMMON_SSS_TOOLS_H_

I don't really like the autogenerated include guards. Elsewhere in SSSD
we use just the filename -- _filename_h_
> +#include <talloc.h>
> +#include <popt.h>
>
> +#include "confdb/confdb.h"
>
> +struct sss_tool_ctx {
> struct confdb_ctx *confdb;
> char *confdb_path;
> char *default_domain;
> struct sss_domain_info *domains;

I wonder if the tool_ctx must be open for consumers? Currently only
domains is used, maybe we could get away with a getter and an opaque
structure? We can always make the structure public in future, but if
it's public from the start, then it's easier to add cruft..
> +};
>
> +struct sss_tool_ctx * sss_tool_init(TALLOC_CTX *mem_ctx,

~~
Extra space.
>                                int *argc, const char **argv);
>
> +struct sss_cmdline {
> const char *exec; /* argv[0] */
> const char *command; /* command name */
> int argc; /* rest of arguments */
> const char **argv;
>
> +};

Can you define the structure in the source and make it opaque?
> +typedef int
> +(*sss_route_fn)(struct sss_cmdline *cmdline,
>            struct sss_tool_ctx *tool_ctx,
>            void *pvt);
>
> +struct sss_route_cmd {
> const char *command;
> sss_route_fn fn;
>
> +};
>
> +int sss_tool_usage(const char *tool_name,
>               struct sss_route_cmd *commands);
>
> +int sss_tool_route(int argc, const char **argv,
>               struct sss_tool_ctx *tool_ctx,
>               struct sss_route_cmd *commands,
>               void *pvt);
>
> +typedef int (*sss_popt_fn)(poptContext pc, char option, void *pvt);
>
> +enum sss_tool_opt {
> SSS_TOOL_OPT_REQUIRED,
> SSS_TOOL_OPT_OPTIONAL
>
> +};
>
> +int sss_tool_popt_ex(struct sss_cmdline *cmdline,
>                 struct poptOption *options,
>                 enum sss_tool_opt require_option,
>                 sss_popt_fn popt_fn,
>                 void *popt_fn_pvt,
>                 const char *free_opt_name,
>                 const char *free_opt_help,
>                 const char **_free_opt);
>
> +int sss_tool_popt(struct sss_cmdline *cmdline,
>              struct poptOption *options,
>              enum sss_tool_opt require_option,
>              sss_popt_fn popt_fn,
>              void *popt_fn_pvt);
>
> +int sss_tool_main(int argc, const char **argv,
>              struct sss_route_cmd *commands,
>              void *pvt);
>
> +enum sss_tool_domain {
> SSS_TOOL_DOMAIN_REQUIRED,
> SSS_TOOL_DOMAIN_OPTIONAL
>
> +};
>
> +int sss_tool_parse_name(TALLOC_CTX *mem_ctx,
>                    struct sss_tool_ctx *tool_ctx,
>                    const char *input,
>                    enum sss_tool_domain require_domain,

Do we need require_domain now that you added the getpwnam?
>                    const char **_username,
>                    struct sss_domain_info **_domain);
> +#endif /* SRC_TOOLS_COMMON_SSS_TOOLS_H_ */
>
> 2.1.0
> From f451abca1214d16ed4b80ee7c3db80b8c9eb53b3 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Fri, 24 Jul 2015 09:58:11 +0200
> Subject: [PATCH 3/3] TOOLS: add sss_override for local overrides
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2584
>
> Makefile.am                |   9 +
>  src/man/Makefile.am        |   1 +
>  src/man/sss_override.8.xml | 106 +++++++
>  src/tools/sss_override.c   | 698 +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 814 insertions(+)
>  create mode 100644 src/man/sss_override.8.xml
>  create mode 100644 src/tools/sss_override.c
>
> diff --git a/Makefile.am b/Makefile.am
> index e2c919b9adaf69f1ea6fde7e4a3e1746ccdcbedf..722eeb4737250e14745f346c6b809863a33d0f07 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -117,6 +117,7 @@ sbin_PROGRAMS = \
>      sss_groupshow \
>      sss_cache \
>      sss_debuglevel \
> sss_override \

Please also append NULL to the list.
>  sss_seed
>
> sssdlibexec_PROGRAMS = \
> @@ -1296,6 +1297,14 @@ sss_signal_LDADD = \
>      $(SSSD_INTERNAL_LTLIBS) \
>      $(NULL)
>
> +sss_override_SOURCES = \
> src/tools/sss_override.c \
> $(SSSD_TOOLS_OBJ)

Also end this list with $(NULL)
> +sss_override_LDADD = \
> $(TOOLS_LIBS) \
> $(SSSD_INTERNAL_LTLIBS)

And here
> +sss_override_CFLAGS = $(AM_CFLAGS)

And here (yes I know it's not consistent with the other tools, but in
new additions to Makefile.am, that's what we try to do.)
> if BUILD_SUDO
>  sss_sudo_cli_SOURCES = \
>      src/sss_client/common.c \
> diff --git a/src/man/Makefile.am b/src/man/Makefile.am
> index 1ef1da48cce74f7d1ad77e3751ee6ac3450f0259..70cadf5951f56b78ff0bfbcb303e255478af5fec 100644
> --- a/src/man/Makefile.am
> +++ b/src/man/Makefile.am
> @@ -51,6 +51,7 @@ man_MANS = \
>      sssd-krb5.5 sssd-simple.5 \
>      sssd_krb5_locator_plugin.8 sss_groupshow.8 \
>      pam_sss.8 sss_obfuscate.8 sss_cache.8 sss_debuglevel.8 sss_seed.8 \
> sss_override.8

The new man page must be added to src/man/po/po4a.cfg
>  $(NULL)
>
> if BUILD_SAMBA
> diff --git a/src/man/sss_override.8.xml b/src/man/sss_override.8.xml
> new file mode 100644
> index 0000000000000000000000000000000000000000..1ebd5adc18434cccf7f9bfc8e289947dde9bb8f5
> --- /dev/null
> +++ b/src/man/sss_override.8.xml
> @@ -0,0 +1,106 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!DOCTYPE reference PUBLIC "-//OASIS//DTD DocBook V4.4//EN"
> +"http://www.oasis-open.org/docbook/xml/4.4/docbookx.dtd">
> +<reference>
> +<title>SSSD Manual pages</title>
> +<refentry>
> <xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="include/upstream.xml" />
>
> <refmeta>
>    <refentrytitle>sss_override</refentrytitle>
>    <manvolnum>8</manvolnum>
>
> </refmeta>
>
> <refnamediv id='name'>
>    <refname>sss_override</refname>
>    <refpurpose>create client-side overrides of user and object attributes</refpurpose>

In general I wonder if client-side is well understood by admins (as
opposed to local or just saying stored on the local machine)
> </refnamediv>
>
> <refsynopsisdiv id='synopsis'>
>    <cmdsynopsis>
>        <command>sss_debuglevel</command>

~~~~~~~~~~~~~~
Copy-n-paste bug :-)
>        <arg choice='plain'><replaceable>COMMAND</replaceable></arg>
>        <arg choice='opt'>
>            <replaceable>options</replaceable>
>        </arg>
>    </cmdsynopsis>
>
> </refsynopsisdiv>
>
> <refsect1 id='description'>
>    <title>DESCRIPTION</title>
>    <para>
>        <command>sss_override</command> enables to create a client-side
>        view and allows to change selected values of specific user
>        and groups. This change takes effect only on client machine. 

~~~~~~~~~~~~~~~
Here I think we should definitely say local.
>    </para>
>
> </refsect1>

We should say that overriding attributes of users and groups only is
supported (IIRC we have an RFE somewhere to override netgroups
per-client...)
> <refsect1 id='commands'>
>    <title>AVAILABLE COMMANDS</title>

We should say that in all commands, NAME refers to the name of the
original object.
>    <variablelist remap='IP'>
>        <cmdsynopsis>
>            <command>user-add</command>
>            <arg choice='plain'><replaceable>NAME</replaceable></arg>
>            <arg choice='opt'>
>                <replaceable>options</replaceable>
>            </arg>
>        </cmdsynopsis>
>        <varlistentry>
>            <term>
>                <option>user-add</option>
>                <emphasis>NAME</emphasis>
>                <optional><option>-n,--name</option> NAME</optional>
>                <optional><option>-u,--uid</option> UID</optional>
>                <optional><option>-g,--gid</option> GID</optional> 
>                <optional><option>-h,--home</option> HOME</optional>
>                <optional><option>-s,--shell</option> SHELL</optional>
>                <optional><option>-c,--gecos</option> GECOS</optional>
>            </term>
>            <listitem>
>                <para>
>                    Override attributes of an user.
>                </para>
>            </listitem>
>        </varlistentry>
>        <varlistentry>
>            <term>
>                <option>user-del</option>
>                <emphasis>NAME</emphasis>
>            </term>
>            <listitem>
>                <para>
>                    Remove user overrides.
>                </para>
>            </listitem>
>        </varlistentry>
>        <varlistentry>
>            <term>
>                <option>group-add</option>
>                <emphasis>NAME</emphasis>
>                <optional><option>-n,--name</option> NAME</optional>
>                <optional><option>-g,--gid</option> GID</optional> 
>            </term>
>            <listitem>
>                <para>
>                    Override attributes of a group.
>                </para>
>            </listitem>
>        </varlistentry>
>        <varlistentry>
>            <term>
>                <option>group-del</option>
>                <emphasis>NAME</emphasis>
>            </term>
>            <listitem>
>                <para>
>                    Remove group overrides.
>                </para>
>            </listitem>
>        </varlistentry>
>    </variablelist>
>
> </refsect1>
> <xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="include/seealso.xml" />
>
> +</refentry>
> +</reference>
> diff --git a/src/tools/sss_override.c b/src/tools/sss_override.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..0460737d0428ecd9036f439fe6885a471a21fad0
> --- /dev/null
> +++ b/src/tools/sss_override.c
> @@ -0,0 +1,698 @@
> +/*
> Authors:
>    Pavel Březina <[e-mail]>
> Copyright (C) 2015 Red Hat
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 3 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program.  If not, see http://www.gnu.org/licenses/.
>
> +*/
>
> +#include <stdlib.h>
>
> +#include "util/util.h"
> +#include "db/sysdb.h"
> +#include "tools/common/sss_tools.h"
>
> +#define LOCALVIEW SYSDB_LOCAL_VIEW_NAME
>
> +struct override_user {
> const char *input_name;
> const char *orig_name;
> struct sss_domain_info *domain;
> const char *name;
> uid_t uid;
> gid_t gid;
> const char *home;
> const char *shell;
> const char *gecos;
>
> +};
>
> +struct override_group {
> const char *input_name;
> const char *orig_name;
> struct sss_domain_info *domain;
> const char *name;
> gid_t gid;
>
> +};
>
> +static int parse_cmdline(struct sss_cmdline *cmdline,
>                     struct sss_tool_ctx *tool_ctx,
>                     struct poptOption *options,
>                     const char **_input_name,
>                     const char **_orig_name,
>                     struct sss_domain_info **_domain)
>
> +{
> enum sss_tool_opt require;
> const char *input_name;
> const char *orig_name;
> struct sss_domain_info *domain;
> int ret;
> require = options == NULL ? SSS_TOOL_OPT_OPTIONAL : SSS_TOOL_OPT_REQUIRED;
> ret = sss_tool_popt_ex(cmdline, options, require,
>                       NULL, NULL, "NAME", "Specify name of modified "

Missing translation.
>                       "object.", &input_name);
> if (ret != EXIT_SUCCESS) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command arguments\n");
>    return ret;
> }
> ret = sss_tool_parse_name(tool_ctx, tool_ctx, input_name,
>                          SSS_TOOL_DOMAIN_OPTIONAL,
>                          &orig_name, &domain);
> if (ret != EOK) {
>    fprintf(stderr, "Unable to parse name.\n");

Missing translation.
>    return ret;
> }
> *_input_name = input_name;
> *_orig_name = orig_name;
> *_domain = domain;
> return EXIT_SUCCESS;
>
> +}

[...]
> +static errno_t prepare_view(struct sss_domain_info *domain)
> +{
> char *viewname = NULL;
> errno_t ret;
> ret = sysdb_get_view_name(NULL, domain->sysdb, &viewname);
> if (ret != EOK && ret != ENOENT) {
>    DEBUG(SSSDBG_OP_FAILURE, "sysdb_get_view_name() failed.\n");
>    return ret;
> }
> if (ret == EOK) {
>    if (!is_default_view(viewname)) {
>        DEBUG(SSSDBG_TRACE_FUNC, "There already exists view %s. "
>              "Only one view is supported. Nothing to do.\n", viewname);

This debug level should be more verbose.
>        ret = EEXIST;
>        goto done;
>    } else if (strcmp(viewname, LOCALVIEW) == 0) {
>        DEBUG(SSSDBG_TRACE_FUNC, "%s view is already present.\n", viewname);
>        ret = EOK;
>        goto done;
>    }
> }
> DEBUG(SSSDBG_TRACE_FUNC, "Creating %s view.\n", LOCALVIEW);
> ret = sysdb_update_view_name(domain->sysdb, LOCALVIEW);
> if (ret == EOK) {
>    printf("SSSD needs to be restarted for the changes to take effect.\n");

Hmm, why? Can we instead send a signal to SSSD to pick up the changes?
(If yes, then it can be a follow-up patch..)
> }
>
> +done:
> talloc_free(viewname);
> return ret;
>
> +}
>
> +static char * build_anchor(TALLOC_CTX *mem_ctx, const char *obj_dn)

~~
Extra space
> +{
> char *anchor;
> char *safe_dn;
> errno_t ret;
> ret = sysdb_dn_sanitize(mem_ctx, obj_dn, &safe_dn);

Is this sanitization required? Shouldn't a sysdb DN be already
sanitized?
> if (ret != EOK) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "sysdb_dn_sanitize() failed\n");
>    return NULL;
> }
> anchor = talloc_asprintf(mem_ctx, ":%s:%s", LOCALVIEW, safe_dn);
> talloc_free(safe_dn);
> return anchor;
>
> +}
>
> +static struct sysdb_attrs * build_attrs(TALLOC_CTX *mem_ctx,

~~
Extra space
>                                    const char *name,
>                                    uid_t uid,
>                                    gid_t gid,
>                                    const char *home,
>                                    const char *shell,
>                                    const char *gecos)
>
> +{
> struct sysdb_attrs *attrs;
> errno_t ret;
> attrs = sysdb_new_attrs(mem_ctx);
> if (attrs == NULL) {
>    return NULL;
> }
> if (name != NULL) {
>    ret = sysdb_attrs_add_string(attrs, SYSDB_NAME, name);
>    if (ret != EOK) {
>        goto done;
>    }
> }
> if (uid != 0) {
>    ret = sysdb_attrs_add_uint32(attrs, SYSDB_UIDNUM, uid);
>    if (ret != EOK) {
>        goto done;
>    }
> }
> if (gid != 0) {
>    ret = sysdb_attrs_add_uint32(attrs, SYSDB_GIDNUM, gid);
>    if (ret != EOK) {
>        goto done;
>    }
> }
> if (home != NULL) {
>    ret = sysdb_attrs_add_string(attrs, SYSDB_HOMEDIR, home);
>    if (ret != EOK) {
>        goto done;
>    }
> }
> if (shell != NULL) {
>    ret = sysdb_attrs_add_string(attrs, SYSDB_SHELL, shell);
>    if (ret != EOK) {
>        goto done;
>    }
> }
> if (gecos != NULL) {
>    ret = sysdb_attrs_add_string(attrs, SYSDB_GECOS, gecos);
>    if (ret != EOK) {
>        goto done;
>    }
> }
>
> +done:
> if (ret != EOK) {
>    talloc_free(attrs);
>    return NULL;
> }
> return attrs;
>
> +}
>
> +static struct sysdb_attrs * build_user_attrs(TALLOC_CTX *mem_ctx,

~~
Extra space
>                                         struct override_user *user)
>
> +{
> return build_attrs(mem_ctx, user->name, user->uid, user->gid, user->home,
>                   user->shell, user->gecos);
>
> +}
>
> +static struct sysdb_attrs * build_group_attrs(TALLOC_CTX *mem_ctx,

~~
Extra space
>                                          struct override_group *group)
>
> +{
> return build_attrs(mem_ctx, group->name, 0, group->gid, 0, NULL, NULL);
>
> +}
>
> +static const char * get_object_dn_and_domain(TALLOC_CTX *mem_ctx,

~~
Extra space
>                                     enum sysdb_member_type type,
>                                     const char *name,
>                                     struct sss_domain_info *domain,
>                                     struct sss_domain_info *domains,
>                                     struct sss_domain_info **_new_domain)
>
> +{
> TALLOC_CTX *tmp_ctx;
> struct sss_domain_info *dom;
> struct ldb_result *res;
> const char *dn;
> const char *strtype;
> bool check_next;
> errno_t ret;
> tmp_ctx = talloc_new(NULL);
> if (tmp_ctx == NULL) {
>    return NULL;
> }
> /* Ensure that the object is in cache. */
> switch (type) {
> case SYSDB_MEMBER_USER:
>    if (getpwnam(name) == NULL) {
>        ret = ENOENT;
>        goto done;
>    }
>    break;
> case SYSDB_MEMBER_GROUP:
>    if (getgrnam(name) == NULL) {
>        ret = ENOENT;
>        goto done;
>    }
>    break;
> default:
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported member type %d\n", type);
>    ret = ERR_INTERNAL;
>    goto done;
> }
> /* Find domain if it is unknown. */
> if (domain == NULL) {
>    check_next = true;
>    dom = domains;
> } else {
>    check_next = false;
>    dom = domain;
> }
> do {
>    switch (type) {
>    case SYSDB_MEMBER_USER:
>        DEBUG(SSSDBG_TRACE_FUNC, "Trying to find user %s@%s\n",
>              name, dom->name);
>        ret = sysdb_getpwnam(tmp_ctx, dom, name, &res);
>        strtype = "user";
>        break;
>    case SYSDB_MEMBER_GROUP:
>        DEBUG(SSSDBG_TRACE_FUNC, "Trying to find group %s@%s\n",
>              name, dom->name);
>        ret = sysdb_getgrnam(tmp_ctx, dom, name, &res);
>        strtype = "group";
>        break;
>    default:
>        DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported member type %d\n", type);
>        ret = ERR_INTERNAL;
>        goto done;
>    }
>    if (ret == EOK && res->count == 0) {
>        ret = ENOENT;
>        if (check_next) {
>            dom = dom->next;
>            continue;
>        }
>    }
>    if (ret != EOK) {
>        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to find %s %s@%s [%d]: %s\n",
>              strtype, name, dom->name, ret, sss_strerror(ret));
>        goto done;
>    } else if (res->count != 1) {
>        DEBUG(SSSDBG_CRIT_FAILURE, "More than one %s found?\n", strtype);
>        ret = ERR_INTERNAL;
>        goto done;
>    }
>    check_next = false;
> } while (check_next && dom != NULL);
> DEBUG(SSSDBG_TRACE_FUNC, "Domain of %s %s is %s\n",
>      strtype, name, dom->name);

Hmm looks like you should have a check for res validity here, for cases
no domain match..

btw it might be nice to split this large function into smaller ones.
Maybe the do-while check could be a separate function returning res.
> dn = ldb_dn_get_linearized(res->msgs[0]->dn);
> if (dn == NULL) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "ldb_dn_get_linearized() failed.\n");
>    ret = ENOMEM;
>    goto done;
> }
> talloc_steal(mem_ctx, dn);
> *_new_domain = dom;
> ret = EOK;
>
> +done:
> talloc_free(tmp_ctx);
> if (ret != EOK) {
>    return NULL;
> }
> return dn;
>
> +}

[...]
> +static errno_t override_object_del(struct sss_domain_info *domain,
>                               const char *obj_dn)
>
> +{
> TALLOC_CTX *tmp_ctx;
> const char *anchor;
> struct ldb_dn *override_dn;
> struct ldb_message *msg;
> errno_t ret;
> int sret;
> bool in_transaction;
> struct ldb_context *ldb = sysdb_ctx_get_ldb(domain->sysdb);
> tmp_ctx = talloc_new(NULL);
> if (tmp_ctx == NULL) {
>    return ENOMEM;
> }
> anchor = build_anchor(tmp_ctx, obj_dn);
> if (anchor == NULL) {
>    ret = ENOMEM;
>    goto done;
> }
> override_dn = ldb_dn_new_fmt(tmp_ctx, ldb,
>                    SYSDB_TMPL_OVERRIDE, anchor, LOCALVIEW);
> if (override_dn == NULL) {
>    ret = ENOMEM;
>    goto done;
> }
> DEBUG(SSSDBG_TRACE_FUNC, "Removing override for %s\n", obj_dn);
> ret = sysdb_transaction_start(domain->sysdb);
> if (ret != EOK) {
>    DEBUG(SSSDBG_OP_FAILURE, "sysdb_transaction_start() failed.\n");
>    goto done;
> }
> in_transaction = true;
> ret = sysdb_delete_entry(domain->sysdb, override_dn, true);

ret is not checked here.
> msg = ldb_msg_new(tmp_ctx);
> if (msg == NULL) {
>    ret = ENOMEM;
>    goto done;
> }
> msg->dn = ldb_dn_new(msg, ldb, obj_dn);
> if (msg->dn == NULL) {
>    ret = ENOMEM;
>    goto done;
> }
> ret = ldb_msg_add_empty(msg, SYSDB_OVERRIDE_DN, LDB_FLAG_MOD_DELETE, NULL);
> if (ret != LDB_SUCCESS) {
>    DEBUG(SSSDBG_OP_FAILURE, "ldb_msg_add_empty() failed\n");
>    ret = sysdb_error_to_errno(ret);
>    goto done;
> }
> ret = ldb_modify(ldb, msg);
> if (ret != LDB_SUCCESS && ret != LDB_ERR_NO_SUCH_ATTRIBUTE) {
>    DEBUG(SSSDBG_OP_FAILURE,
>          "ldb_modify() failed: [%s](%d)[%s]\n",
>          ldb_strerror(ret), ret, ldb_errstring(ldb));
>    ret = sysdb_error_to_errno(ret);
>    goto done;
> }

[...]
> +static int override_user_add(struct sss_cmdline *cmdline,
>                         struct sss_tool_ctx *tool_ctx,
>                         void *pvt)
>
> +{
> struct override_user user = {NULL};
> struct sysdb_attrs *attrs;
> const char *dn;
> int ret;
> ret = parse_cmdline_user_add(cmdline, tool_ctx, &user);
> if (ret != EOK) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command line.\n");
>    return EXIT_FAILURE;
> }
> dn = get_user_dn_and_domain(tool_ctx, tool_ctx->domains, &user);
> if (dn == NULL) {
>    fprintf(stderr, "Unable to find user %s@%s.\n",

Missing translation.
>            user.orig_name, user.domain->name);
>    return EXIT_FAILURE;
> }
> ret = prepare_view(user.domain);
> if (ret == ENOENT) {
>    fprintf(stderr, "Other than LOCAL view already exist in domain %s.\n",

Missing translation.
>            user.domain->name);
>    return EXIT_FAILURE;
> }
> attrs = build_user_attrs(tool_ctx, &user);
> if (attrs == NULL) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to build sysdb attrs.\n");
>    return EXIT_FAILURE;
> }
> ret = override_object_add(user.domain, SYSDB_MEMBER_USER, attrs, dn);
> if (ret != EOK) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to add override object.\n");
>    return EXIT_FAILURE;
> }
> return EXIT_SUCCESS;
>
> +}
>
> +static int override_user_del(struct sss_cmdline *cmdline,
>                         struct sss_tool_ctx *tool_ctx,
>                         void *pvt)
>
> +{
> struct override_user user = {NULL};
> const char *dn;
> int ret;
> ret = parse_cmdline_user_del(cmdline, tool_ctx, &user);
> if (ret != EOK) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command line.\n");
>    return EXIT_FAILURE;
> }
> dn = get_user_dn_and_domain(tool_ctx, tool_ctx->domains, &user);
> if (dn == NULL) {
>    fprintf(stderr, "Unable to find user %s@%s.\n",

Missing translation.
>            user.orig_name, user.domain->name);
>    return EXIT_FAILURE;
> }
> ret = override_object_del(user.domain, dn);
> if (ret != EOK) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to add override object.\n");
>    return EXIT_FAILURE;
> }
> return EXIT_SUCCESS;
>
> +}
>
> +static int override_group_add(struct sss_cmdline *cmdline,
>                          struct sss_tool_ctx *tool_ctx,
>                          void *pvt)
>
> +{
> struct override_group group = {NULL};
> struct sysdb_attrs *attrs;
> const char *dn;
> int ret;
> ret = parse_cmdline_group_add(cmdline, tool_ctx, &group);
> if (ret != EOK) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command line.\n");
>    return EXIT_FAILURE;
> }
> dn = get_group_dn_and_domain(tool_ctx, tool_ctx->domains, &group);
> if (dn == NULL) {
>    fprintf(stderr, "Unable to find group %s@%s.\n",

Missing translation.
>            group.orig_name, group.domain->name);
>    return EXIT_FAILURE;
> }
> ret = prepare_view(group.domain);
> if (ret == ENOENT) {
>    fprintf(stderr, "Other than LOCAL view already exist in domain %s.\n",

Missing translation.
>            group.domain->name);
>    return EXIT_FAILURE;
> }
> attrs = build_group_attrs(tool_ctx, &group);
> if (attrs == NULL) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to build sysdb attrs.\n");
>    return EXIT_FAILURE;
> }
> ret = override_object_add(group.domain, SYSDB_MEMBER_GROUP, attrs, dn);
> if (ret != EOK) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to add override object.\n");
>    return EXIT_FAILURE;
> }
> return EXIT_SUCCESS;
>
> +}
>
> +static int override_group_del(struct sss_cmdline *cmdline,
>                          struct sss_tool_ctx *tool_ctx,
>                          void *pvt)
>
> +{
> struct override_group group = {NULL};
> const char *dn;
> int ret;
> ret = parse_cmdline_group_del(cmdline, tool_ctx, &group);
> if (ret != EOK) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse command line.\n");
>    return EXIT_FAILURE;
> }
> dn = get_group_dn_and_domain(tool_ctx, tool_ctx->domains, &group);
> if (dn == NULL) {
>    fprintf(stderr, "Unable to find group %s@%s.\n",

Missing translation.
>            group.orig_name, group.domain->name);
>    return EXIT_FAILURE;
> }
> ret = override_object_del(group.domain, dn);
> if (ret != EOK) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to add override object.\n");
>    return EXIT_FAILURE;
> }
> return EXIT_SUCCESS;
>
> +}
>
> +int main(int argc, const char **argv)
> +{
> struct sss_route_cmd commands[] = {
>    {"user-add", override_user_add},
>    {"user-del", override_user_del},
>    {"group-add", override_group_add},
>    {"group-del", override_group_del},
>    {NULL, NULL}
> };
> return sss_tool_main(argc, argv, commands, NULL);

OK, this is really nice :-)

It would be also nice to add integration tests for the overrides. But
maybe it would be better if someone else than you actually wrote the
tests to catch some issues..

[Jakub Hrozek]

On Sun, Jul 26, 2015 at 08:14:22PM +0200, Jakub Hrozek wrote:
> On Fri, Jul 24, 2015 at 01:08:17PM +0200, Pavel Březina wrote:
> > https://fedorahosted.org/sssd/ticket/2584
> >
> > If you have any idea how to improve manual page, please, share it.
>
> Hi,
>
> please see my comments inline.

btw the patches work more or less fine -- great work, thanks.

The only functional error is the missing check for NULL res value if an
entry is found by getpwnam() but not sysdb_getpwnam():

[root@client ~]# sss_override user-add root -n toor
ldb: unable to dlopen /usr/lib64/ldb/modules/ldb/memberof.la :
/usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header
Segmentation fault (core dumped)

After testing, I put the patches through Coverity. Here are the errors
(some were already mentioned in the review):

Error: UNINIT (CWE-457): [#def1]
sssd-1.13.1/src/tools/common/sss_tools.c:249: var_decl: Declaring variable "pc" without initializer.
sssd-1.13.1/src/tools/common/sss_tools.c:321: uninit_use_in_call: Using uninitialized value "pc" when calling "poptFreeContext".
#  319|   
#  320|   done:
#  321|->     poptFreeContext(pc);
#  322|       talloc_free(help);
#  323|       return ret;

Error: COMPILER_WARNING: [#def2]
sssd-1.13.1/src/tools/common/sss_tools.c:321:5: warning: 'pc' may be used uninitialized in this function [-Wmaybe-uninitialized]
#     poptFreeContext(pc);
#     ^
#  319|   
#  320|   done:
#  321|->     poptFreeContext(pc);
#  322|       talloc_free(help);
#  323|       return ret;

Error: UNINIT (CWE-457): [#def3]
sssd-1.13.1/src/tools/common/sss_tools.c:250: var_decl: Declaring variable "ret" without initializer.
sssd-1.13.1/src/tools/common/sss_tools.c:323: uninit_use: Using uninitialized value "ret".
#  321|       poptFreeContext(pc);
#  322|       talloc_free(help);
#  323|->     return ret;
#  324|   }
#  325|

Error: COMPILER_WARNING: [#def4]
sssd-1.13.1/src/tools/common/sss_tools.c: scope_hint: In function 'sss_tool_popt_ex'
sssd-1.13.1/src/tools/common/sss_tools.c:323:5: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
#     return ret;
#     ^
#  321|       poptFreeContext(pc);
#  322|       talloc_free(help);
#  323|->     return ret;
#  324|   }
#  325|

Error: UNINIT (CWE-457): [#def5]
sssd-1.13.1/src/tools/sss_override.c:203: var_decl: Declaring variable "ret" without initializer.
sssd-1.13.1/src/tools/sss_override.c:253: uninit_use: Using uninitialized value "ret".
#  251|   
#  252|   done:
#  253|->     if (ret != EOK) {
#  254|           talloc_free(attrs);
#  255|           return NULL;

Error: DEADCODE (CWE-561): [#def6]
sssd-1.13.1/src/tools/sss_override.c:302: equality_cond: Jumping to case "SYSDB_MEMBER_GROUP".
sssd-1.13.1/src/tools/sss_override.c:331: equality_cond: Jumping to case "SYSDB_MEMBER_GROUP".
sssd-1.13.1/src/tools/sss_override.c:296: equality_cond: Jumping to case "SYSDB_MEMBER_USER".
sssd-1.13.1/src/tools/sss_override.c:325: equality_cond: Jumping to case "SYSDB_MEMBER_USER".
sssd-1.13.1/src/tools/sss_override.c:324: between: When switching on "type", the value of "type" must be between 0 and 1.
sssd-1.13.1/src/tools/sss_override.c:324: dead_error_condition: The switch value "type" cannot reach the default case.
sssd-1.13.1/src/tools/sss_override.c:337: dead_error_begin: Execution cannot reach this statement: "default:".
#  335|               strtype = "group";
#  336|               break;
#  337|->         default:
#  338|               DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported member type %d\n", type);
#  339|               ret = ERR_INTERNAL;

Error: FORWARD_NULL (CWE-476): [#def7]
sssd-1.13.1/src/tools/sss_override.c:363: var_compare_op: Comparing "dom" to null implies that "dom" might be null.
sssd-1.13.1/src/tools/sss_override.c:365: var_deref_op: Dereferencing null pointer "dom".
#  363|       } while (check_next && dom != NULL);
#  364|   
#  365|->     DEBUG(SSSDBG_TRACE_FUNC, "Domain of %s %s is %s\n",
#  366|             strtype, name, dom->name);
#  367|

Error: UNINIT (CWE-457): [#def8]
sssd-1.13.1/src/tools/sss_override.c:458: var_decl: Declaring variable "in_transaction" without initializer.
sssd-1.13.1/src/tools/sss_override.c:528: uninit_use: Using uninitialized value "in_transaction".
#  526|   
#  527|   done:
#  528|->     if (in_transaction) {
#  529|           sret = sysdb_transaction_cancel(domain->sysdb);
#  530|           if (sret != EOK) {

Error: COMPILER_WARNING: [#def9]
sssd-1.13.1/src/tools/sss_override.c: scope_hint: In function 'override_object_del.isra.1'
sssd-1.13.1/src/tools/sss_override.c:528:8: warning: 'in_transaction' may be used uninitialized in this function [-Wmaybe-uninitialized]
#     if (in_transaction) {
#        ^
#  526|   
#  527|   done:
#  528|->     if (in_transaction) {
#  529|           sret = sysdb_transaction_cancel(domain->sysdb);
#  530|           if (sret != EOK) {

Also you forgot to package the new tool and the new man page in the specfile.

And finally, some questions/proposals after testing:

- Do you think it makes sense to also add -find and -show commands?

- I wonder if we should explicitly barf if the user triying to override
  UID to 0 ? I did test it and it's not possible, sssd returns the original
  UID, which is good, I just wonder about a nicer error message.

[Pavel Březina]

On 07/26/2015 09:40 PM, Jakub Hrozek wrote:
> On Sun, Jul 26, 2015 at 08:14:22PM +0200, Jakub Hrozek wrote:
> > On Fri, Jul 24, 2015 at 01:08:17PM +0200, Pavel Březina wrote:
> > > https://fedorahosted.org/sssd/ticket/2584
> > >
> > > If you have any idea how to improve manual page, please, share it.
> >
> > Hi,
> >
> > please see my comments inline.
>
> btw the patches work more or less fine -- great work, thanks.
>
> The only functional error is the missing check for NULL res value if an
> entry is found by getpwnam() but not sysdb_getpwnam():
>
> [root@client ~]# sss_override user-add root -n toor
> ldb: unable to dlopen /usr/lib64/ldb/modules/ldb/memberof.la :
> /usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header
> Segmentation fault (core dumped)

I think it was problem of accessing dom rather than res. Check is there.
> After testing, I put the patches through Coverity. Here are the errors
> (some were already mentioned in the review):

I fixed all of them, hopefully, except:
> Error: DEADCODE (CWE-561): [#def6]
> sssd-1.13.1/src/tools/sss_override.c:302: equality_cond: Jumping to case "SYSDB_MEMBER_GROUP".
> sssd-1.13.1/src/tools/sss_override.c:331: equality_cond: Jumping to case "SYSDB_MEMBER_GROUP".
> sssd-1.13.1/src/tools/sss_override.c:296: equality_cond: Jumping to case "SYSDB_MEMBER_USER".
> sssd-1.13.1/src/tools/sss_override.c:325: equality_cond: Jumping to case "SYSDB_MEMBER_USER".
> sssd-1.13.1/src/tools/sss_override.c:324: between: When switching on "type", the value of "type" must be between 0 and 1.
> sssd-1.13.1/src/tools/sss_override.c:324: dead_error_condition: The switch value "type" cannot reach the default case.
> sssd-1.13.1/src/tools/sss_override.c:337: dead_error_begin: Execution cannot reach this statement: "default:".
> #  335|               strtype = "group";
> #  336|               break;
> #  337|->         default:
> #  338|               DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported member type %d\n", type);
> #  339|               ret = ERR_INTERNAL;

Yes, it is a dead code. But I think it's better to leave it in:
1) Without default a compiler will produce warning since not all 
SYSDB_MEMBER* are used.
2) Better to have it.

Do you think it may be better to use all SYSDB_MEMBER* instead of 
default branch? It will however also produce the coverity warning, I think.
> Also you forgot to package the new tool and the new man page in the specfile.
>
> And finally, some questions/proposals after testing:
> Do you think it makes sense to also add -find and -show commands?

In the future, yes. We may want them.
> I wonder if we should explicitly barf if the user triying to override
>  UID to 0 ? I did test it and it's not possible, sssd returns the original
>  UID, which is good, I just wonder about a nicer error message.

Or a man page change? Or both? It is not in this patchset...

[Jakub Hrozek]

On Mon, Jul 27, 2015 at 12:04:33PM +0200, Pavel Březina wrote:
> On 07/26/2015 09:40 PM, Jakub Hrozek wrote:
> > On Sun, Jul 26, 2015 at 08:14:22PM +0200, Jakub Hrozek wrote:
> > > On Fri, Jul 24, 2015 at 01:08:17PM +0200, Pavel Březina wrote:
> > > > https://fedorahosted.org/sssd/ticket/2584
> > > >
> > > > If you have any idea how to improve manual page, please, share it.
> > >
> > > Hi,
> > >
> > > please see my comments inline.
> >
> > btw the patches work more or less fine -- great work, thanks.
> >
> > The only functional error is the missing check for NULL res value if an
> > entry is found by getpwnam() but not sysdb_getpwnam():
> >
> > [root@client ~]# sss_override user-add root -n toor
> > ldb: unable to dlopen /usr/lib64/ldb/modules/ldb/memberof.la :
> > /usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header
> > Segmentation fault (core dumped)
>
> I think it was problem of accessing dom rather than res. Check is there.

Thanks.
> > After testing, I put the patches through Coverity. Here are the errors
> > (some were already mentioned in the review):
>
> I fixed all of them, hopefully, except:
> > Error: DEADCODE (CWE-561): [#def6]
> > sssd-1.13.1/src/tools/sss_override.c:302: equality_cond: Jumping to case "SYSDB_MEMBER_GROUP".
> > sssd-1.13.1/src/tools/sss_override.c:331: equality_cond: Jumping to case "SYSDB_MEMBER_GROUP".
> > sssd-1.13.1/src/tools/sss_override.c:296: equality_cond: Jumping to case "SYSDB_MEMBER_USER".
> > sssd-1.13.1/src/tools/sss_override.c:325: equality_cond: Jumping to case "SYSDB_MEMBER_USER".
> > sssd-1.13.1/src/tools/sss_override.c:324: between: When switching on "type", the value of "type" must be between 0 and 1.
> > sssd-1.13.1/src/tools/sss_override.c:324: dead_error_condition: The switch value "type" cannot reach the default case.
> > sssd-1.13.1/src/tools/sss_override.c:337: dead_error_begin: Execution cannot reach this statement: "default:".
> > #  335|               strtype = "group";
> > #  336|               break;
> > #  337|->         default:
> > #  338|               DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported member type %d\n", type);
> > #  339|               ret = ERR_INTERNAL;
>
> Yes, it is a dead code. But I think it's better to leave it in:
> Without default a compiler will produce warning since not all
>
> SYSDB_MEMBER* are used.
> 2) Better to have it.
>
> Do you think it may be better to use all SYSDB_MEMBER* instead of default
> branch? It will however also produce the coverity warning, I think.

I think this is OK and it's better to be paranoid and fail.

Just be prepared there will be a Coverity warning next Monday on the
internal list :-) and when it arrives, mark the Coverity error as a
false positive..
> > Also you forgot to package the new tool and the new man page in the specfile.
> >
> > And finally, some questions/proposals after testing:
> > Do you think it makes sense to also add -find and -show commands?
>
> In the future, yes. We may want them.

Please file a ticket. I suspect we'll get a request, so it might be nice
to acknowledge the issue.
> > I wonder if we should explicitly barf if the user triying to override
> > UID to 0 ? I did test it and it's not possible, sssd returns the original
> > UID, which is good, I just wonder about a nicer error message.
>
> Or a man page change? Or both? It is not in this patchset...

Both would be best, it's not like we're going to change that behaviour,
ever, so once we write these two, they're to stay :-) It doesn't have to
be part of this patchset either.

[Lukas Slebodnik]

On (27/07/15 12:13), Jakub Hrozek wrote:
> On Mon, Jul 27, 2015 at 12:04:33PM +0200, Pavel Březina wrote:
> > On 07/26/2015 09:40 PM, Jakub Hrozek wrote:
> > > On Sun, Jul 26, 2015 at 08:14:22PM +0200, Jakub Hrozek wrote:
> > > > On Fri, Jul 24, 2015 at 01:08:17PM +0200, Pavel Březina wrote:
> > > > > https://fedorahosted.org/sssd/ticket/2584
> > > > >
> > > > > If you have any idea how to improve manual page, please, share it.
> > > >
> > > > Hi,
> > > >
> > > > please see my comments inline.
> > >
> > > btw the patches work more or less fine -- great work, thanks.
> > >
> > > The only functional error is the missing check for NULL res value if an
> > > entry is found by getpwnam() but not sysdb_getpwnam():
> > >
> > > [root@client ~]# sss_override user-add root -n toor
> > > ldb: unable to dlopen /usr/lib64/ldb/modules/ldb/memberof.la :
> > > /usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header
> > > Segmentation fault (core dumped)
> >
> > I think it was problem of accessing dom rather than res. Check is there.
>
> Thanks.
> > > After testing, I put the patches through Coverity. Here are the errors
> > > (some were already mentioned in the review):
> >
> > I fixed all of them, hopefully, except:
> > > Error: DEADCODE (CWE-561): [#def6]
> > > sssd-1.13.1/src/tools/sss_override.c:302: equality_cond: Jumping to case "SYSDB_MEMBER_GROUP".
> > > sssd-1.13.1/src/tools/sss_override.c:331: equality_cond: Jumping to case "SYSDB_MEMBER_GROUP".
> > > sssd-1.13.1/src/tools/sss_override.c:296: equality_cond: Jumping to case "SYSDB_MEMBER_USER".
> > > sssd-1.13.1/src/tools/sss_override.c:325: equality_cond: Jumping to case "SYSDB_MEMBER_USER".
> > > sssd-1.13.1/src/tools/sss_override.c:324: between: When switching on "type", the value of "type" must be between 0 and 1.
> > > sssd-1.13.1/src/tools/sss_override.c:324: dead_error_condition: The switch value "type" cannot reach the default case.
> > > sssd-1.13.1/src/tools/sss_override.c:337: dead_error_begin: Execution cannot reach this statement: "default:".
> > > #  335|               strtype = "group";
> > > #  336|               break;
> > > #  337|->         default:
> > > #  338|               DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported member type %d\n", type);
> > > #  339|               ret = ERR_INTERNAL;
> >
> > Yes, it is a dead code. But I think it's better to leave it in:
> > Without default a compiler will produce warning since not all
> >
> > SYSDB_MEMBER* are used.
> > 2) Better to have it.
> >
> > Do you think it may be better to use all SYSDB_MEMBER* instead of default
> > branch? It will however also produce the coverity warning, I think.
>
> I think this is OK and it's better to be paranoid and fail.
>
> Just be prepared there will be a Coverity warning next Monday on the
> internal list :-) and when it arrives, mark the Coverity error as a
> false positive..

It's not a false positive. "false positive" means bug in static analysers.
It should be marked as an intentional.

LS

[Jakub Hrozek]

On Sun, Jul 26, 2015 at 08:14:22PM +0200, Jakub Hrozek wrote:
> > +int sss_tool_main(int argc, const char **argv,
> >              struct sss_route_cmd *commands,
> >              void *pvt)
> >
> > +{
> > struct sss_tool_ctx *tool_ctx;
> > uid_t uid;
> > int ret;
> > uid = getuid();
> > if (uid != 0) {
> >    DEBUG(SSSDBG_CRIT_FAILURE, "Running under %d, must be root\n", uid);
>
> I think here it should be either root or sssd user. Even better might be
> to try and gracefully fail if opening the databases fail -- I don't mind
> which solution you choose.

Please ignore this, of course requiring root for a /tool/ is the right
thing to do. Sorry.

[Pavel Březina]

On 07/26/2015 08:14 PM, Jakub Hrozek wrote:
> On Fri, Jul 24, 2015 at 01:08:17PM +0200, Pavel Březina wrote:
> > https://fedorahosted.org/sssd/ticket/2584
>
> btw (unrelated to these patches) it seems strange to require the user of
> the sysdb database to connect to the db and then separately initialize
> subdomains...I would vote for merging that code..

+1
> > +#include <talloc.h>
> > +#include <popt.h>
> >
> > +#include "confdb/confdb.h"
> >
> > +struct sss_tool_ctx {
> > struct confdb_ctx *confdb;
> > char *confdb_path;
> > char *default_domain;
> > struct sss_domain_info *domains;
>
> I wonder if the tool_ctx must be open for consumers? Currently only
> domains is used, maybe we could get away with a getter and an opaque
> structure? We can always make the structure public in future, but if
> it's public from the start, then it's easier to add cruft..

I think it is not worth the effort with such simple structure. I am
all for opaque structures but here is just no gain in code safety. It 
would only make code lines bigger.
> > +enum sss_tool_domain {
> > SSS_TOOL_DOMAIN_REQUIRED,
> > SSS_TOOL_DOMAIN_OPTIONAL
> >
> > +};
> >
> > +int sss_tool_parse_name(TALLOC_CTX *mem_ctx,
> >                    struct sss_tool_ctx *tool_ctx,
> >                    const char *input,
> >                    enum sss_tool_domain require_domain,
>
> Do we need require_domain now that you added the getpwnam?

No, removed.
> > <refsect1 id='description'>
> >    <title>DESCRIPTION</title>
> >    <para>
> >        <command>sss_override</command> enables to create a client-side
> >        view and allows to change selected values of specific user
> >        and groups. This change takes effect only on client machine.
>                                                          ~~~~~~~~~~~~~~~
>
> Here I think we should definitely say local.
> >    </para>
> >
> > </refsect1>
>
> We should say that overriding attributes of users and groups only is
> supported (IIRC we have an RFE somewhere to override netgroups
> per-client...)

It says "specific user and groups". That says exactly this, I think.
> > +static errno_t prepare_view(struct sss_domain_info *domain)
> > +{
> > char *viewname = NULL;
> > errno_t ret;
> > ret = sysdb_get_view_name(NULL, domain->sysdb, &viewname);
> > if (ret != EOK && ret != ENOENT) {
> >    DEBUG(SSSDBG_OP_FAILURE, "sysdb_get_view_name() failed.\n");
> >    return ret;
> > }
> > if (ret == EOK) {
> >    if (!is_default_view(viewname)) {
> >        DEBUG(SSSDBG_TRACE_FUNC, "There already exists view %s. "
> >              "Only one view is supported. Nothing to do.\n", viewname);
>
> This debug level should be more verbose.
> >        ret = EEXIST;
> >        goto done;
> >    } else if (strcmp(viewname, LOCALVIEW) == 0) {
> >        DEBUG(SSSDBG_TRACE_FUNC, "%s view is already present.\n", viewname);
> >        ret = EOK;
> >        goto done;
> >    }
> > }
> > DEBUG(SSSDBG_TRACE_FUNC, "Creating %s view.\n", LOCALVIEW);
> > ret = sysdb_update_view_name(domain->sysdb, LOCALVIEW);
> > if (ret == EOK) {
> >    printf("SSSD needs to be restarted for the changes to take effect.\n");
>
> Hmm, why? Can we instead send a signal to SSSD to pick up the changes?
> (If yes, then it can be a follow-up patch..)

No, it is not possible to change view on the fly the way views are 
implemented.
> > +{
> > char *anchor;
> > char *safe_dn;
> > errno_t ret;
> > ret = sysdb_dn_sanitize(mem_ctx, obj_dn, &safe_dn);
>
> Is this sanitization required? Shouldn't a sysdb DN be already
> sanitized?

Yes, it is. Values in sections of dn are sanitized but we also need to 
sanitize commas separating sections. Since it is used only as an 
internal identifier we do not need to care about double escaping.
> > if (ret != EOK) {
> >    DEBUG(SSSDBG_CRIT_FAILURE, "sysdb_dn_sanitize() failed\n");
> >    return NULL;
> > }
> > anchor = talloc_asprintf(mem_ctx, ":%s:%s", LOCALVIEW, safe_dn);
> > talloc_free(safe_dn);
> > return anchor;
> >
> > +}
> >                                     enum sysdb_member_type type,
> >                                     const char *name,
> >                                     struct sss_domain_info *domain,
> >                                     struct sss_domain_info *domains,
> >                                     struct sss_domain_info **_new_domain)
> >
> > +{
> > TALLOC_CTX *tmp_ctx;
> > struct sss_domain_info *dom;
> > struct ldb_result *res;
> > const char *dn;
> > const char *strtype;
> > bool check_next;
> > errno_t ret;
> > tmp_ctx = talloc_new(NULL);
> > if (tmp_ctx == NULL) {
> >    return NULL;
> > }
> > /* Ensure that the object is in cache. */
> > switch (type) {
> > case SYSDB_MEMBER_USER:
> >    if (getpwnam(name) == NULL) {
> >        ret = ENOENT;
> >        goto done;
> >    }
> >    break;
> > case SYSDB_MEMBER_GROUP:
> >    if (getgrnam(name) == NULL) {
> >        ret = ENOENT;
> >        goto done;
> >    }
> >    break;
> > default:
> >    DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported member type %d\n", type);
> >    ret = ERR_INTERNAL;
> >    goto done;
> > }
> > /* Find domain if it is unknown. */
> > if (domain == NULL) {
> >    check_next = true;
> >    dom = domains;
> > } else {
> >    check_next = false;
> >    dom = domain;
> > }
> > do {
> >    switch (type) {
> >    case SYSDB_MEMBER_USER:
> >        DEBUG(SSSDBG_TRACE_FUNC, "Trying to find user %s@%s\n",
> >              name, dom->name);
> >        ret = sysdb_getpwnam(tmp_ctx, dom, name, &res);
> >        strtype = "user";
> >        break;
> >    case SYSDB_MEMBER_GROUP:
> >        DEBUG(SSSDBG_TRACE_FUNC, "Trying to find group %s@%s\n",
> >              name, dom->name);
> >        ret = sysdb_getgrnam(tmp_ctx, dom, name, &res);
> >        strtype = "group";
> >        break;
> >    default:
> >        DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported member type %d\n", type);
> >        ret = ERR_INTERNAL;
> >        goto done;
> >    }
> >    if (ret == EOK && res->count == 0) {
> >        ret = ENOENT;
> >        if (check_next) {
> >            dom = dom->next;
> >            continue;
> >        }
> >    }
> >    if (ret != EOK) {
> >        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to find %s %s@%s [%d]: %s\n",
> >              strtype, name, dom->name, ret, sss_strerror(ret));
> >        goto done;
> >    } else if (res->count != 1) {
> >        DEBUG(SSSDBG_CRIT_FAILURE, "More than one %s found?\n", strtype);
> >        ret = ERR_INTERNAL;
> >        goto done;
> >    }
> >    check_next = false;
> > } while (check_next && dom != NULL);
> > DEBUG(SSSDBG_TRACE_FUNC, "Domain of %s %s is %s\n",
> >      strtype, name, dom->name);
>
> Hmm looks like you should have a check for res validity here, for cases
> no domain match..

Added a dom validity.
> btw it might be nice to split this large function into smaller ones.
> Maybe the do-while check could be a separate function returning res.

There is no way to split it that would be actually beneficial. There are 
three blocks:
a) beginning - getpwnam
b) middle - iteration
c) end - linearized dn

If you take away the iteration you can directly return dn from the new 
function since you don't need res at all so that would also took away 
the end. And the beginning belongs to the function were iteration is IMHO.
> > dn = ldb_dn_get_linearized(res->msgs[0]->dn);
> > if (dn == NULL) {
> >    DEBUG(SSSDBG_CRIT_FAILURE, "ldb_dn_get_linearized() failed.\n");
> >    ret = ENOMEM;
> >    goto done;
> > }
> > talloc_steal(mem_ctx, dn);
> > *_new_domain = dom;
> > ret = EOK;
> >
> > +done:
> > talloc_free(tmp_ctx);
> > if (ret != EOK) {
> >    return NULL;
> > }
> > return dn;
> >
> > +}

[Jakub Hrozek]

On Mon, Jul 27, 2015 at 12:00:25PM +0200, Pavel Březina wrote:
> On 07/26/2015 08:14 PM, Jakub Hrozek wrote:
> > On Fri, Jul 24, 2015 at 01:08:17PM +0200, Pavel Březina wrote:
> > > https://fedorahosted.org/sssd/ticket/2584
> >
> > btw (unrelated to these patches) it seems strange to require the user of
> > the sysdb database to connect to the db and then separately initialize
> > subdomains...I would vote for merging that code..
>
> +1

I'll file a ticket for michal.
> > > +#include <talloc.h>
> > > +#include <popt.h>
> > >
> > > +#include "confdb/confdb.h"
> > >
> > > +struct sss_tool_ctx {
> > > struct confdb_ctx *confdb;
> > > char *confdb_path;
> > > char *default_domain;
> > > struct sss_domain_info *domains;
> >
> > I wonder if the tool_ctx must be open for consumers? Currently only
> > domains is used, maybe we could get away with a getter and an opaque
> > structure? We can always make the structure public in future, but if
> > it's public from the start, then it's easier to add cruft..
>
> I think it is not worth the effort with such simple structure. I am
> all for opaque structures but here is just no gain in code safety. It would
> only make code lines bigger.

Fine.
> > > +enum sss_tool_domain {
> > > SSS_TOOL_DOMAIN_REQUIRED,
> > > SSS_TOOL_DOMAIN_OPTIONAL
> > >
> > > +};
> > >
> > > +int sss_tool_parse_name(TALLOC_CTX *mem_ctx,
> > >                    struct sss_tool_ctx *tool_ctx,
> > >                    const char *input,
> > >                    enum sss_tool_domain require_domain,
> >
> > Do we need require_domain now that you added the getpwnam?
>
> No, removed.

Thanks

[...]
> > Hmm looks like you should have a check for res validity here, for cases
> > no domain match..
>
> Added a dom validity.

Thanks.
> > btw it might be nice to split this large function into smaller ones.
> > Maybe the do-while check could be a separate function returning res.
>
> There is no way to split it that would be actually beneficial. There are
> three blocks:
> a) beginning - getpwnam
> b) middle - iteration
> c) end - linearized dn
>
> If you take away the iteration you can directly return dn from the new
> function since you don't need res at all so that would also took away the
> end. And the beginning belongs to the function were iteration is IMHO.

OK
> From 45e80988edc6dab703209068942fad5378d38e72 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Fri, 24 Jul 2015 09:55:28 +0200
> Subject: [PATCH 1/3] SYSDB: prepare for LOCAL view
>
> Objects doesn't have to have overrideDN specified when using LOCAL view.
> Since the view is not stored on the server we do not want to contact
> LDAP therefore we special case LOCAL view saying that it is OK that
> this attribute is missing.
>
> Preparation for:
> https://fedorahosted.org/sssd/ticket/2584
>
> src/db/sysdb.h                      |  3 +-
>  src/db/sysdb_views.c                |  7 +++++
>  src/tests/cmocka/test_sysdb_views.c | 62 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 71 insertions(+), 1 deletion(-)
>
> diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> index 0f745ccb1a646d77ba4ad3d714d5f4dce0a51211..3bb2e50320594d1b6f7e3daa3a64134e46b60d22 100644
> --- a/src/db/sysdb.h
> +++ b/src/db/sysdb.h
> @@ -157,9 +157,10 @@
>  #define SYSDB_AD_ACCOUNT_EXPIRES "adAccountExpires"
>  #define SYSDB_AD_USER_ACCOUNT_CONTROL "adUserAccountControl"
>
> +#define SYSDB_DEFAULT_VIEW_NAME "default"
> +#define SYSDB_LOCAL_VIEW_NAME "LOCAL" /* reserved for client-side overrides */
>  #define SYSDB_VIEW_CLASS "view"
>  #define SYSDB_VIEW_NAME "viewName"
> -#define SYSDB_DEFAULT_VIEW_NAME "default"
>  #define SYSDB_OVERRIDE_CLASS "overrride"
>  #define SYSDB_OVERRIDE_ANCHOR_UUID "overrideAnchorUUID"
>  #define SYSDB_OVERRIDE_USER_CLASS "userOverride"
> diff --git a/src/db/sysdb_views.c b/src/db/sysdb_views.c
> index aadd6018f4d1e2ca33e2e00dd8b13b55a8c03f3e..f4560344e992d8245e37a5a4e2f74c7b70ce41ec 100644
> --- a/src/db/sysdb_views.c
> +++ b/src/db/sysdb_views.c
> @@ -1186,9 +1186,16 @@ errno_t sysdb_add_overrides_to_object(struct sss_domain_info *domain,
>          override_dn_str = ldb_msg_find_attr_as_string(obj,
>                                                        SYSDB_OVERRIDE_DN, NULL);
>          if (override_dn_str == NULL) {
>        if (strcmp(domain->view_name, SYSDB_LOCAL_VIEW_NAME) == 0) {
>            /* LOCAL view doesn't have to have overrideDN specified. */
>            ret = EOK;
>            goto done;
>        }
>        DEBUG(SSSDBG_CRIT_FAILURE,
>              "Missing override DN for objext [%s].\n",
>              ldb_dn_get_linearized(obj->dn));
>        ret = ENOENT;
>        goto done;
>    }
>
> diff --git a/src/tests/cmocka/test_sysdb_views.c b/src/tests/cmocka/test_sysdb_views.c
> index 1fb598219e9ee581e465ddbb32ba9f2544600c26..5d2d50ef94093664465305b53831ed878cf2c871 100644
> --- a/src/tests/cmocka/test_sysdb_views.c
> +++ b/src/tests/cmocka/test_sysdb_views.c
> @@ -275,6 +275,64 @@ void test_sysdb_add_overrides_to_object(void **state)
>      assert_int_equal(ldb_val_string_cmp(&el->values[1], "OVERRIDEKEY2"), 0);
>  }
>
> +void test_sysdb_add_overrides_to_object_local(void **state)
> +{
> int ret;
> struct ldb_message *orig;
> struct ldb_message_element *el;
> char *tmp_str;
> struct sysdb_test_ctx *test_ctx = talloc_get_type_abort(*state,
>                                                     struct sysdb_test_ctx);
> orig = ldb_msg_new(test_ctx);
> assert_non_null(orig);
> tmp_str = talloc_strdup(orig,  "ORIGNAME");

Please put assert_non_null here and to other assignments to tmp_str in
these two tests, otherwise ACK
> ret = ldb_msg_add_string(orig, SYSDB_NAME, tmp_str);
> assert_int_equal(ret, EOK);
> tmp_str = talloc_strdup(orig,  "ORIGGECOS");
> ret = ldb_msg_add_string(orig, SYSDB_GECOS, tmp_str);
> assert_int_equal(ret, EOK);
> test_ctx->domain->has_views = true;
> test_ctx->domain->view_name = "LOCAL";
> ret = sysdb_add_overrides_to_object(test_ctx->domain, orig, NULL, NULL);
> assert_int_equal(ret, EOK);
>
> +}
> From 5a7b46aee3acd9a1d40cc85cb8085e9d43f48a7e Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Wed, 22 Jul 2015 10:02:02 +0200
> Subject: [PATCH 2/3] TOOLS: add common command framework

ACK
> From 3babec59cb170457c441000b66ff76895526aba7 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Fri, 24 Jul 2015 09:58:11 +0200
> Subject: [PATCH 3/3] TOOLS: add sss_override for local overrides
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2584

There was one more Coverity warning:
Error: UNINIT (CWE-457): [#def1]
sssd-1.13.1/src/tools/sss_override.c:202: var_decl: Declaring variable "ret" without initializer.
sssd-1.13.1/src/tools/sss_override.c:252: uninit_use: Using uninitialized value "ret".
#  250|   
#  251|   done:
#  252|->     if (ret != EOK) {
#  253|           talloc_free(attrs);
#  254|           return NULL;

Otherwise looks good.

[Pavel Březina]

On 07/27/2015 05:07 PM, Jakub Hrozek wrote:
> On Mon, Jul 27, 2015 at 12:00:25PM +0200, Pavel Březina wrote:
> > On 07/26/2015 08:14 PM, Jakub Hrozek wrote:
> > > On Fri, Jul 24, 2015 at 01:08:17PM +0200, Pavel Březina wrote:
> > > > https://fedorahosted.org/sssd/ticket/2584
> > >
> > > btw (unrelated to these patches) it seems strange to require the user of
> > > the sysdb database to connect to the db and then separately initialize
> > > subdomains...I would vote for merging that code..
> >
> > +1
>
> I'll file a ticket for michal.
> > > > +#include <talloc.h>
> > > > +#include <popt.h>
> > > >
> > > > +#include "confdb/confdb.h"
> > > >
> > > > +struct sss_tool_ctx {
> > > > struct confdb_ctx *confdb;
> > > > char *confdb_path;
> > > > char *default_domain;
> > > > struct sss_domain_info *domains;
> > >
> > > I wonder if the tool_ctx must be open for consumers? Currently only
> > > domains is used, maybe we could get away with a getter and an opaque
> > > structure? We can always make the structure public in future, but if
> > > it's public from the start, then it's easier to add cruft..
> >
> > I think it is not worth the effort with such simple structure. I am
> > all for opaque structures but here is just no gain in code safety. It would
> > only make code lines bigger.
>
> Fine.
> > > > +enum sss_tool_domain {
> > > > SSS_TOOL_DOMAIN_REQUIRED,
> > > > SSS_TOOL_DOMAIN_OPTIONAL
> > > >
> > > > +};
> > > >
> > > > +int sss_tool_parse_name(TALLOC_CTX *mem_ctx,
> > > >                    struct sss_tool_ctx *tool_ctx,
> > > >                    const char *input,
> > > >                    enum sss_tool_domain require_domain,
> > >
> > > Do we need require_domain now that you added the getpwnam?
> >
> > No, removed.
>
> Thanks
>
> [...]
> > > Hmm looks like you should have a check for res validity here, for cases
> > > no domain match..
> >
> > Added a dom validity.
>
> Thanks.
> > > btw it might be nice to split this large function into smaller ones.
> > > Maybe the do-while check could be a separate function returning res.
> >
> > There is no way to split it that would be actually beneficial. There are
> > three blocks:
> > a) beginning - getpwnam
> > b) middle - iteration
> > c) end - linearized dn
> >
> > If you take away the iteration you can directly return dn from the new
> > function since you don't need res at all so that would also took away the
> > end. And the beginning belongs to the function were iteration is IMHO.
>
> OK
> > From 45e80988edc6dab703209068942fad5378d38e72 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Fri, 24 Jul 2015 09:55:28 +0200
> > Subject: [PATCH 1/3] SYSDB: prepare for LOCAL view
> >
> > Objects doesn't have to have overrideDN specified when using LOCAL view.
> > Since the view is not stored on the server we do not want to contact
> > LDAP therefore we special case LOCAL view saying that it is OK that
> > this attribute is missing.
> >
> > Preparation for:
> > https://fedorahosted.org/sssd/ticket/2584
> >
> > src/db/sysdb.h                      |  3 +-
> >   src/db/sysdb_views.c                |  7 +++++
> >   src/tests/cmocka/test_sysdb_views.c | 62 +++++++++++++++++++++++++++++++++++++
> >   3 files changed, 71 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> > index 0f745ccb1a646d77ba4ad3d714d5f4dce0a51211..3bb2e50320594d1b6f7e3daa3a64134e46b60d22 100644
> > --- a/src/db/sysdb.h
> > +++ b/src/db/sysdb.h
> > @@ -157,9 +157,10 @@
> >   #define SYSDB_AD_ACCOUNT_EXPIRES "adAccountExpires"
> >   #define SYSDB_AD_USER_ACCOUNT_CONTROL "adUserAccountControl"
> >
> > +#define SYSDB_DEFAULT_VIEW_NAME "default"
> > +#define SYSDB_LOCAL_VIEW_NAME "LOCAL" /* reserved for client-side overrides */
> >   #define SYSDB_VIEW_CLASS "view"
> >   #define SYSDB_VIEW_NAME "viewName"
> > -#define SYSDB_DEFAULT_VIEW_NAME "default"
> >   #define SYSDB_OVERRIDE_CLASS "overrride"
> >   #define SYSDB_OVERRIDE_ANCHOR_UUID "overrideAnchorUUID"
> >   #define SYSDB_OVERRIDE_USER_CLASS "userOverride"
> > diff --git a/src/db/sysdb_views.c b/src/db/sysdb_views.c
> > index aadd6018f4d1e2ca33e2e00dd8b13b55a8c03f3e..f4560344e992d8245e37a5a4e2f74c7b70ce41ec 100644
> > --- a/src/db/sysdb_views.c
> > +++ b/src/db/sysdb_views.c
> > @@ -1186,9 +1186,16 @@ errno_t sysdb_add_overrides_to_object(struct sss_domain_info *domain,
> >           override_dn_str = ldb_msg_find_attr_as_string(obj,
> >                                                         SYSDB_OVERRIDE_DN, NULL);
> >           if (override_dn_str == NULL) {
> >        if (strcmp(domain->view_name, SYSDB_LOCAL_VIEW_NAME) == 0) {
> >            /* LOCAL view doesn't have to have overrideDN specified. */
> >            ret = EOK;
> >            goto done;
> >        }
> >         DEBUG(SSSDBG_CRIT_FAILURE,
> >               "Missing override DN for objext [%s].\n",
> >               ldb_dn_get_linearized(obj->dn));
> >         ret = ENOENT;
> >         goto done;
> >     }
> >
> > diff --git a/src/tests/cmocka/test_sysdb_views.c b/src/tests/cmocka/test_sysdb_views.c
> > index 1fb598219e9ee581e465ddbb32ba9f2544600c26..5d2d50ef94093664465305b53831ed878cf2c871 100644
> > --- a/src/tests/cmocka/test_sysdb_views.c
> > +++ b/src/tests/cmocka/test_sysdb_views.c
> > @@ -275,6 +275,64 @@ void test_sysdb_add_overrides_to_object(void **state)
> >       assert_int_equal(ldb_val_string_cmp(&el->values[1], "OVERRIDEKEY2"), 0);
> >   }
> >
> > +void test_sysdb_add_overrides_to_object_local(void **state)
> > +{
> > int ret;
> > struct ldb_message *orig;
> > struct ldb_message_element *el;
> > char *tmp_str;
> > struct sysdb_test_ctx *test_ctx = talloc_get_type_abort(*state,
> >                                                     struct sysdb_test_ctx);
> > orig = ldb_msg_new(test_ctx);
> > assert_non_null(orig);
> > tmp_str = talloc_strdup(orig,  "ORIGNAME");
>
> Please put assert_non_null here and to other assignments to tmp_str in
> these two tests, otherwise ACK
> > ret = ldb_msg_add_string(orig, SYSDB_NAME, tmp_str);
> > assert_int_equal(ret, EOK);
> > tmp_str = talloc_strdup(orig,  "ORIGGECOS");
> > ret = ldb_msg_add_string(orig, SYSDB_GECOS, tmp_str);
> > assert_int_equal(ret, EOK);
> > test_ctx->domain->has_views = true;
> > test_ctx->domain->view_name = "LOCAL";
> > ret = sysdb_add_overrides_to_object(test_ctx->domain, orig, NULL, NULL);
> > assert_int_equal(ret, EOK);
> >
> > +}
> > From 5a7b46aee3acd9a1d40cc85cb8085e9d43f48a7e Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Wed, 22 Jul 2015 10:02:02 +0200
> > Subject: [PATCH 2/3] TOOLS: add common command framework
>
> ACK
> > From 3babec59cb170457c441000b66ff76895526aba7 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Fri, 24 Jul 2015 09:58:11 +0200
> > Subject: [PATCH 3/3] TOOLS: add sss_override for local overrides
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2584
>
> There was one more Coverity warning:
> Error: UNINIT (CWE-457): [#def1]
> sssd-1.13.1/src/tools/sss_override.c:202: var_decl: Declaring variable "ret" without initializer.
> sssd-1.13.1/src/tools/sss_override.c:252: uninit_use: Using uninitialized value "ret".
> #  250|
> #  251|   done:
> #  252|->     if (ret != EOK) {
> #  253|           talloc_free(attrs);
> #  254|           return NULL;
>
> Otherwise looks good.
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Thanks, new patches are attached. In addition to your comments:
- one new patch that adds null check in original unit test
- man page and tool change that says that zero ids are not supported

[Jakub Hrozek]

On Mon, Jul 27, 2015 at 05:27:29PM +0200, Pavel Březina wrote:
> On 07/27/2015 05:07 PM, Jakub Hrozek wrote:
> > On Mon, Jul 27, 2015 at 12:00:25PM +0200, Pavel Březina wrote:
> > > On 07/26/2015 08:14 PM, Jakub Hrozek wrote:
> > > > On Fri, Jul 24, 2015 at 01:08:17PM +0200, Pavel Březina wrote:
> > > > > https://fedorahosted.org/sssd/ticket/2584
> > > >
> > > > btw (unrelated to these patches) it seems strange to require the user of
> > > > the sysdb database to connect to the db and then separately initialize
> > > > subdomains...I would vote for merging that code..
> > >
> > > +1
> >
> > I'll file a ticket for michal.
> > > > > +#include <talloc.h>
> > > > > +#include <popt.h>
> > > > >
> > > > > +#include "confdb/confdb.h"
> > > > >
> > > > > +struct sss_tool_ctx {
> > > > > struct confdb_ctx *confdb;
> > > > > char *confdb_path;
> > > > > char *default_domain;
> > > > > struct sss_domain_info *domains;
> > > >
> > > > I wonder if the tool_ctx must be open for consumers? Currently only
> > > > domains is used, maybe we could get away with a getter and an opaque
> > > > structure? We can always make the structure public in future, but if
> > > > it's public from the start, then it's easier to add cruft..
> > >
> > > I think it is not worth the effort with such simple structure. I am
> > > all for opaque structures but here is just no gain in code safety. It would
> > > only make code lines bigger.
> >
> > Fine.
> > > > > +enum sss_tool_domain {
> > > > > SSS_TOOL_DOMAIN_REQUIRED,
> > > > > SSS_TOOL_DOMAIN_OPTIONAL
> > > > >
> > > > > +};
> > > > >
> > > > > +int sss_tool_parse_name(TALLOC_CTX *mem_ctx,
> > > > >                    struct sss_tool_ctx *tool_ctx,
> > > > >                    const char *input,
> > > > >                    enum sss_tool_domain require_domain,
> > > >
> > > > Do we need require_domain now that you added the getpwnam?
> > >
> > > No, removed.
> >
> > Thanks
> >
> > [...]
> > > > Hmm looks like you should have a check for res validity here, for cases
> > > > no domain match..
> > >
> > > Added a dom validity.
> >
> > Thanks.
> > > > btw it might be nice to split this large function into smaller ones.
> > > > Maybe the do-while check could be a separate function returning res.
> > >
> > > There is no way to split it that would be actually beneficial. There are
> > > three blocks:
> > > a) beginning - getpwnam
> > > b) middle - iteration
> > > c) end - linearized dn
> > >
> > > If you take away the iteration you can directly return dn from the new
> > > function since you don't need res at all so that would also took away the
> > > end. And the beginning belongs to the function were iteration is IMHO.
> >
> > OK
> > > From 45e80988edc6dab703209068942fad5378d38e72 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Fri, 24 Jul 2015 09:55:28 +0200
> > > Subject: [PATCH 1/3] SYSDB: prepare for LOCAL view
> > >
> > > Objects doesn't have to have overrideDN specified when using LOCAL view.
> > > Since the view is not stored on the server we do not want to contact
> > > LDAP therefore we special case LOCAL view saying that it is OK that
> > > this attribute is missing.
> > >
> > > Preparation for:
> > > https://fedorahosted.org/sssd/ticket/2584
> > >
> > > src/db/sysdb.h                      |  3 +-
> > >  src/db/sysdb_views.c                |  7 +++++
> > >  src/tests/cmocka/test_sysdb_views.c | 62 +++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 71 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> > > index 0f745ccb1a646d77ba4ad3d714d5f4dce0a51211..3bb2e50320594d1b6f7e3daa3a64134e46b60d22 100644
> > > --- a/src/db/sysdb.h
> > > +++ b/src/db/sysdb.h
> > > @@ -157,9 +157,10 @@
> > >  #define SYSDB_AD_ACCOUNT_EXPIRES "adAccountExpires"
> > >  #define SYSDB_AD_USER_ACCOUNT_CONTROL "adUserAccountControl"
> > >
> > > +#define SYSDB_DEFAULT_VIEW_NAME "default"
> > > +#define SYSDB_LOCAL_VIEW_NAME "LOCAL" /* reserved for client-side overrides */
> > >  #define SYSDB_VIEW_CLASS "view"
> > >  #define SYSDB_VIEW_NAME "viewName"
> > > -#define SYSDB_DEFAULT_VIEW_NAME "default"
> > >  #define SYSDB_OVERRIDE_CLASS "overrride"
> > >  #define SYSDB_OVERRIDE_ANCHOR_UUID "overrideAnchorUUID"
> > >  #define SYSDB_OVERRIDE_USER_CLASS "userOverride"
> > > diff --git a/src/db/sysdb_views.c b/src/db/sysdb_views.c
> > > index aadd6018f4d1e2ca33e2e00dd8b13b55a8c03f3e..f4560344e992d8245e37a5a4e2f74c7b70ce41ec 100644
> > > --- a/src/db/sysdb_views.c
> > > +++ b/src/db/sysdb_views.c
> > > @@ -1186,9 +1186,16 @@ errno_t sysdb_add_overrides_to_object(struct sss_domain_info *domain,
> > >          override_dn_str = ldb_msg_find_attr_as_string(obj,
> > >                                                        SYSDB_OVERRIDE_DN, NULL);
> > >          if (override_dn_str == NULL) {
> > >        if (strcmp(domain->view_name, SYSDB_LOCAL_VIEW_NAME) == 0) {
> > >            /* LOCAL view doesn't have to have overrideDN specified. */
> > >            ret = EOK;
> > >            goto done;
> > >        }
> > >        DEBUG(SSSDBG_CRIT_FAILURE,
> > >              "Missing override DN for objext [%s].\n",
> > >              ldb_dn_get_linearized(obj->dn));
> > >        ret = ENOENT;
> > >        goto done;
> > >    }
> > >
> > > diff --git a/src/tests/cmocka/test_sysdb_views.c b/src/tests/cmocka/test_sysdb_views.c
> > > index 1fb598219e9ee581e465ddbb32ba9f2544600c26..5d2d50ef94093664465305b53831ed878cf2c871 100644
> > > --- a/src/tests/cmocka/test_sysdb_views.c
> > > +++ b/src/tests/cmocka/test_sysdb_views.c
> > > @@ -275,6 +275,64 @@ void test_sysdb_add_overrides_to_object(void **state)
> > >      assert_int_equal(ldb_val_string_cmp(&el->values[1], "OVERRIDEKEY2"), 0);
> > >  }
> > >
> > > +void test_sysdb_add_overrides_to_object_local(void **state)
> > > +{
> > > int ret;
> > > struct ldb_message *orig;
> > > struct ldb_message_element *el;
> > > char *tmp_str;
> > > struct sysdb_test_ctx *test_ctx = talloc_get_type_abort(*state,
> > >                                                     struct sysdb_test_ctx);
> > > orig = ldb_msg_new(test_ctx);
> > > assert_non_null(orig);
> > > tmp_str = talloc_strdup(orig,  "ORIGNAME");
> >
> > Please put assert_non_null here and to other assignments to tmp_str in
> > these two tests, otherwise ACK
> > > ret = ldb_msg_add_string(orig, SYSDB_NAME, tmp_str);
> > > assert_int_equal(ret, EOK);
> > > tmp_str = talloc_strdup(orig,  "ORIGGECOS");
> > > ret = ldb_msg_add_string(orig, SYSDB_GECOS, tmp_str);
> > > assert_int_equal(ret, EOK);
> > > test_ctx->domain->has_views = true;
> > > test_ctx->domain->view_name = "LOCAL";
> > > ret = sysdb_add_overrides_to_object(test_ctx->domain, orig, NULL, NULL);
> > > assert_int_equal(ret, EOK);
> > >
> > > +}
> > > From 5a7b46aee3acd9a1d40cc85cb8085e9d43f48a7e Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Wed, 22 Jul 2015 10:02:02 +0200
> > > Subject: [PATCH 2/3] TOOLS: add common command framework
> >
> > ACK
> > > From 3babec59cb170457c441000b66ff76895526aba7 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Fri, 24 Jul 2015 09:58:11 +0200
> > > Subject: [PATCH 3/3] TOOLS: add sss_override for local overrides
> > >
> > > Resolves:
> > > https://fedorahosted.org/sssd/ticket/2584
> >
> > There was one more Coverity warning:
> > Error: UNINIT (CWE-457): [#def1]
> > sssd-1.13.1/src/tools/sss_override.c:202: var_decl: Declaring variable "ret" without initializer.
> > sssd-1.13.1/src/tools/sss_override.c:252: uninit_use: Using uninitialized value "ret".
> > #  250|
> > #  251|   done:
> > #  252|->     if (ret != EOK) {
> > #  253|           talloc_free(attrs);
> > #  254|           return NULL;
> >
> > Otherwise looks good.
> > _______________________________________________
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Thanks, new patches are attached. In addition to your comments:
> one new patch that adds null check in original unit test
> man page and tool change that says that zero ids are not supported

I am still waiting for Coverity and CI results.
> From db9ba08b8c359eec318fa3dfc28e038618ea35f7 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Mon, 27 Jul 2015 17:24:45 +0200
> Subject: [PATCH 1/4] VIEWS TEST: add null-check

ACK
> From 9b67c5777f51cedd1b6669d0f103c232e80e9ad6 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Fri, 24 Jul 2015 09:55:28 +0200
> Subject: [PATCH 2/4] SYSDB: prepare for LOCAL view

ACK
> From 39b1bf17cb52e000c8ef74862fb88a4762176730 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Wed, 22 Jul 2015 10:02:02 +0200
> Subject: [PATCH 3/4] TOOLS: add common command framework

ACK
> From 99c97b41a6feeb0edd5be11d27b8e832d4694843 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Fri, 24 Jul 2015 09:58:11 +0200
> Subject: [PATCH 4/4] TOOLS: add sss_override for local overrides

there is still an issue with user getpwnam finds but sysdb_getpwnam not:

(gdb) set args user-add root -n toor
(gdb) r
Starting program: /usr/sbin/sss_override user-add root -n toor
Missing separate debuginfos, use: dnf debuginfo-install glibc-2.21-5.fc22.x86_64
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
ldb: unable to dlopen /usr/lib64/ldb/modules/ldb/memberof.la : /usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header

Program received signal SIGSEGV, Segmentation fault.
override_user_add (cmdline=0x7fffffffe390, tool_ctx=0x6110c0, pvt=0x0)
    at /sssd/src/tools/sss_override.c:567
567             fprintf(stderr, _("Unable to find user %s@%s.\n"),
Missing separate debuginfos, use: dnf debuginfo-install bzip2-libs-1.0.6-14.fc22.x86_64 cyrus-sasl-lib-2.1.26-22.fc22.x86_64 dbus-libs-1.8.18-1.fc22.x86_64 elfutils-libelf-0.161-6.fc22.x86_64 elfutils-libs-0.161-6.fc22.x86_64 glib2-2.44.1-1.fc22.x86_64 libattr-2.4.47-9.fc22.x86_64 libbasicobjects-0.1.1-24.fc22.x86_64 libcap-2.24-7.fc22.x86_64 libcollection-0.6.2-24.fc22.x86_64 libdhash-0.4.3-24.fc22.x86_64 libgcc-5.1.1-1.fc22.x86_64 libgcrypt-1.6.3-4.fc22.x86_64 libgpg-error-1.17-2.fc22.x86_64 libini_config-1.1.0-24.fc22.x86_64 libldb-1.1.20-1.fc22.x86_64 libpath_utils-0.2.1-24.fc22.x86_64 libref_array-0.1.4-24.fc22.x86_64 libselinux-2.3-10.fc22.x86_64 libtalloc-2.1.2-1.fc22.x86_64 libtdb-1.3.4-1.fc22.x86_64 libtevent-0.9.24-1.fc22.x86_64 nspr-4.10.8-1.fc22.x86_64 nss-3.19.2-1.0.fc22.x86_64 nss-softokn-freebl-3.19.2-1.0.fc22.x86_64 nss-util-3.19.2-1.0.fc22.x86_64 openldap-2.4.40-12.fc22.x86_64 openssl-libs-1.0.1k-11.fc22.x86_64 pcre-8.37-2.fc22.x86_64 popt-1.16-5.fc22.x86_64 systemd-libs-219-19.fc22.x86_64 xz-libs-5.2.0-2.fc22.x86_64 zlib-1.2.8-7.fc22.x86_64
(gdb) list 567
562             return EXIT_FAILURE;
563         }
564
565         dn = get_user_dn_and_domain(tool_ctx, tool_ctx->domains, &user);
566         if (dn == NULL) {
567             fprintf(stderr, _("Unable to find user %s@%s.\n"),
568                     user.orig_name, user.domain->name);
569             return EXIT_FAILURE;
570         }
571
(gdb) p user
$1 = {input_name = 0x7fffffffe7b5 "root", orig_name = 0x611a30 "root", domain = 0x0, 
  name = 0x619c00 "toor", uid = 0, gid = 0, home = 0x0, shell = 0x0, gecos = 0x0}

Same issue is there with groups btw.

[Pavel Březina]

On 07/27/2015 05:27 PM, Pavel Březina wrote:
> On 07/27/2015 05:07 PM, Jakub Hrozek wrote:
> > On Mon, Jul 27, 2015 at 12:00:25PM +0200, Pavel Březina wrote:
> > > On 07/26/2015 08:14 PM, Jakub Hrozek wrote:
> > > > On Fri, Jul 24, 2015 at 01:08:17PM +0200, Pavel Březina wrote:
> > > > > https://fedorahosted.org/sssd/ticket/2584
> > > >
> > > > btw (unrelated to these patches) it seems strange to require the
> > > > user of
> > > > the sysdb database to connect to the db and then separately initialize
> > > > subdomains...I would vote for merging that code..
> > >
> > > +1
> >
> > I'll file a ticket for michal.
> > > > > +#include <talloc.h>
> > > > > +#include <popt.h>
> > > > >
> > > > > +#include "confdb/confdb.h"
> > > > >
> > > > > +struct sss_tool_ctx {
> > > > > struct confdb_ctx *confdb;
> > > > > char *confdb_path;
> > > > > char *default_domain;
> > > > > struct sss_domain_info *domains;
> > > >
> > > > I wonder if the tool_ctx must be open for consumers? Currently only
> > > > domains is used, maybe we could get away with a getter and an opaque
> > > > structure? We can always make the structure public in future, but if
> > > > it's public from the start, then it's easier to add cruft..
> > >
> > > I think it is not worth the effort with such simple structure. I am
> > > all for opaque structures but here is just no gain in code safety. It
> > > would
> > > only make code lines bigger.
> >
> > Fine.
> > > > > +enum sss_tool_domain {
> > > > > SSS_TOOL_DOMAIN_REQUIRED,
> > > > > SSS_TOOL_DOMAIN_OPTIONAL
> > > > >
> > > > > +};
> > > > >
> > > > > +int sss_tool_parse_name(TALLOC_CTX *mem_ctx,
> > > > >                    struct sss_tool_ctx *tool_ctx,
> > > > >                    const char *input,
> > > > >                    enum sss_tool_domain require_domain,
> > > >
> > > > Do we need require_domain now that you added the getpwnam?
> > >
> > > No, removed.
> >
> > Thanks
> >
> > [...]
> > > > Hmm looks like you should have a check for res validity here, for cases
> > > > no domain match..
> > >
> > > Added a dom validity.
> >
> > Thanks.
> > > > btw it might be nice to split this large function into smaller ones.
> > > > Maybe the do-while check could be a separate function returning res.
> > >
> > > There is no way to split it that would be actually beneficial. There are
> > > three blocks:
> > > a) beginning - getpwnam
> > > b) middle - iteration
> > > c) end - linearized dn
> > >
> > > If you take away the iteration you can directly return dn from the new
> > > function since you don't need res at all so that would also took away
> > > the
> > > end. And the beginning belongs to the function were iteration is IMHO.
> >
> > OK
> > > From 45e80988edc6dab703209068942fad5378d38e72 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Fri, 24 Jul 2015 09:55:28 +0200
> > > Subject: [PATCH 1/3] SYSDB: prepare for LOCAL view
> > >
> > > Objects doesn't have to have overrideDN specified when using LOCAL view.
> > > Since the view is not stored on the server we do not want to contact
> > > LDAP therefore we special case LOCAL view saying that it is OK that
> > > this attribute is missing.
> > >
> > > Preparation for:
> > > https://fedorahosted.org/sssd/ticket/2584
> > >
> > > src/db/sysdb.h                      |  3 +-
> > >   src/db/sysdb_views.c                |  7 +++++
> > >   src/tests/cmocka/test_sysdb_views.c | 62
> > > +++++++++++++++++++++++++++++++++++++
> > >   3 files changed, 71 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> > > index
> > > 0f745ccb1a646d77ba4ad3d714d5f4dce0a51211..3bb2e50320594d1b6f7e3daa3a64134e46b60d22
> > > 100644
> > > --- a/src/db/sysdb.h
> > > +++ b/src/db/sysdb.h
> > > @@ -157,9 +157,10 @@
> > >   #define SYSDB_AD_ACCOUNT_EXPIRES "adAccountExpires"
> > >   #define SYSDB_AD_USER_ACCOUNT_CONTROL "adUserAccountControl"
> > >
> > > +#define SYSDB_DEFAULT_VIEW_NAME "default"
> > > +#define SYSDB_LOCAL_VIEW_NAME "LOCAL" /* reserved for client-side
> > > overrides */
> > >   #define SYSDB_VIEW_CLASS "view"
> > >   #define SYSDB_VIEW_NAME "viewName"
> > > -#define SYSDB_DEFAULT_VIEW_NAME "default"
> > >   #define SYSDB_OVERRIDE_CLASS "overrride"
> > >   #define SYSDB_OVERRIDE_ANCHOR_UUID "overrideAnchorUUID"
> > >   #define SYSDB_OVERRIDE_USER_CLASS "userOverride"
> > > diff --git a/src/db/sysdb_views.c b/src/db/sysdb_views.c
> > > index
> > > aadd6018f4d1e2ca33e2e00dd8b13b55a8c03f3e..f4560344e992d8245e37a5a4e2f74c7b70ce41ec
> > > 100644
> > > --- a/src/db/sysdb_views.c
> > > +++ b/src/db/sysdb_views.c
> > > @@ -1186,9 +1186,16 @@ errno_t sysdb_add_overrides_to_object(struct
> > > sss_domain_info *domain,
> > >           override_dn_str = ldb_msg_find_attr_as_string(obj,
> > >
> > > SYSDB_OVERRIDE_DN, NULL);
> > >           if (override_dn_str == NULL) {
> > >        if (strcmp(domain->view_name, SYSDB_LOCAL_VIEW_NAME) ==
> > > {
> > >            /* LOCAL view doesn't have to have overrideDN
> > >
> > > specified. */
> > >            ret = EOK;
> > >            goto done;
> > >        }
> > >         DEBUG(SSSDBG_CRIT_FAILURE,
> > >               "Missing override DN for objext [%s].\n",
> > >               ldb_dn_get_linearized(obj->dn));
> > >         ret = ENOENT;
> > >         goto done;
> > >     }
> > >
> > > diff --git a/src/tests/cmocka/test_sysdb_views.c
> > > b/src/tests/cmocka/test_sysdb_views.c
> > > index
> > > 1fb598219e9ee581e465ddbb32ba9f2544600c26..5d2d50ef94093664465305b53831ed878cf2c871
> > > 100644
> > > --- a/src/tests/cmocka/test_sysdb_views.c
> > > +++ b/src/tests/cmocka/test_sysdb_views.c
> > > @@ -275,6 +275,64 @@ void test_sysdb_add_overrides_to_object(void
> > > **state)
> > >       assert_int_equal(ldb_val_string_cmp(&el->values[1],
> > > "OVERRIDEKEY2"), 0);
> > >   }
> > >
> > > +void test_sysdb_add_overrides_to_object_local(void **state)
> > > +{
> > > int ret;
> > > struct ldb_message *orig;
> > > struct ldb_message_element *el;
> > > char *tmp_str;
> > > struct sysdb_test_ctx *test_ctx = talloc_get_type_abort(*state,
> > >                                                     struct
> > >
> > > sysdb_test_ctx);
> > > orig = ldb_msg_new(test_ctx);
> > > assert_non_null(orig);
> > > tmp_str = talloc_strdup(orig,  "ORIGNAME");
> >
> > Please put assert_non_null here and to other assignments to tmp_str in
> > these two tests, otherwise ACK
> > > ret = ldb_msg_add_string(orig, SYSDB_NAME, tmp_str);
> > > assert_int_equal(ret, EOK);
> > > tmp_str = talloc_strdup(orig,  "ORIGGECOS");
> > > ret = ldb_msg_add_string(orig, SYSDB_GECOS, tmp_str);
> > > assert_int_equal(ret, EOK);
> > > test_ctx->domain->has_views = true;
> > > test_ctx->domain->view_name = "LOCAL";
> > > ret = sysdb_add_overrides_to_object(test_ctx->domain, orig,
> > >
> > > NULL, NULL);
> > > assert_int_equal(ret, EOK);
> > >
> > > +}
> > > From 5a7b46aee3acd9a1d40cc85cb8085e9d43f48a7e Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Wed, 22 Jul 2015 10:02:02 +0200
> > > Subject: [PATCH 2/3] TOOLS: add common command framework
> >
> > ACK
> > > From 3babec59cb170457c441000b66ff76895526aba7 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Fri, 24 Jul 2015 09:58:11 +0200
> > > Subject: [PATCH 3/3] TOOLS: add sss_override for local overrides
> > >
> > > Resolves:
> > > https://fedorahosted.org/sssd/ticket/2584
> >
> > There was one more Coverity warning:
> > Error: UNINIT (CWE-457): [#def1]
> > sssd-1.13.1/src/tools/sss_override.c:202: var_decl: Declaring variable
> > "ret" without initializer.
> > sssd-1.13.1/src/tools/sss_override.c:252: uninit_use: Using
> > uninitialized value "ret".
> > #  250|
> > #  251|   done:
> > #  252|->     if (ret != EOK) {
> > #  253|           talloc_free(attrs);
> > #  254|           return NULL;
> >
> > Otherwise looks good.
> > _______________________________________________
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Thanks, new patches are attached. In addition to your comments:
> one new patch that adds null check in original unit test
> man page and tool change that says that zero ids are not supported

New patches are attached as for irc discussion. Diff:

diff --git a/src/man/sss_override.8.xml b/src/man/sss_override.8.xml
index 88df5a0..307cb55 100644
--- a/src/man/sss_override.8.xml
+++ b/src/man/sss_override.8.xml
@@ -33,6 +33,10 @@
              view and allows to change selected values of specific user
              and groups. This change takes effect only on local machine.
          </para>
+        <para>
+            Overrides data are stored in SSSD cache. If the cache is 
deleted
+            all local overrides are lost.
+        </para>
      </refsect1>

<refsect1 id='commands'>
diff --git a/src/tools/sss_override.c b/src/tools/sss_override.c
index c007350..676327d 100644
--- a/src/tools/sss_override.c
+++ b/src/tools/sss_override.c
@@ -565,7 +565,8 @@ static int override_user_add(struct sss_cmdline 
*cmdline,
      dn = get_user_dn_and_domain(tool_ctx, tool_ctx->domains, &user);
      if (dn == NULL) {
          fprintf(stderr, _("Unable to find user %s@%s.\n"),
-                user.orig_name, user.domain->name);
+                user.orig_name,
+                user.domain == NULL ? "[unknown]" : user.domain->name);
          return EXIT_FAILURE;
      }

[Pavel Březina]

On 07/27/2015 05:27 PM, Pavel Březina wrote:
> On 07/27/2015 05:07 PM, Jakub Hrozek wrote:
> > On Mon, Jul 27, 2015 at 12:00:25PM +0200, Pavel Březina wrote:
> > > On 07/26/2015 08:14 PM, Jakub Hrozek wrote:
> > > > On Fri, Jul 24, 2015 at 01:08:17PM +0200, Pavel Březina wrote:
> > > > > https://fedorahosted.org/sssd/ticket/2584
> > > >
> > > > btw (unrelated to these patches) it seems strange to require the
> > > > user of
> > > > the sysdb database to connect to the db and then separately initialize
> > > > subdomains...I would vote for merging that code..
> > >
> > > +1
> >
> > I'll file a ticket for michal.
> > > > > +#include <talloc.h>
> > > > > +#include <popt.h>
> > > > >
> > > > > +#include "confdb/confdb.h"
> > > > >
> > > > > +struct sss_tool_ctx {
> > > > > struct confdb_ctx *confdb;
> > > > > char *confdb_path;
> > > > > char *default_domain;
> > > > > struct sss_domain_info *domains;
> > > >
> > > > I wonder if the tool_ctx must be open for consumers? Currently only
> > > > domains is used, maybe we could get away with a getter and an opaque
> > > > structure? We can always make the structure public in future, but if
> > > > it's public from the start, then it's easier to add cruft..
> > >
> > > I think it is not worth the effort with such simple structure. I am
> > > all for opaque structures but here is just no gain in code safety. It
> > > would
> > > only make code lines bigger.
> >
> > Fine.
> > > > > +enum sss_tool_domain {
> > > > > SSS_TOOL_DOMAIN_REQUIRED,
> > > > > SSS_TOOL_DOMAIN_OPTIONAL
> > > > >
> > > > > +};
> > > > >
> > > > > +int sss_tool_parse_name(TALLOC_CTX *mem_ctx,
> > > > >                    struct sss_tool_ctx *tool_ctx,
> > > > >                    const char *input,
> > > > >                    enum sss_tool_domain require_domain,
> > > >
> > > > Do we need require_domain now that you added the getpwnam?
> > >
> > > No, removed.
> >
> > Thanks
> >
> > [...]
> > > > Hmm looks like you should have a check for res validity here, for cases
> > > > no domain match..
> > >
> > > Added a dom validity.
> >
> > Thanks.
> > > > btw it might be nice to split this large function into smaller ones.
> > > > Maybe the do-while check could be a separate function returning res.
> > >
> > > There is no way to split it that would be actually beneficial. There are
> > > three blocks:
> > > a) beginning - getpwnam
> > > b) middle - iteration
> > > c) end - linearized dn
> > >
> > > If you take away the iteration you can directly return dn from the new
> > > function since you don't need res at all so that would also took away
> > > the
> > > end. And the beginning belongs to the function were iteration is IMHO.
> >
> > OK
> > > From 45e80988edc6dab703209068942fad5378d38e72 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Fri, 24 Jul 2015 09:55:28 +0200
> > > Subject: [PATCH 1/3] SYSDB: prepare for LOCAL view
> > >
> > > Objects doesn't have to have overrideDN specified when using LOCAL view.
> > > Since the view is not stored on the server we do not want to contact
> > > LDAP therefore we special case LOCAL view saying that it is OK that
> > > this attribute is missing.
> > >
> > > Preparation for:
> > > https://fedorahosted.org/sssd/ticket/2584
> > >
> > > src/db/sysdb.h                      |  3 +-
> > >   src/db/sysdb_views.c                |  7 +++++
> > >   src/tests/cmocka/test_sysdb_views.c | 62
> > > +++++++++++++++++++++++++++++++++++++
> > >   3 files changed, 71 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> > > index
> > > 0f745ccb1a646d77ba4ad3d714d5f4dce0a51211..3bb2e50320594d1b6f7e3daa3a64134e46b60d22
> > > 100644
> > > --- a/src/db/sysdb.h
> > > +++ b/src/db/sysdb.h
> > > @@ -157,9 +157,10 @@
> > >   #define SYSDB_AD_ACCOUNT_EXPIRES "adAccountExpires"
> > >   #define SYSDB_AD_USER_ACCOUNT_CONTROL "adUserAccountControl"
> > >
> > > +#define SYSDB_DEFAULT_VIEW_NAME "default"
> > > +#define SYSDB_LOCAL_VIEW_NAME "LOCAL" /* reserved for client-side
> > > overrides */
> > >   #define SYSDB_VIEW_CLASS "view"
> > >   #define SYSDB_VIEW_NAME "viewName"
> > > -#define SYSDB_DEFAULT_VIEW_NAME "default"
> > >   #define SYSDB_OVERRIDE_CLASS "overrride"
> > >   #define SYSDB_OVERRIDE_ANCHOR_UUID "overrideAnchorUUID"
> > >   #define SYSDB_OVERRIDE_USER_CLASS "userOverride"
> > > diff --git a/src/db/sysdb_views.c b/src/db/sysdb_views.c
> > > index
> > > aadd6018f4d1e2ca33e2e00dd8b13b55a8c03f3e..f4560344e992d8245e37a5a4e2f74c7b70ce41ec
> > > 100644
> > > --- a/src/db/sysdb_views.c
> > > +++ b/src/db/sysdb_views.c
> > > @@ -1186,9 +1186,16 @@ errno_t sysdb_add_overrides_to_object(struct
> > > sss_domain_info *domain,
> > >           override_dn_str = ldb_msg_find_attr_as_string(obj,
> > >
> > > SYSDB_OVERRIDE_DN, NULL);
> > >           if (override_dn_str == NULL) {
> > >        if (strcmp(domain->view_name, SYSDB_LOCAL_VIEW_NAME) ==
> > > {
> > >            /* LOCAL view doesn't have to have overrideDN
> > >
> > > specified. */
> > >            ret = EOK;
> > >            goto done;
> > >        }
> > >         DEBUG(SSSDBG_CRIT_FAILURE,
> > >               "Missing override DN for objext [%s].\n",
> > >               ldb_dn_get_linearized(obj->dn));
> > >         ret = ENOENT;
> > >         goto done;
> > >     }
> > >
> > > diff --git a/src/tests/cmocka/test_sysdb_views.c
> > > b/src/tests/cmocka/test_sysdb_views.c
> > > index
> > > 1fb598219e9ee581e465ddbb32ba9f2544600c26..5d2d50ef94093664465305b53831ed878cf2c871
> > > 100644
> > > --- a/src/tests/cmocka/test_sysdb_views.c
> > > +++ b/src/tests/cmocka/test_sysdb_views.c
> > > @@ -275,6 +275,64 @@ void test_sysdb_add_overrides_to_object(void
> > > **state)
> > >       assert_int_equal(ldb_val_string_cmp(&el->values[1],
> > > "OVERRIDEKEY2"), 0);
> > >   }
> > >
> > > +void test_sysdb_add_overrides_to_object_local(void **state)
> > > +{
> > > int ret;
> > > struct ldb_message *orig;
> > > struct ldb_message_element *el;
> > > char *tmp_str;
> > > struct sysdb_test_ctx *test_ctx = talloc_get_type_abort(*state,
> > >                                                     struct
> > >
> > > sysdb_test_ctx);
> > > orig = ldb_msg_new(test_ctx);
> > > assert_non_null(orig);
> > > tmp_str = talloc_strdup(orig,  "ORIGNAME");
> >
> > Please put assert_non_null here and to other assignments to tmp_str in
> > these two tests, otherwise ACK
> > > ret = ldb_msg_add_string(orig, SYSDB_NAME, tmp_str);
> > > assert_int_equal(ret, EOK);
> > > tmp_str = talloc_strdup(orig,  "ORIGGECOS");
> > > ret = ldb_msg_add_string(orig, SYSDB_GECOS, tmp_str);
> > > assert_int_equal(ret, EOK);
> > > test_ctx->domain->has_views = true;
> > > test_ctx->domain->view_name = "LOCAL";
> > > ret = sysdb_add_overrides_to_object(test_ctx->domain, orig,
> > >
> > > NULL, NULL);
> > > assert_int_equal(ret, EOK);
> > >
> > > +}
> > > From 5a7b46aee3acd9a1d40cc85cb8085e9d43f48a7e Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Wed, 22 Jul 2015 10:02:02 +0200
> > > Subject: [PATCH 2/3] TOOLS: add common command framework
> >
> > ACK
> > > From 3babec59cb170457c441000b66ff76895526aba7 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Fri, 24 Jul 2015 09:58:11 +0200
> > > Subject: [PATCH 3/3] TOOLS: add sss_override for local overrides
> > >
> > > Resolves:
> > > https://fedorahosted.org/sssd/ticket/2584
> >
> > There was one more Coverity warning:
> > Error: UNINIT (CWE-457): [#def1]
> > sssd-1.13.1/src/tools/sss_override.c:202: var_decl: Declaring variable
> > "ret" without initializer.
> > sssd-1.13.1/src/tools/sss_override.c:252: uninit_use: Using
> > uninitialized value "ret".
> > #  250|
> > #  251|   done:
> > #  252|->     if (ret != EOK) {
> > #  253|           talloc_free(attrs);
> > #  254|           return NULL;
> >
> > Otherwise looks good.
> > _______________________________________________
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Thanks, new patches are attached. In addition to your comments:
> one new patch that adds null check in original unit test
> man page and tool change that says that zero ids are not supported

Few more bug fixed during irc review. Thanks Jakub!

[Jakub Hrozek]

On Mon, Jul 27, 2015 at 07:51:06PM +0200, Pavel Březina wrote:
> On 07/27/2015 05:27 PM, Pavel Březina wrote:
> > On 07/27/2015 05:07 PM, Jakub Hrozek wrote:
> > > On Mon, Jul 27, 2015 at 12:00:25PM +0200, Pavel Březina wrote:
> > > > On 07/26/2015 08:14 PM, Jakub Hrozek wrote:
> > > > > On Fri, Jul 24, 2015 at 01:08:17PM +0200, Pavel Březina wrote:
> > > > > > https://fedorahosted.org/sssd/ticket/2584
> > > > >
> > > > > btw (unrelated to these patches) it seems strange to require the
> > > > > user of
> > > > > the sysdb database to connect to the db and then separately initialize
> > > > > subdomains...I would vote for merging that code..
> > > >
> > > > +1
> > >
> > > I'll file a ticket for michal.
> > > > > > +#include <talloc.h>
> > > > > > +#include <popt.h>
> > > > > >
> > > > > > +#include "confdb/confdb.h"
> > > > > >
> > > > > > +struct sss_tool_ctx {
> > > > > > struct confdb_ctx *confdb;
> > > > > > char *confdb_path;
> > > > > > char *default_domain;
> > > > > > struct sss_domain_info *domains;
> > > > >
> > > > > I wonder if the tool_ctx must be open for consumers? Currently only
> > > > > domains is used, maybe we could get away with a getter and an opaque
> > > > > structure? We can always make the structure public in future, but if
> > > > > it's public from the start, then it's easier to add cruft..
> > > >
> > > > I think it is not worth the effort with such simple structure. I am
> > > > all for opaque structures but here is just no gain in code safety. It
> > > > would
> > > > only make code lines bigger.
> > >
> > > Fine.
> > > > > > +enum sss_tool_domain {
> > > > > > SSS_TOOL_DOMAIN_REQUIRED,
> > > > > > SSS_TOOL_DOMAIN_OPTIONAL
> > > > > >
> > > > > > +};
> > > > > >
> > > > > > +int sss_tool_parse_name(TALLOC_CTX *mem_ctx,
> > > > > >                    struct sss_tool_ctx *tool_ctx,
> > > > > >                    const char *input,
> > > > > >                    enum sss_tool_domain require_domain,
> > > > >
> > > > > Do we need require_domain now that you added the getpwnam?
> > > >
> > > > No, removed.
> > >
> > > Thanks
> > >
> > > [...]
> > > > > Hmm looks like you should have a check for res validity here, for cases
> > > > > no domain match..
> > > >
> > > > Added a dom validity.
> > >
> > > Thanks.
> > > > > btw it might be nice to split this large function into smaller ones.
> > > > > Maybe the do-while check could be a separate function returning res.
> > > >
> > > > There is no way to split it that would be actually beneficial. There are
> > > > three blocks:
> > > > a) beginning - getpwnam
> > > > b) middle - iteration
> > > > c) end - linearized dn
> > > >
> > > > If you take away the iteration you can directly return dn from the new
> > > > function since you don't need res at all so that would also took away
> > > > the
> > > > end. And the beginning belongs to the function were iteration is IMHO.
> > >
> > > OK
> > > > From 45e80988edc6dab703209068942fad5378d38e72 Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > Date: Fri, 24 Jul 2015 09:55:28 +0200
> > > > Subject: [PATCH 1/3] SYSDB: prepare for LOCAL view
> > > >
> > > > Objects doesn't have to have overrideDN specified when using LOCAL view.
> > > > Since the view is not stored on the server we do not want to contact
> > > > LDAP therefore we special case LOCAL view saying that it is OK that
> > > > this attribute is missing.
> > > >
> > > > Preparation for:
> > > > https://fedorahosted.org/sssd/ticket/2584
> > > >
> > > > src/db/sysdb.h                      |  3 +-
> > > >  src/db/sysdb_views.c                |  7 +++++
> > > >  src/tests/cmocka/test_sysdb_views.c | 62
> > > > +++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 71 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> > > > index
> > > > 0f745ccb1a646d77ba4ad3d714d5f4dce0a51211..3bb2e50320594d1b6f7e3daa3a64134e46b60d22
> > > > 100644
> > > > --- a/src/db/sysdb.h
> > > > +++ b/src/db/sysdb.h
> > > > @@ -157,9 +157,10 @@
> > > >  #define SYSDB_AD_ACCOUNT_EXPIRES "adAccountExpires"
> > > >  #define SYSDB_AD_USER_ACCOUNT_CONTROL "adUserAccountControl"
> > > >
> > > > +#define SYSDB_DEFAULT_VIEW_NAME "default"
> > > > +#define SYSDB_LOCAL_VIEW_NAME "LOCAL" /* reserved for client-side
> > > > overrides */
> > > >  #define SYSDB_VIEW_CLASS "view"
> > > >  #define SYSDB_VIEW_NAME "viewName"
> > > > -#define SYSDB_DEFAULT_VIEW_NAME "default"
> > > >  #define SYSDB_OVERRIDE_CLASS "overrride"
> > > >  #define SYSDB_OVERRIDE_ANCHOR_UUID "overrideAnchorUUID"
> > > >  #define SYSDB_OVERRIDE_USER_CLASS "userOverride"
> > > > diff --git a/src/db/sysdb_views.c b/src/db/sysdb_views.c
> > > > index
> > > > aadd6018f4d1e2ca33e2e00dd8b13b55a8c03f3e..f4560344e992d8245e37a5a4e2f74c7b70ce41ec
> > > > 100644
> > > > --- a/src/db/sysdb_views.c
> > > > +++ b/src/db/sysdb_views.c
> > > > @@ -1186,9 +1186,16 @@ errno_t sysdb_add_overrides_to_object(struct
> > > > sss_domain_info *domain,
> > > >          override_dn_str = ldb_msg_find_attr_as_string(obj,
> > > >
> > > > SYSDB_OVERRIDE_DN, NULL);
> > > >          if (override_dn_str == NULL) {
> > > >        if (strcmp(domain->view_name, SYSDB_LOCAL_VIEW_NAME) ==
> > > > {
> > > >            /* LOCAL view doesn't have to have overrideDN
> > > >
> > > > specified. */
> > > >            ret = EOK;
> > > >            goto done;
> > > >        }
> > > >        DEBUG(SSSDBG_CRIT_FAILURE,
> > > >              "Missing override DN for objext [%s].\n",
> > > >              ldb_dn_get_linearized(obj->dn));
> > > >        ret = ENOENT;
> > > >        goto done;
> > > >    }
> > > >
> > > > diff --git a/src/tests/cmocka/test_sysdb_views.c
> > > > b/src/tests/cmocka/test_sysdb_views.c
> > > > index
> > > > 1fb598219e9ee581e465ddbb32ba9f2544600c26..5d2d50ef94093664465305b53831ed878cf2c871
> > > > 100644
> > > > --- a/src/tests/cmocka/test_sysdb_views.c
> > > > +++ b/src/tests/cmocka/test_sysdb_views.c
> > > > @@ -275,6 +275,64 @@ void test_sysdb_add_overrides_to_object(void
> > > > **state)
> > > >      assert_int_equal(ldb_val_string_cmp(&el->values[1],
> > > > "OVERRIDEKEY2"), 0);
> > > >  }
> > > >
> > > > +void test_sysdb_add_overrides_to_object_local(void **state)
> > > > +{
> > > > int ret;
> > > > struct ldb_message *orig;
> > > > struct ldb_message_element *el;
> > > > char *tmp_str;
> > > > struct sysdb_test_ctx *test_ctx = talloc_get_type_abort(*state,
> > > >                                                     struct
> > > >
> > > > sysdb_test_ctx);
> > > > orig = ldb_msg_new(test_ctx);
> > > > assert_non_null(orig);
> > > > tmp_str = talloc_strdup(orig,  "ORIGNAME");
> > >
> > > Please put assert_non_null here and to other assignments to tmp_str in
> > > these two tests, otherwise ACK
> > > > ret = ldb_msg_add_string(orig, SYSDB_NAME, tmp_str);
> > > > assert_int_equal(ret, EOK);
> > > > tmp_str = talloc_strdup(orig,  "ORIGGECOS");
> > > > ret = ldb_msg_add_string(orig, SYSDB_GECOS, tmp_str);
> > > > assert_int_equal(ret, EOK);
> > > > test_ctx->domain->has_views = true;
> > > > test_ctx->domain->view_name = "LOCAL";
> > > > ret = sysdb_add_overrides_to_object(test_ctx->domain, orig,
> > > >
> > > > NULL, NULL);
> > > > assert_int_equal(ret, EOK);
> > > >
> > > > +}
> > > > From 5a7b46aee3acd9a1d40cc85cb8085e9d43f48a7e Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > Date: Wed, 22 Jul 2015 10:02:02 +0200
> > > > Subject: [PATCH 2/3] TOOLS: add common command framework
> > >
> > > ACK
> > > > From 3babec59cb170457c441000b66ff76895526aba7 Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > Date: Fri, 24 Jul 2015 09:58:11 +0200
> > > > Subject: [PATCH 3/3] TOOLS: add sss_override for local overrides
> > > >
> > > > Resolves:
> > > > https://fedorahosted.org/sssd/ticket/2584
> > >
> > > There was one more Coverity warning:
> > > Error: UNINIT (CWE-457): [#def1]
> > > sssd-1.13.1/src/tools/sss_override.c:202: var_decl: Declaring variable
> > > "ret" without initializer.
> > > sssd-1.13.1/src/tools/sss_override.c:252: uninit_use: Using
> > > uninitialized value "ret".
> > > #  250|
> > > #  251|   done:
> > > #  252|->     if (ret != EOK) {
> > > #  253|           talloc_free(attrs);
> > > #  254|           return NULL;
> > >
> > > Otherwise looks good.
> > > _______________________________________________
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > Thanks, new patches are attached. In addition to your comments:
> > one new patch that adds null check in original unit test
> > man page and tool change that says that zero ids are not supported
>
> Few more bug fixed during irc review. Thanks Jakub!

These patches work well for me, all attributes can be overriden,
existing overrides in IPA case are detected and the crash is gone.

Both CI and Coverity are happy as well.

ACK

[Jakub Hrozek]

On Mon, Jul 27, 2015 at 10:00:03PM +0200, Jakub Hrozek wrote:
> On Mon, Jul 27, 2015 at 07:51:06PM +0200, Pavel Březina wrote:
> > On 07/27/2015 05:27 PM, Pavel Březina wrote:
> > > On 07/27/2015 05:07 PM, Jakub Hrozek wrote:
> > > > On Mon, Jul 27, 2015 at 12:00:25PM +0200, Pavel Březina wrote:
> > > > > On 07/26/2015 08:14 PM, Jakub Hrozek wrote:
> > > > > > On Fri, Jul 24, 2015 at 01:08:17PM +0200, Pavel Březina wrote:
> > > > > > >https://fedorahosted.org/sssd/ticket/2584
> > > > > > btw (unrelated to these patches) it seems strange to require the
> > > > > > user of
> > > > > > the sysdb database to connect to the db and then separately initialize
> > > > > > subdomains...I would vote for merging that code..
> > > > >
> > > > > +1
> > > >
> > > > I'll file a ticket for michal.
> > > > > > >+
> > > > > > >+#include <talloc.h>
> > > > > > >+#include <popt.h>
> > > > > > >+
> > > > > > >+#include "confdb/confdb.h"
> > > > > > >+
> > > > > > >+struct sss_tool_ctx {
> > > > > > >+    struct confdb_ctx *confdb;
> > > > > > >+    char *confdb_path;
> > > > > > >+
> > > > > > >+    char *default_domain;
> > > > > > >+    struct sss_domain_info *domains;
> > > > > >
> > > > > > I wonder if the tool_ctx must be open for consumers? Currently only
> > > > > > domains is used, maybe we could get away with a getter and an opaque
> > > > > > structure? We can always make the structure public in future, but if
> > > > > > it's public from the start, then it's easier to add cruft..
> > > > >
> > > > > I think it is not worth the effort with such simple structure. I am
> > > > > all for opaque structures but here is just no gain in code safety. It
> > > > > would
> > > > > only make code lines bigger.
> > > >
> > > > Fine.
> > > > > > >+enum sss_tool_domain {
> > > > > > >+    SSS_TOOL_DOMAIN_REQUIRED,
> > > > > > >+    SSS_TOOL_DOMAIN_OPTIONAL
> > > > > > >+};
> > > > > > >+
> > > > > > >+int sss_tool_parse_name(TALLOC_CTX *mem_ctx,
> > > > > > >+                        struct sss_tool_ctx *tool_ctx,
> > > > > > >+                        const char *input,
> > > > > > >+                        enum sss_tool_domain require_domain,
> > > > > >
> > > > > > Do we need require_domain now that you added the getpwnam?
> > > > >
> > > > > No, removed.
> > > >
> > > > Thanks
> > > >
> > > > [...]
> > > > > > Hmm looks like you should have a check for res validity here, for cases
> > > > > > no domain match..
> > > > >
> > > > > Added a dom validity.
> > > >
> > > > Thanks.
> > > > > > btw it might be nice to split this large function into smaller ones.
> > > > > > Maybe the do-while check could be a separate function returning res.
> > > > >
> > > > > There is no way to split it that would be actually beneficial. There are
> > > > > three blocks:
> > > > > a) beginning - getpwnam
> > > > > b) middle - iteration
> > > > > c) end - linearized dn
> > > > >
> > > > > If you take away the iteration you can directly return dn from the new
> > > > > function since you don't need res at all so that would also took away
> > > > > the
> > > > > end. And the beginning belongs to the function were iteration is IMHO.
> > > >
> > > > OK
> > > > > From 45e80988edc6dab703209068942fad5378d38e72 Mon Sep 17 00:00:00 2001
> > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > Date: Fri, 24 Jul 2015 09:55:28 +0200
> > > > > Subject: [PATCH 1/3] SYSDB: prepare for LOCAL view
> > > > >
> > > > > Objects doesn't have to have overrideDN specified when using LOCAL view.
> > > > > Since the view is not stored on the server we do not want to contact
> > > > > LDAP therefore we special case LOCAL view saying that it is OK that
> > > > > this attribute is missing.
> > > > >
> > > > > Preparation for:
> > > > > https://fedorahosted.org/sssd/ticket/2584
> > > > >
> > > > > src/db/sysdb.h                      |  3 +-
> > > > >  src/db/sysdb_views.c                |  7 +++++
> > > > >  src/tests/cmocka/test_sysdb_views.c | 62
> > > > > +++++++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 71 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> > > > > index
> > > > > 0f745ccb1a646d77ba4ad3d714d5f4dce0a51211..3bb2e50320594d1b6f7e3daa3a64134e46b60d22
> > > > > 100644
> > > > > --- a/src/db/sysdb.h
> > > > > +++ b/src/db/sysdb.h
> > > > > @@ -157,9 +157,10 @@
> > > > >  #define SYSDB_AD_ACCOUNT_EXPIRES "adAccountExpires"
> > > > >  #define SYSDB_AD_USER_ACCOUNT_CONTROL "adUserAccountControl"
> > > > >
> > > > > +#define SYSDB_DEFAULT_VIEW_NAME "default"
> > > > > +#define SYSDB_LOCAL_VIEW_NAME "LOCAL" /* reserved for client-side
> > > > > overrides */
> > > > >  #define SYSDB_VIEW_CLASS "view"
> > > > >  #define SYSDB_VIEW_NAME "viewName"
> > > > > -#define SYSDB_DEFAULT_VIEW_NAME "default"
> > > > >  #define SYSDB_OVERRIDE_CLASS "overrride"
> > > > >  #define SYSDB_OVERRIDE_ANCHOR_UUID "overrideAnchorUUID"
> > > > >  #define SYSDB_OVERRIDE_USER_CLASS "userOverride"
> > > > > diff --git a/src/db/sysdb_views.c b/src/db/sysdb_views.c
> > > > > index
> > > > > aadd6018f4d1e2ca33e2e00dd8b13b55a8c03f3e..f4560344e992d8245e37a5a4e2f74c7b70ce41ec
> > > > > 100644
> > > > > --- a/src/db/sysdb_views.c
> > > > > +++ b/src/db/sysdb_views.c
> > > > > @@ -1186,9 +1186,16 @@ errno_t sysdb_add_overrides_to_object(struct
> > > > > sss_domain_info *domain,
> > > > >          override_dn_str = ldb_msg_find_attr_as_string(obj,
> > > > >
> > > > > SYSDB_OVERRIDE_DN, NULL);
> > > > >          if (override_dn_str == NULL) {
> > > > >        if (strcmp(domain->view_name, SYSDB_LOCAL_VIEW_NAME) ==
> > > > > {
> > > > >            /* LOCAL view doesn't have to have overrideDN
> > > > >
> > > > > specified. */
> > > > >            ret = EOK;
> > > > >            goto done;
> > > > >        }
> > > > >        DEBUG(SSSDBG_CRIT_FAILURE,
> > > > >              "Missing override DN for objext [%s].\n",
> > > > >              ldb_dn_get_linearized(obj->dn));
> > > > >        ret = ENOENT;
> > > > >        goto done;
> > > > >    }
> > > > >
> > > > > diff --git a/src/tests/cmocka/test_sysdb_views.c
> > > > > b/src/tests/cmocka/test_sysdb_views.c
> > > > > index
> > > > > 1fb598219e9ee581e465ddbb32ba9f2544600c26..5d2d50ef94093664465305b53831ed878cf2c871
> > > > > 100644
> > > > > --- a/src/tests/cmocka/test_sysdb_views.c
> > > > > +++ b/src/tests/cmocka/test_sysdb_views.c
> > > > > @@ -275,6 +275,64 @@ void test_sysdb_add_overrides_to_object(void
> > > > > **state)
> > > > >      assert_int_equal(ldb_val_string_cmp(&el->values[1],
> > > > > "OVERRIDEKEY2"), 0);
> > > > >  }
> > > > >
> > > > > +void test_sysdb_add_overrides_to_object_local(void **state)
> > > > > +{
> > > > > int ret;
> > > > > struct ldb_message *orig;
> > > > > struct ldb_message_element *el;
> > > > > char *tmp_str;
> > > > > struct sysdb_test_ctx *test_ctx = talloc_get_type_abort(*state,
> > > > >                                                     struct
> > > > >
> > > > > sysdb_test_ctx);
> > > > > orig = ldb_msg_new(test_ctx);
> > > > > assert_non_null(orig);
> > > > > tmp_str = talloc_strdup(orig,  "ORIGNAME");
> > > >
> > > > Please put assert_non_null here and to other assignments to tmp_str in
> > > > these two tests, otherwise ACK
> > > > > ret = ldb_msg_add_string(orig, SYSDB_NAME, tmp_str);
> > > > > assert_int_equal(ret, EOK);
> > > > > tmp_str = talloc_strdup(orig,  "ORIGGECOS");
> > > > > ret = ldb_msg_add_string(orig, SYSDB_GECOS, tmp_str);
> > > > > assert_int_equal(ret, EOK);
> > > > > test_ctx->domain->has_views = true;
> > > > > test_ctx->domain->view_name = "LOCAL";
> > > > > ret = sysdb_add_overrides_to_object(test_ctx->domain, orig,
> > > > >
> > > > > NULL, NULL);
> > > > > assert_int_equal(ret, EOK);
> > > > >
> > > > > +}
> > > > > From 5a7b46aee3acd9a1d40cc85cb8085e9d43f48a7e Mon Sep 17 00:00:00 2001
> > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > Date: Wed, 22 Jul 2015 10:02:02 +0200
> > > > > Subject: [PATCH 2/3] TOOLS: add common command framework
> > > >
> > > > ACK
> > > > > From 3babec59cb170457c441000b66ff76895526aba7 Mon Sep 17 00:00:00 2001
> > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > Date: Fri, 24 Jul 2015 09:58:11 +0200
> > > > > Subject: [PATCH 3/3] TOOLS: add sss_override for local overrides
> > > > >
> > > > > Resolves:
> > > > > https://fedorahosted.org/sssd/ticket/2584
> > > >
> > > > There was one more Coverity warning:
> > > > Error: UNINIT (CWE-457): [#def1]
> > > > sssd-1.13.1/src/tools/sss_override.c:202: var_decl: Declaring variable
> > > > "ret" without initializer.
> > > > sssd-1.13.1/src/tools/sss_override.c:252: uninit_use: Using
> > > > uninitialized value "ret".
> > > > #  250|
> > > > #  251|   done:
> > > > #  252|->     if (ret != EOK) {
> > > > #  253|           talloc_free(attrs);
> > > > #  254|           return NULL;
> > > >
> > > > Otherwise looks good.
> > > > _______________________________________________
> > > > sssd-devel mailing list
> > > > [e-mail]
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > >
> > > Thanks, new patches are attached. In addition to your comments:
> > > one new patch that adds null check in original unit test
> > > man page and tool change that says that zero ids are not supported
> >
> > Few more bug fixed during irc review. Thanks Jakub!
>
> These patches work well for me, all attributes can be overriden,
> existing overrides in IPA case are detected and the crash is gone.
>
> Both CI and Coverity are happy as well.
>
> ACK

Pushed to master:
    * b69cb1787209e85cc246eb9a944242689bfe0c46
    * 284937e6b5b0c9d7a1d3382d0d2820d1168842fb
    * a8d31510d12af6ee39fb3e1e13f3a4f6bdef33c1
    * 166a622bcba319aaa7190fbc348af887cbba30bf

[Jakub Hrozek]

On Mon, Jul 27, 2015 at 12:00:25PM +0200, Pavel Březina wrote:
> > Hmm, why? Can we instead send a signal to SSSD to pick up the changes?
> > (If yes, then it can be a follow-up patch..)
>
> No, it is not possible to change view on the fly the way views are
> implemented.

I think this is the rule for the IPA provider, but I don't see why it
should be this way in responders if we can update the domain info. I'll
file a ticket.

[Sumit Bose]

On Mon, Jul 27, 2015 at 05:09:17PM +0200, Jakub Hrozek wrote:
> On Mon, Jul 27, 2015 at 12:00:25PM +0200, Pavel Březina wrote:
> > > Hmm, why? Can we instead send a signal to SSSD to pick up the changes?
> > > (If yes, then it can be a follow-up patch..)
> >
> > No, it is not possible to change view on the fly the way views are
> > implemented.
>
> I think this is the rule for the IPA provider, but I don't see why it
> should be this way in responders if we can update the domain info. I'll
> file a ticket.

I would say it is a general precaution. Switching the view on-the-fly
might change UIDs and GIDs mappings of running processes and logged-in
users on-the-fly. All kind of strange things might happen then, e.g.
calling 'whoami' might return a different user name, if in the new view
the UID of the running user belongs to a different user.

bye,
Sumit
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Březina]

On 07/27/2015 05:23 PM, Sumit Bose wrote:
> On Mon, Jul 27, 2015 at 05:09:17PM +0200, Jakub Hrozek wrote:
> > On Mon, Jul 27, 2015 at 12:00:25PM +0200, Pavel Březina wrote:
> > > > Hmm, why? Can we instead send a signal to SSSD to pick up the changes?
> > > > (If yes, then it can be a follow-up patch..)
> > >
> > > No, it is not possible to change view on the fly the way views are
> > > implemented.
> >
> > I think this is the rule for the IPA provider, but I don't see why it
> > should be this way in responders if we can update the domain info. I'll
> > file a ticket.
>
> I would say it is a general precaution. Switching the view on-the-fly
> might change UIDs and GIDs mappings of running processes and logged-in
> users on-the-fly. All kind of strange things might happen then, e.g.
> calling 'whoami' might return a different user name, if in the new view
> the UID of the running user belongs to a different user.

Hmm, that is not really a problem of viewname change, but of override 
added/modified/removed which is supported on the fly.

Are there any checks on IPA provider (or server) that prohibits 
overriding to a name/uid that already exist? The tool will allow.

[Jakub Hrozek]

On Mon, Jul 27, 2015 at 05:23:54PM +0200, Sumit Bose wrote:
> On Mon, Jul 27, 2015 at 05:09:17PM +0200, Jakub Hrozek wrote:
> > On Mon, Jul 27, 2015 at 12:00:25PM +0200, Pavel Březina wrote:
> > > > Hmm, why? Can we instead send a signal to SSSD to pick up the changes?
> > > > (If yes, then it can be a follow-up patch..)
> > >
> > > No, it is not possible to change view on the fly the way views are
> > > implemented.
> >
> > I think this is the rule for the IPA provider, but I don't see why it
> > should be this way in responders if we can update the domain info. I'll
> > file a ticket.
>
> I would say it is a general precaution. Switching the view on-the-fly
> might change UIDs and GIDs mappings of running processes and logged-in
> users on-the-fly. All kind of strange things might happen then, e.g.
> calling 'whoami' might return a different user name, if in the new view
> the UID of the running user belongs to a different user.
>
> bye,
> Sumit

Hmm, that makes sense, but isn't it a problem after the view is changed
for newly-added overrides also?