URL: https://github.com/SSSD/sssd/pull/274 Title: #274: Merge sss_cache and sss_debuglevel into sssctl

centos-ci commented: """ Can one of the admins verify this patch? """

See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-301468954

URL: https://github.com/SSSD/sssd/pull/274 Title: #274: Merge sss_cache and sss_debuglevel into sssctl

Label: +Changes requested

URL: https://github.com/SSSD/sssd/pull/274 Title: #274: Merge sss_cache and sss_debuglevel into sssctl

jhrozek commented: """ I still see value in having sssctl manage the cache expiration because especially for sudo rules, we not only need to expire the rules in the cache, but also tell the deamon to fetch the rules again. There I was thinking of having an interface provided by the IFP responder that the tool could call into. And having a reliable way of expiring sudo rules would be handy, it's something I see mentioned in support cases a log.

@lslebodn will probably have a better idea than I how to solve the packaging problem but what about just moving the sss_cache wrapper to sssd-tools? Of course, upgrades will then be problematic, because if someone doesn't have sss-tools installed, then sss_cache would be 'lost' for this user on upgrade. But I'm not sure I have a better idea short of forcing sss-tools on everyone who had sssd-common previously.. """

See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-302773603

URL: https://github.com/SSSD/sssd/pull/274 Title: #274: Merge sss_cache and sss_debuglevel into sssctl

lslebodn commented: """ Extra message is helpful for users but it still will not fix tests or any other automated script.

In theory we could merge the package `sssd-tools` into `sssd-common`. But it pull additional dependencies: ``` sh# dnf install --assumeno /usr/sbin/sssctl Last metadata expiration check: 0:01:15 ago on Wed Jul 19 16:34:23 2017. Dependencies resolved. ================================================================================ Package Arch Version Repository Size ================================================================================ Installing: sssd-tools x86_64 1.15.2-5.fc26 fedora 357 k Installing dependencies: libsss_simpleifp x86_64 1.15.2-5.fc26 fedora 70 k python3-sss x86_64 1.15.2-5.fc26 fedora 90 k sssd-dbus x86_64 1.15.2-5.fc26 fedora 164 k

Transaction Summary ================================================================================ Install 4 Packages

Total download size: 681 k Installed size: 1.4 M Operation aborted. ```

And I would like to do opposite. Reduce `sssd-common` due to `sssd-kcm` and `sssd-secrets`.

ATM the simplest would be to drop removing `sss_cache`. But we can still have "an allias" `sssctl expire-cache` -> `sss_cache`. Because I like name `expire_cache` more then `sss_cache` :-) """

See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-316446652

URL: https://github.com/SSSD/sssd/pull/274 Title: #274: Merge sss_cache and sss_debuglevel into sssctl

lslebodn commented: """ On (20/07/17 10:22), mzidek-rh wrote:

In sssctl we follow (or at least try to) the TOPIC-ACTION convention for naming the commands. So "expire-cache" is not a good name. It should be 'cache-expire'. We already have 'cache' topic in the local data section and I think this command fits there to.

That's true but as I mentioned in previous comment. The simplest would be to drop removing `sss_cache`

Ideal would be to have hybrid way how to call subcommands. git has binaries in /usr/libexec/git-core/ or other paths.

In such way, we could have /usr/bin/sssctl-cache-expire which would have minimal dependencies and could be easily called from shell wrapper /usr/bin/sss_cache because it would be still part of package sssd-common even though package sssd-tools will not be installed.

But that's probably a little bit bigger scope for task which Justin can do as part of his job :-)

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-316667054

URL: https://github.com/SSSD/sssd/pull/274 Title: #274: Merge sss_cache and sss_debuglevel into sssctl

justin-stephenson commented: """ PR updated: - Dropped the sss_cache commits - Rebased to master - Added new commit adding `sssctl cache-expire` command as a simple wrapper for `sss_cache` """

See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-317855700

URL: https://github.com/SSSD/sssd/pull/274 Title: #274: Merge sss_cache and sss_debuglevel into sssctl

mzidek-rh commented: """ There is a tabulator instead of space after '=' sign in Makefile.am: ``` 458 459 dist_sbin_SCRIPTS = contrib/tools/sss_debuglevel 460

``` But as Lukas mentioned we should not put sss_debuglevel in the tarball directly, but only the .in version and call the replace_script on it in the Makefile.am, so that proper paths are expanded in the script (see how replace_script is used in the Makefile.am for other files). So the configure.ac line should be removed too.

Also I would not put this to the contrib directory because this is directory that we use for things that are not packaged in the tarball or nice things that someone creted but are not core parts of the project (like some helpful sripts etc). Maybe a new directory src/tools/wrappers would be more appropriate? @lslebodn do you have any proposal in this regard?

I can see these warnings when I run autoreconf -if indicating you forgot to remove some lines from Makefile.am: ``` Makefile.am: installing 'build/depcomp' Makefile.am:1685: warning: variable 'sss_debuglevel_SOURCES' is defined but no program or Makefile.am:1685: library has 'sss_debuglevel' as canonical name (possible typo) Makefile.am:1688: warning: variable 'sss_debuglevel_LDADD' is defined but no program or Makefile.am:1688: library has 'sss_debuglevel' as canonical name (possible typo)

```

Bad indentation in src/tools/sssctl/sssctl.h ``` 113 errno_t sssctl_debug_level(struct sss_cmdline *cmdline, 114 struct sss_tool_ctx *tool_ctx, 115 void *pvt); ^ bad indentation ``` In src/tools/sssctl/sssctl_logs.c, the config.h should be the first header that is included (I think we do not follow this everywhere, but still..)

