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.
Change to src/lib/sifp/sss_simpleifp.exports is wrong.
Current version:
We cannot add new function to already released version
They shoudl be added to new version.
@see attached diff
diff --git a/src/lib/sifp/sss_simpleifp.exports b/src/lib/sifp/sss_simpleifp.exports
index 3d598fb..9fa6ecd 100644
--- a/src/lib/sifp/sss_simpleifp.exports
+++ b/src/lib/sifp/sss_simpleifp.exports
@@ -7,6 +7,7 @@ SSS_SIMPLEIFP_0.0 {
sss_sifp_init_ex;
sss_sifp_get_last_io_error_name;
sss_sifp_get_last_io_error_message;
+ sss_sifp_strerr;
sss_sifp_create_message;
sss_sifp_send_message;
sss_sifp_send_message_ex;
@@ -14,7 +15,9 @@ SSS_SIMPLEIFP_0.0 {
sss_sifp_fetch_all_attrs;
sss_sifp_fetch_object;
sss_sifp_invoke_list;
+ sss_sifp_invoke_list_ex;
sss_sifp_invoke_find;
+ sss_sifp_invoke_find_ex;
sss_sifp_find_attr_as_bool;
sss_sifp_find_attr_as_int16;
sss_sifp_find_attr_as_uint16;
I didn't check rest of patches and I do not plant to do it :-)
LS