On Tue, Jun 21, 2016 at 04:15:06PM +0200, Pavel Březina wrote:
> On 06/21/2016 03:50 PM, Jakub Hrozek wrote:
> > On Mon, Jun 20, 2016 at 07:13:10PM +0200, Jakub Hrozek wrote:
> > > Hi,
> > >
> > > Pavel's sssctl tool is at:
> > >
https://fedorapeople.org/cgit/pbrezina/public_git/sssd.git/log/?h=sssctl
> > >
> > > I'll try to have a first pass at review today in the evening, but I
> > > thought I'll pass on the branch in case anyone else is interested..
> >
> > Hi, I went through the patches today. I pushed some of my review
> > comments to my branch:
> >
https://github.com/jhrozek/sssd/tree/sssctl
> > these are mostly trivial. The only important part is that sss_simpleifp
> > changes are no longer backwards-incompatible to make enterprise distros
> > happy.
> >
> > Other comments:
> > 1) The sssctl tool restarts sssd with a command and only supports
> > systemctl and service. For Beta, I would propose:
> > - if sssd is built with systemd, use systemctl
> > - otherwise, detect /sbin/service during configure time. If it's
> > available, use /sbin/service
> > - otherwise, don't support restarting sssd at all, but tell the
> > admin to restart it
>
> Agree. I was thinking whether to put it in or not but then again it's to
> simplify life of our users but I didn't want to overthink it. All those
> operations can be done manually without sssctl.
>
> >
> > For the next release, I would prefer to call systemd D-Bus
> > interface to restart the service instead. This is cross-platform
> > enough. Let other distributions submit patches for restarting the
> > service via another service manager. This would be handled with a
> > 1.14.0 ticket.
>
> Do you mean this interface?
>
https://www.freedesktop.org/wiki/Software/systemd/dbus/
Yes.
>
> >
> > 2) We should open a ticket to merge sss_cache, sss_debuglevel and
> > maybe others under sssctl. sss_cache would just be a shell wrapper
> > that calls sssctl for some time and eventually removed (in 2.0?)
>
> Agree.
>
> >
> > 3) the commands that remove the cache should remove the timestamps
> > cache as well, if it exists (depends on which patches get to master
> > first)
> >
> > 4) removing logs -- is there a reason to not remove *.log but
> > special case child.log as well?
>
> The same reason why I avoided to remove *.ldb and pinpointed the files
> instead. So we do not remove anything that doesn't belong to SSSD even if it
> is under our directory.
It is /our/ directory :-) I think a wildcard would be better, otherwise,
I'm sure we will forget to add a pattern when we add some new logfile.
>
> >
> > 5) CI fails. I haven't ran Coverity yet either.
> >
>
Hi,
I'm continuing the review based on today's branch contents:
* IFP: Add domain nodes - ACK
* IFP: new header file that contains interface definitions - ACK
* sss_sifp: make it compatible with latest version of the infopipe - ACK
* sss_sifp: return context even on IO error - ACK
* sss_sifp: bump version to 1:0:0 - ACK to the contents, but the
commit message still says 1:0 :-)
* sss_tools: add command description - ACK
* sss_tools: add help commands to usage message - ACK
* sss_tools: unify description of --debug - ACK
* sss_tools: tell whether an option was provided - ACK
* sss_tools: add commands delimiter - ACK
* sss_tools: pad help message properly - ACK
* sss_tools: return errno_t instead of system code - ACK
* sss_tools: return errno_t instead of system code - ACK
* sss_tools: add test if sssd is running - ACK
* sss_tools: create confdb if not exist - ACK, but we might want to
change this patch once we push the patches to merge configurations
from include directories
* sss_override: return EXIT_SUCCESS even when no overrides are found - ACK
* sss_override: return EXIT_FAILURE if file does not exist during import - ACK
* ERRORS: Add errors to indicated whether SSSD is running or not - ACK
* SBUS ERRORS: Add unknown domain - ACK
* DP: Add function to get be_ctx directly from dp_client - ACK, but
but see below for additional check
* DP: Add org.freedesktop.sssd.DataProvider.Backend
- in dp_backend_is_online() (and elsewhere) does it make sense
to be paranoid and check if be_ctx is non-NULL
- in dp_backend_is_online(), did you mean to check domname for
NULL instead of '\0' ?
* DP: Add org.freedesktop.sssd.DataProvider.Failover - ACK
* IFP: Provide domain and failover status - please make
ifp_domains_domain_is_online_done() static. Same for
ifp_domains_domain_list_services_done(). The code itself looks
good to me.
* sssctl: new tool - we still need to detect /sbin/service at
configure time. Moreover, I would expect fetch-logs withouth
arguments to tar up all the logs.
Also, please let me know if you change anything in the already acked
patches.