On (23/06/16 13:23), Jakub Hrozek wrote:
>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
diff --git a/src/lib/sifp/sss_simpleifp.exports
b/src/lib/sifp/sss_simpleifp.exports
index 9fa6ecd..f491092 100644
--- a/src/lib/sifp/sss_simpleifp.exports
+++ b/src/lib/sifp/sss_simpleifp.exports
@@ -7,7 +7,6 @@ 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;
@@ -15,9 +14,7 @@ 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;
@@ -49,3 +46,11 @@ SSS_SIMPLEIFP_0.0 {
local:
*;
};
+
+SSS_SIMPLEIFP_0.1 {
+ # public functions
+ global:
+ sss_sifp_strerr;
+ sss_sifp_invoke_list_ex;
+ sss_sifp_invoke_find_ex;
+} SSS_SIMPLEIFP_0.0;
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
