On Wed, Jun 22, 2016 at 03:23:28PM +0200, Jakub Hrozek wrote:
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,
Because Pavel is not available these days, I took the liberty of fixing
the minor issues in the patches myself.
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
Let's push these patches first.
* 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
Since we don't check for be_ctx validity elsewhere either, I didn't add
the 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' ?
I fixed the NULL check here.
* 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.
I fixed the internal functions to be static.
* 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.
I added a new patch that checks for /sbin/service at configure time and
returns ENOSYS on platforms that don't have either of /sbin/service or
systemctl. On these platforms, the tool will just ask the admin to
restart sssd on his own.
There is also a patch that silences some warnings from Coverity.
Additionally, I added the -I option for the include files from simpleifp
so that CI passes:
http://sssd-ci.duckdns.org/logs/job/45/82/summary.html
Lastly, I opened some follow-up tickets for issues that we need to fix
post-Beta. Some need to be fixed before we release 1.14.0:
-
https://fedorahosted.org/sssd/ticket/3055 - The sssctl tool is missing
a manpage
-
https://fedorahosted.org/sssd/ticket/3056 - The sssctl tool should restart
the service with systemd's
dbus API
-
https://fedorahosted.org/sssd/ticket/3058 - The sssctl tool is missing
integration tests
-
https://fedorahosted.org/sssd/ticket/3059 - The sssctl tool doesn't work with
users from subdomains
And this one can be considered as future work:
-
https://fedorahosted.org/sssd/ticket/3057 - Merge existing command line
tools into sssctl
If nobody complains about my fixups, I would like to push these patches
and release a Beta.