Another bad indentation in src/tools/sssctl/sssctl.h ``` 101 errno_t sssctl_cache_expire(struct sss_cmdline *cmdline, 102 struct sss_tool_ctx *tool_ctx, 103 void *pvt); 104 ^^^ bad indentation

```

"""

See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-318339105

URL: https://github.com/SSSD/sssd/pull/274 Author: justin-stephenson Title: #274: Merge sss_cache and sss_debuglevel into sssctl Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/274/head:pr274 git checkout pr274

URL: https://github.com/SSSD/sssd/pull/274 Title: #274: Merge sss_cache and sss_debuglevel into sssctl

jhrozek commented: """ I'm sorry this PR review stalled (again :-/) but I've read the patches and I like them now. I have two nitpicks, one was found by Coverity: ```` Error: CHECKED_RETURN (CWE-252): sssd-1.15.4/src/tools/sssctl/sssctl_data.c:307: check_return: Calling "sssctl_run_command" without checking return value (as is done elsewhere 7 out of 8 times). sssd-1.15.4/src/tools/sssctl/sssctl_data.c:108: example_assign: Example 1: Assigning: "ret" = return value from "sssctl_run_command("sss_override user-export /var/lib/sss/backup/sssd_user_overrides.bak")". sssd-1.15.4/src/tools/sssctl/sssctl_data.c:110: example_checked: Example 1 (cont.): "ret" has its value checked in "ret != 0". sssd-1.15.4/src/tools/sssctl/sssctl_data.c:332: example_assign: Example 2: Assigning: "ret" = return value from "sssctl_run_command(cmd)". sssd-1.15.4/src/tools/sssctl/sssctl_data.c:333: example_checked: Example 2 (cont.): "ret" has its value checked in "ret != 0". sssd-1.15.4/src/tools/sssctl/sssctl_data.c:161: example_assign: Example 3: Assigning: "ret" = return value from "sssctl_run_command("sss_override user-import /var/lib/sss/backup/sssd_user_overrides.bak")". sssd-1.15.4/src/tools/sssctl/sssctl_data.c:163: example_checked: Example 3 (cont.): "ret" has its value checked in "ret != 0". sssd-1.15.4/src/tools/sssctl/sssctl_data.c:170: example_assign: Example 4: Assigning: "ret" = return value from "sssctl_run_command("sss_override group-import /var/lib/sss/backup/sssd_group_overrides.bak")". sssd-1.15.4/src/tools/sssctl/sssctl_data.c:172: example_checked: Example 4 (cont.): "ret" has its value checked in "ret != 0". sssd-1.15.4/src/tools/sssctl/sssctl_logs.c:292: example_assign: Example 5: Assigning: "ret" = return value from "sssctl_run_command(cmd)". sssd-1.15.4/src/tools/sssctl/sssctl_logs.c:293: example_checked: Example 5 (cont.): "ret" has its value checked in "ret != 0". # 305| fprintf(stderr, _("Missing option. \n")); # 306| cachecmd = "sss_cache --help"; # 307|-> sssctl_run_command(cachecmd); # 308| ret = EXIT_FAILURE; # 309| goto done; ````

And the other I'll point out inline. """

See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-329431797

URL: https://github.com/SSSD/sssd/pull/274 Title: #274: Merge sss_cache and sss_debuglevel into sssctl

jhrozek commented: """ In addition, distcheck failed: ``` ERROR: files left in build directory after distclean: ./src/tools/wrappers/sss_debuglevel make[1]: *** [distcleancheck] Error 1 ``` """

See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-329444877

URL: https://github.com/SSSD/sssd/pull/274 Title: #274: Merge sss_cache and sss_debuglevel into sssctl

justin-stephenson commented: """ Thank you for the review @jhrozek

Patchset updated. """

See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-330415031

URL: https://github.com/SSSD/sssd/pull/274 Title: #274: Merge sss_cache and sss_debuglevel into sssctl

Label: +Accepted

URL: https://github.com/SSSD/sssd/pull/274 Title: #274: Merge sss_cache and sss_debuglevel into sssctl

Label: -Accepted

URL: https://github.com/SSSD/sssd/pull/274 Author: justin-stephenson Title: #274: Merge sss_cache and sss_debuglevel into sssctl Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/274/head:pr274 git checkout pr274

URL: https://github.com/SSSD/sssd/pull/274 Title: #274: Merge sss_cache and sss_debuglevel into sssctl

Label: +Changes requested

URL: https://github.com/SSSD/sssd/pull/274 Title: #274: Merge sss_cache and sss_debuglevel into sssctl

justin-stephenson commented: """ @lslebodn comments addressed, I was primarily following similar sssctl functions as an example. """

See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-331213596

URL: https://github.com/SSSD/sssd/pull/274 Title: #274: Merge sss_cache and sss_debuglevel into sssctl

lslebodn commented: """ @justin-stephenson, I did few changes to build each commit separately (make git bisect happy :-) Are you fine with them? https://pagure.io/fork/lslebodn/SSSD/sssd/commits/pr274_changes """

See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-331904859

URL: https://github.com/SSSD/sssd/pull/274 Author: justin-stephenson Title: #274: Merge sss_cache and sss_debuglevel into sssctl Action: closed

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/274/head:pr274 git checkout pr274

URL: https://github.com/SSSD/sssd/pull/274 Title: #274: Merge sss_cache and sss_debuglevel into sssctl

Label: +Pushed