On Thu, Jun 23, 2016 at 01:40:50PM +0200, Lukas Slebodnik wrote:

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;

Ugh, thanks, that's because the first version just broke the API, I didn't the fix the version exports properly after fixing the API..

On 06/23/2016 08:14 PM, Jakub Hrozek wrote:

On Thu, Jun 23, 2016 at 01:51:53PM +0200, Jakub Hrozek wrote:

...

...

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;

Ugh, thanks, that's because the first version just broke the API, I didn't the fix the version exports properly after fixing the API..

Attached are patches I intend to push once CI and Coverity finish.

When I have badly configured SSSD (for example domains = nonexistent.com), I can not run sssctl command at all. It complains that there are no properly confiured domains even if all I wanted is

sssctl -?

to print help. Should I open a ticket to fix this later? We may have more functionality that does not necessarily need properly confiured SSSD in the future.

Michal

On (24/06/16 13:07), Pavel Březina wrote:

On 06/23/2016 08:14 PM, Jakub Hrozek wrote:

...

On Thu, Jun 23, 2016 at 01:51:53PM +0200, Jakub Hrozek wrote:

...

...

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;

^ shouldn't this line say 0.1?

No.

SSS_SIMPLEIFP_0.1 { ^^^^^^^^^^^^^^^^^ here is a name of new version then there are newly added functions } SSS_SIMPLEIFP_0.0; ^^^^^^^^^^^^^^^^^ and here is version which is extended. Usually, previous version.

LS

On 06/24/2016 01:33 PM, Pavel Březina wrote:

On 06/23/2016 08:14 PM, Jakub Hrozek wrote:

...

On Thu, Jun 23, 2016 at 01:51:53PM +0200, Jakub Hrozek wrote:

...

...

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;

Ugh, thanks, that's because the first version just broke the API, I didn't the fix the version exports properly after fixing the API..

Attached are patches I intend to push once CI and Coverity finish.

Thank you very much for your help. I did following changes:

Rebased sss_tools: create confdb if not exist. For some reason it did not apply and git couldn't perform three way merge. I do not know what the conflict was so I reconstructed the patch manually.

In src/providers/data_provider/dp_iface_backend.c:

• if (domname == NULL) {

• if (domname == '\0') { domain = be_ctx->domain; } else {

It is not possible to send NULL string over D-Bus so we need to check that the string is empty.

In src/tools/sssctl/sssctl.c: -#ifdef HAVE_SYSTEMD_LOGIN +#ifdef HAVE_SYSTEMD #define SSSD_SVC_CMD(cmd) "systemctl " cmd " sssd.service" #else #define SSSD_SVC_CMD(cmd) "service sssd " cmd

And renamed: sssctl_svc_mgmg -> sssctl_manage_service (I couldn't understand what mgmg stands for)

enum sss_svc_acct -> enum sssctl_svc_action (acct seems wrong to me since it means account and I wanted to keep tool's prefix)

Turned out I sent wrong patches.