On Tue, Jun 21, 2016 at 04:44:05PM +0200, Jakub Hrozek wrote:
> 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.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
