[PATCHES] backend: initialize sudo only when it is enabled in services

[Pavel Březina]

0001 fixes https://fedorahosted.org/sssd/ticket/1458
0002 fixes a memory leak when be_process_init() fails. I think it should 
be fixed, even though the backend is in this case immediately terminated

[Sumit Bose]

On Wed, Sep 05, 2012 at 03:05:49PM +0200, Pavel Březina wrote:
> 0001 fixes https://fedorahosted.org/sssd/ticket/1458
> 0002 fixes a memory leak when be_process_init() fails. I think it
> should be fixed, even though the backend is in this case immediately
> terminated

Hi,

the patches compile without errors and work as expected. Nevertheless I
have a few comments:

0001:
 - I think it make sense to skip the initialization of the sudo target
   if there is no responder configured. But to avoid surprises on the
   user side I would recommend to do the initialization if it is
   configured explicitly in sssd.conf, i.e sudo_provider = ldap, and
   give a warning in the logs that the corresponding responder will not
   be running.
> ret = confdb_get_string(ctx->cdb, ctx, CONFDB_MONITOR_CONF_ENTRY,
>                        CONFDB_MONITOR_ACTIVE_SERVICES, NULL, &services);
> if (ret != EOK) {
>    DEBUG(SSSDBG_FATAL_FAILURE,
>          ("Unable to read from confdb [%d]: %s\n", ret, strerror(ret)));
>    return ret;
> }

- I would prefer to use confdb_get_string_as_list() and use strcmp()
   later on, to be on the safe side.

0002: ACK

bye,
Sumit

[Pavel Březina]

On 09/07/2012 11:31 AM, Sumit Bose wrote:
> On Wed, Sep 05, 2012 at 03:05:49PM +0200, Pavel Březina wrote:
> > 0001 fixes https://fedorahosted.org/sssd/ticket/1458
> > 0002 fixes a memory leak when be_process_init() fails. I think it
> > should be fixed, even though the backend is in this case immediately
> > terminated
>
> Hi,
>
> the patches compile without errors and work as expected. Nevertheless I
> have a few comments:
>
> 0001:
> I think it make sense to skip the initialization of the sudo target
> if there is no responder configured. But to avoid surprises on the
> user side I would recommend to do the initialization if it is
> configured explicitly in sssd.conf, i.e sudo_provider = ldap, and
> give a warning in the logs that the corresponding responder will not
> be running.

I discussed this with Jakub before writing the patch. sudo_provider
currently defaults to id_provider. If I get it right, you are
suggesting to make the default value "none".

We agreed with Jakub that this makes more pressure on the user to
understand the configuration so we chose not to do it. Although the
configurations I've been seeing so far are mostly generated by
authconfig or ipa-client-install (where are all providers set
explicitly) so I don't think it is a problem after all. What do you say
Jakub?

If we decide to do it this way, it will also require changes in man page.
> > ret = confdb_get_string(ctx->cdb, ctx, CONFDB_MONITOR_CONF_ENTRY,
> >                        CONFDB_MONITOR_ACTIVE_SERVICES, NULL, &services);
> > if (ret != EOK) {
> >    DEBUG(SSSDBG_FATAL_FAILURE,
> >          ("Unable to read from confdb [%d]: %s\n", ret, strerror(ret)));
> >    return ret;
> > }
> I would prefer to use confdb_get_string_as_list() and use strcmp()
> later on, to be on the safe side.
>
> 0002: ACK
>
> bye,
> Sumit
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Mon, Sep 10, 2012 at 09:35:25AM +0200, Pavel Březina wrote:
> On 09/07/2012 11:31 AM, Sumit Bose wrote:
> > On Wed, Sep 05, 2012 at 03:05:49PM +0200, Pavel Březina wrote:
> > > 0001 fixes https://fedorahosted.org/sssd/ticket/1458
> > > 0002 fixes a memory leak when be_process_init() fails. I think it
> > > should be fixed, even though the backend is in this case immediately
> > > terminated
> >
> > Hi,
> >
> > the patches compile without errors and work as expected. Nevertheless I
> > have a few comments:
> >
> > 0001:
> > I think it make sense to skip the initialization of the sudo target
> > if there is no responder configured. But to avoid surprises on the
> > user side I would recommend to do the initialization if it is
> > configured explicitly in sssd.conf, i.e sudo_provider = ldap, and
> > give a warning in the logs that the corresponding responder will not
> > be running.
>
> I discussed this with Jakub before writing the patch. sudo_provider
> currently defaults to id_provider. If I get it right, you are
> suggesting to make the default value "none".
>
> We agreed with Jakub that this makes more pressure on the user to
> understand the configuration so we chose not to do it. Although the
> configurations I've been seeing so far are mostly generated by
> authconfig or ipa-client-install (where are all providers set
> explicitly) so I don't think it is a problem after all. What do you say
> Jakub?

I would personally prefer that the value is inherited from id_provider
to keep the configuration simple..

One place I can think of where this inheritance might get confusing is
multidomain environment where enabling the [sudo] service would
automatically enable the sudo provider in all domains.

[Sumit Bose]

On Mon, Sep 10, 2012 at 09:35:25AM +0200, Pavel Březina wrote:
> On 09/07/2012 11:31 AM, Sumit Bose wrote:
> > On Wed, Sep 05, 2012 at 03:05:49PM +0200, Pavel Březina wrote:
> > > 0001 fixes https://fedorahosted.org/sssd/ticket/1458
> > > 0002 fixes a memory leak when be_process_init() fails. I think it
> > > should be fixed, even though the backend is in this case immediately
> > > terminated
> >
> > Hi,
> >
> > the patches compile without errors and work as expected. Nevertheless I
> > have a few comments:
> >
> > 0001:
> > I think it make sense to skip the initialization of the sudo target
> > if there is no responder configured. But to avoid surprises on the
> > user side I would recommend to do the initialization if it is
> > configured explicitly in sssd.conf, i.e sudo_provider = ldap, and
> > give a warning in the logs that the corresponding responder will not
> > be running.
>
> I discussed this with Jakub before writing the patch. sudo_provider
> currently defaults to id_provider. If I get it right, you are
> suggesting to make the default value "none".

No, sorry for not being clear here. I think the id_provider default is
right. I just wanted to say that if for whatever reasons the
sudo_provider parameter is used explicitly in sssd.conf the
sudo_provider should be initialization even if sudo is not listed in
services.

bye,
Sumit
> We agreed with Jakub that this makes more pressure on the user to
> understand the configuration so we chose not to do it. Although the
> configurations I've been seeing so far are mostly generated by
> authconfig or ipa-client-install (where are all providers set
> explicitly) so I don't think it is a problem after all. What do you say
> Jakub?
>
> If we decide to do it this way, it will also require changes in man page.
> > > ret = confdb_get_string(ctx->cdb, ctx, CONFDB_MONITOR_CONF_ENTRY,
> > >                        CONFDB_MONITOR_ACTIVE_SERVICES, NULL, &services);
> > > if (ret != EOK) {
> > >    DEBUG(SSSDBG_FATAL_FAILURE,
> > >          ("Unable to read from confdb [%d]: %s\n", ret, strerror(ret)));
> > >    return ret;
> > > }
> > I would prefer to use confdb_get_string_as_list() and use strcmp()
> > later on, to be on the safe side.
> >
> > 0002: ACK
> >
> > bye,
> > Sumit
> > _______________________________________________
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Březina]

On 09/10/2012 10:48 AM, Sumit Bose wrote:
> On Mon, Sep 10, 2012 at 09:35:25AM +0200, Pavel Březina wrote:
> > On 09/07/2012 11:31 AM, Sumit Bose wrote:
> > > On Wed, Sep 05, 2012 at 03:05:49PM +0200, Pavel Březina wrote:
> > > > 0001 fixes https://fedorahosted.org/sssd/ticket/1458
> > > > 0002 fixes a memory leak when be_process_init() fails. I think it
> > > > should be fixed, even though the backend is in this case immediately
> > > > terminated
> > >
> > > Hi,
> > >
> > > the patches compile without errors and work as expected. Nevertheless I
> > > have a few comments:
> > >
> > > 0001:
> > > I think it make sense to skip the initialization of the sudo target
> > > if there is no responder configured. But to avoid surprises on the
> > > user side I would recommend to do the initialization if it is
> > > configured explicitly in sssd.conf, i.e sudo_provider = ldap, and
> > > give a warning in the logs that the corresponding responder will not
> > > be running.
> >
> > I discussed this with Jakub before writing the patch. sudo_provider
> > currently defaults to id_provider. If I get it right, you are
> > suggesting to make the default value "none".
>
> No, sorry for not being clear here. I think the id_provider default is
> right. I just wanted to say that if for whatever reasons the
> sudo_provider parameter is used explicitly in sssd.conf the
> sudo_provider should be initialization even if sudo is not listed in
> services.
>
> bye,
> Sumit
> > We agreed with Jakub that this makes more pressure on the user to
> > understand the configuration so we chose not to do it. Although the
> > configurations I've been seeing so far are mostly generated by
> > authconfig or ipa-client-install (where are all providers set
> > explicitly) so I don't think it is a problem after all. What do you say
> > Jakub?
> >
> > If we decide to do it this way, it will also require changes in man page.
> > > > ret = confdb_get_string(ctx->cdb, ctx, CONFDB_MONITOR_CONF_ENTRY,
> > > >                        CONFDB_MONITOR_ACTIVE_SERVICES, NULL, &services);
> > > > if (ret != EOK) {
> > > >    DEBUG(SSSDBG_FATAL_FAILURE,
> > > >          ("Unable to read from confdb [%d]: %s\n", ret, strerror(ret)));
> > > >    return ret;
> > > > }
> > > I would prefer to use confdb_get_string_as_list() and use strcmp()
> > > later on, to be on the safe side.
> > >
> > > 0002: ACK
> > >
> > > bye,
> > > Sumit

OK, that makes sense. Patches are attached.

I moved the code into separate function.

[Sumit Bose]

On Mon, Sep 10, 2012 at 01:52:53PM +0200, Pavel Březina wrote:
> On 09/10/2012 10:48 AM, Sumit Bose wrote:
> > On Mon, Sep 10, 2012 at 09:35:25AM +0200, Pavel Březina wrote:
> > > On 09/07/2012 11:31 AM, Sumit Bose wrote:
> > > > On Wed, Sep 05, 2012 at 03:05:49PM +0200, Pavel Březina wrote:
> > > > > 0001 fixes https://fedorahosted.org/sssd/ticket/1458
> > > > > 0002 fixes a memory leak when be_process_init() fails. I think it
> > > > > should be fixed, even though the backend is in this case immediately
> > > > > terminated
> > > >
> > > > Hi,
> > > >
> > > > the patches compile without errors and work as expected. Nevertheless I
> > > > have a few comments:
> > > >
> > > > 0001:
> > > > I think it make sense to skip the initialization of the sudo target
> > > > if there is no responder configured. But to avoid surprises on the
> > > > user side I would recommend to do the initialization if it is
> > > > configured explicitly in sssd.conf, i.e sudo_provider = ldap, and
> > > > give a warning in the logs that the corresponding responder will not
> > > > be running.
> > >
> > > I discussed this with Jakub before writing the patch. sudo_provider
> > > currently defaults to id_provider. If I get it right, you are
> > > suggesting to make the default value "none".
> >
> > No, sorry for not being clear here. I think the id_provider default is
> > right. I just wanted to say that if for whatever reasons the
> > sudo_provider parameter is used explicitly in sssd.conf the
> > sudo_provider should be initialization even if sudo is not listed in
> > services.
> >
> > bye,
> > Sumit
> > > We agreed with Jakub that this makes more pressure on the user to
> > > understand the configuration so we chose not to do it. Although the
> > > configurations I've been seeing so far are mostly generated by
> > > authconfig or ipa-client-install (where are all providers set
> > > explicitly) so I don't think it is a problem after all. What do you say
> > > Jakub?
> > >
> > > If we decide to do it this way, it will also require changes in man page.
> > > > > ret = confdb_get_string(ctx->cdb, ctx, CONFDB_MONITOR_CONF_ENTRY,
> > > > >                        CONFDB_MONITOR_ACTIVE_SERVICES, NULL, &services);
> > > > > if (ret != EOK) {
> > > > >    DEBUG(SSSDBG_FATAL_FAILURE,
> > > > >          ("Unable to read from confdb [%d]: %s\n", ret, strerror(ret)));
> > > > >    return ret;
> > > > > }
> > > > I would prefer to use confdb_get_string_as_list() and use strcmp()
> > > > later on, to be on the safe side.
> > > >
> > > > 0002: ACK
> > > >
> > > > bye,
> > > > Sumit
>
> OK, that makes sense. Patches are attached.
>
> I moved the code into separate function.

Thank you for the new version. I only have a few comments left:
 - provider_enabled is not used
 - in the (!responder_enabled && provider != NULL && strcmp(provider, NO_PROVIDER)
   case, please just print the debug message and call load_backend_module()
   afterwards. So that the provider is loaded as requested in the config
   file. This would also avoid the strange debug message
(Tue Sep 11 18:46:38 2012) [sssd[be[ipa17.devel]]] [be_process_init_sudo] (0x0080): SUDO provider is set, but it is not listed in active services. SUDO support will not work!
(Tue Sep 11 18:46:38 2012) [sssd[be[ipa17.devel]]] [be_process_init] (0x2000): SUDO backend target successfully loaded from provider [(null)].

bye,
Sumit

[Pavel Březina]

On 09/11/2012 06:54 PM, Sumit Bose wrote:
> On Mon, Sep 10, 2012 at 01:52:53PM +0200, Pavel Březina wrote:
> > On 09/10/2012 10:48 AM, Sumit Bose wrote:
> > > On Mon, Sep 10, 2012 at 09:35:25AM +0200, Pavel Březina wrote:
> > > > On 09/07/2012 11:31 AM, Sumit Bose wrote:
> > > > > On Wed, Sep 05, 2012 at 03:05:49PM +0200, Pavel Březina wrote:
> > > > > > 0001 fixes https://fedorahosted.org/sssd/ticket/1458
> > > > > > 0002 fixes a memory leak when be_process_init() fails. I think it
> > > > > > should be fixed, even though the backend is in this case immediately
> > > > > > terminated
> > > > >
> > > > > Hi,
> > > > >
> > > > > the patches compile without errors and work as expected. Nevertheless I
> > > > > have a few comments:
> > > > >
> > > > > 0001:
> > > > > I think it make sense to skip the initialization of the sudo target
> > > > > if there is no responder configured. But to avoid surprises on the
> > > > > user side I would recommend to do the initialization if it is
> > > > > configured explicitly in sssd.conf, i.e sudo_provider = ldap, and
> > > > > give a warning in the logs that the corresponding responder will not
> > > > > be running.
> > > >
> > > > I discussed this with Jakub before writing the patch. sudo_provider
> > > > currently defaults to id_provider. If I get it right, you are
> > > > suggesting to make the default value "none".
> > >
> > > No, sorry for not being clear here. I think the id_provider default is
> > > right. I just wanted to say that if for whatever reasons the
> > > sudo_provider parameter is used explicitly in sssd.conf the
> > > sudo_provider should be initialization even if sudo is not listed in
> > > services.
> > >
> > > bye,
> > > Sumit
> > > > We agreed with Jakub that this makes more pressure on the user to
> > > > understand the configuration so we chose not to do it. Although the
> > > > configurations I've been seeing so far are mostly generated by
> > > > authconfig or ipa-client-install (where are all providers set
> > > > explicitly) so I don't think it is a problem after all. What do you say
> > > > Jakub?
> > > >
> > > > If we decide to do it this way, it will also require changes in man page.
> > > > > > ret = confdb_get_string(ctx->cdb, ctx, CONFDB_MONITOR_CONF_ENTRY,
> > > > > >                        CONFDB_MONITOR_ACTIVE_SERVICES, NULL, &services);
> > > > > > if (ret != EOK) {
> > > > > >    DEBUG(SSSDBG_FATAL_FAILURE,
> > > > > >          ("Unable to read from confdb [%d]: %s\n", ret, strerror(ret)));
> > > > > >    return ret;
> > > > > > }
> > > > > I would prefer to use confdb_get_string_as_list() and use strcmp()
> > > > > later on, to be on the safe side.
> > > > >
> > > > > 0002: ACK
> > > > >
> > > > > bye,
> > > > > Sumit
> >
> > OK, that makes sense. Patches are attached.
> >
> > I moved the code into separate function.
>
> Thank you for the new version. I only have a few comments left:
> provider_enabled is not used
> in the (!responder_enabled && provider != NULL && strcmp(provider, NO_PROVIDER)
> case, please just print the debug message and call load_backend_module()
> afterwards. So that the provider is loaded as requested in the config
> file. This would also avoid the strange debug message
>
> (Tue Sep 11 18:46:38 2012) [sssd[be[ipa17.devel]]] [be_process_init_sudo] (0x0080): SUDO provider is set, but it is not listed in active services. SUDO support will not work!
> (Tue Sep 11 18:46:38 2012) [sssd[be[ipa17.devel]]] [be_process_init] (0x2000): SUDO backend target successfully loaded from provider [(null)].

Thank you. I had the changes prepared, but I forgot to amend the commit.
The patch is attached.

[Sumit Bose]

On Wed, Sep 12, 2012 at 10:19:57AM +0200, Pavel Březina wrote:
> On 09/11/2012 06:54 PM, Sumit Bose wrote:
> > On Mon, Sep 10, 2012 at 01:52:53PM +0200, Pavel Březina wrote:
> > > On 09/10/2012 10:48 AM, Sumit Bose wrote:
> > > > On Mon, Sep 10, 2012 at 09:35:25AM +0200, Pavel Březina wrote:
> > > > > On 09/07/2012 11:31 AM, Sumit Bose wrote:
> > > > > > On Wed, Sep 05, 2012 at 03:05:49PM +0200, Pavel Březina wrote:
> > > > > > >0001 fixes https://fedorahosted.org/sssd/ticket/1458
> > > > > > >0002 fixes a memory leak when be_process_init() fails. I think it
> > > > > > >should be fixed, even though the backend is in this case immediately
> > > > > > >terminated
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > the patches compile without errors and work as expected. Nevertheless I
> > > > > > have a few comments:
> > > > > >
> > > > > > 0001:
> > > > > > I think it make sense to skip the initialization of the sudo target
> > > > > > if there is no responder configured. But to avoid surprises on the
> > > > > > user side I would recommend to do the initialization if it is
> > > > > > configured explicitly in sssd.conf, i.e sudo_provider = ldap, and
> > > > > > give a warning in the logs that the corresponding responder will not
> > > > > > be running.
> > > > >
> > > > > I discussed this with Jakub before writing the patch. sudo_provider
> > > > > currently defaults to id_provider. If I get it right, you are
> > > > > suggesting to make the default value "none".
> > > >
> > > > No, sorry for not being clear here. I think the id_provider default is
> > > > right. I just wanted to say that if for whatever reasons the
> > > > sudo_provider parameter is used explicitly in sssd.conf the
> > > > sudo_provider should be initialization even if sudo is not listed in
> > > > services.
> > > >
> > > > bye,
> > > > Sumit
> > > > > We agreed with Jakub that this makes more pressure on the user to
> > > > > understand the configuration so we chose not to do it. Although the
> > > > > configurations I've been seeing so far are mostly generated by
> > > > > authconfig or ipa-client-install (where are all providers set
> > > > > explicitly) so I don't think it is a problem after all. What do you say
> > > > > Jakub?
> > > > >
> > > > > If we decide to do it this way, it will also require changes in man page.
> > > > > > >+    ret = confdb_get_string(ctx->cdb, ctx, CONFDB_MONITOR_CONF_ENTRY,
> > > > > > >+                            CONFDB_MONITOR_ACTIVE_SERVICES, NULL, &services);
> > > > > > >+    if (ret != EOK) {
> > > > > > >+        DEBUG(SSSDBG_FATAL_FAILURE,
> > > > > > >+              ("Unable to read from confdb [%d]: %s\n", ret, strerror(ret)));
> > > > > > >+        return ret;
> > > > > > >+    }
> > > > > > >+
> > > > > > I would prefer to use confdb_get_string_as_list() and use strcmp()
> > > > > > later on, to be on the safe side.
> > > > > >
> > > > > > 0002: ACK
> > > > > >
> > > > > > bye,
> > > > > > Sumit
> > >
> > > OK, that makes sense. Patches are attached.
> > >
> > > I moved the code into separate function.
> >
> > Thank you for the new version. I only have a few comments left:
> > provider_enabled is not used
> > in the (!responder_enabled && provider != NULL && strcmp(provider, NO_PROVIDER)
> > case, please just print the debug message and call load_backend_module()
> > afterwards. So that the provider is loaded as requested in the config
> > file. This would also avoid the strange debug message
> >
> > (Tue Sep 11 18:46:38 2012) [sssd[be[ipa17.devel]]] [be_process_init_sudo] (0x0080): SUDO provider is set, but it is not listed in active services. SUDO support will not work!
> > (Tue Sep 11 18:46:38 2012) [sssd[be[ipa17.devel]]] [be_process_init] (0x2000): SUDO backend target successfully loaded from provider [(null)].
>
> Thank you. I had the changes prepared, but I forgot to amend the commit.
> The patch is attached.

ACK

bye,
Sumit

[Jakub Hrozek]

On Wed, Sep 12, 2012 at 02:23:41PM +0200, Sumit Bose wrote:
> On Wed, Sep 12, 2012 at 10:19:57AM +0200, Pavel Březina wrote:
> > On 09/11/2012 06:54 PM, Sumit Bose wrote:
> > > On Mon, Sep 10, 2012 at 01:52:53PM +0200, Pavel Březina wrote:
> > > > On 09/10/2012 10:48 AM, Sumit Bose wrote:
> > > > > On Mon, Sep 10, 2012 at 09:35:25AM +0200, Pavel Březina wrote:
> > > > > > On 09/07/2012 11:31 AM, Sumit Bose wrote:
> > > > > > >On Wed, Sep 05, 2012 at 03:05:49PM +0200, Pavel Březina wrote:
> > > > > > >>0001 fixes https://fedorahosted.org/sssd/ticket/1458
> > > > > > >>0002 fixes a memory leak when be_process_init() fails. I think it
> > > > > > >>should be fixed, even though the backend is in this case immediately
> > > > > > >>terminated
> > > > > > >
> > > > > > >Hi,
> > > > > > >
> > > > > > >the patches compile without errors and work as expected. Nevertheless I
> > > > > > >have a few comments:
> > > > > > >
> > > > > > >0001:
> > > > > > >  - I think it make sense to skip the initialization of the sudo target
> > > > > > >    if there is no responder configured. But to avoid surprises on the
> > > > > > >    user side I would recommend to do the initialization if it is
> > > > > > >    configured explicitly in sssd.conf, i.e sudo_provider = ldap, and
> > > > > > >    give a warning in the logs that the corresponding responder will not
> > > > > > >    be running.
> > > > > >
> > > > > > I discussed this with Jakub before writing the patch. sudo_provider
> > > > > > currently defaults to id_provider. If I get it right, you are
> > > > > > suggesting to make the default value "none".
> > > > >
> > > > > No, sorry for not being clear here. I think the id_provider default is
> > > > > right. I just wanted to say that if for whatever reasons the
> > > > > sudo_provider parameter is used explicitly in sssd.conf the
> > > > > sudo_provider should be initialization even if sudo is not listed in
> > > > > services.
> > > > >
> > > > > bye,
> > > > > Sumit
> > > > > > We agreed with Jakub that this makes more pressure on the user to
> > > > > > understand the configuration so we chose not to do it. Although the
> > > > > > configurations I've been seeing so far are mostly generated by
> > > > > > authconfig or ipa-client-install (where are all providers set
> > > > > > explicitly) so I don't think it is a problem after all. What do you say
> > > > > > Jakub?
> > > > > >
> > > > > > If we decide to do it this way, it will also require changes in man page.
> > > > > >
> > > > > > >>+    ret = confdb_get_string(ctx->cdb, ctx, CONFDB_MONITOR_CONF_ENTRY,
> > > > > > >>+                            CONFDB_MONITOR_ACTIVE_SERVICES, NULL, &services);
> > > > > > >>+    if (ret != EOK) {
> > > > > > >>+        DEBUG(SSSDBG_FATAL_FAILURE,
> > > > > > >>+              ("Unable to read from confdb [%d]: %s\n", ret, strerror(ret)));
> > > > > > >>+        return ret;
> > > > > > >>+    }
> > > > > > >>+
> > > > > > >
> > > > > > >  - I would prefer to use confdb_get_string_as_list() and use strcmp()
> > > > > > >    later on, to be on the safe side.
> > > > > > >
> > > > > > >0002: ACK
> > > > > > >
> > > > > > >bye,
> > > > > > >Sumit
> > > >
> > > > OK, that makes sense. Patches are attached.
> > > >
> > > > I moved the code into separate function.
> > >
> > > Thank you for the new version. I only have a few comments left:
> > > provider_enabled is not used
> > > in the (!responder_enabled && provider != NULL && strcmp(provider, NO_PROVIDER)
> > > case, please just print the debug message and call load_backend_module()
> > > afterwards. So that the provider is loaded as requested in the config
> > > file. This would also avoid the strange debug message
> > >
> > > (Tue Sep 11 18:46:38 2012) [sssd[be[ipa17.devel]]] [be_process_init_sudo] (0x0080): SUDO provider is set, but it is not listed in active services. SUDO support will not work!
> > > (Tue Sep 11 18:46:38 2012) [sssd[be[ipa17.devel]]] [be_process_init] (0x2000): SUDO backend target successfully loaded from provider [(null)].
> >
> > Thank you. I had the changes prepared, but I forgot to amend the commit.
> > The patch is attached.
>
> ACK
>
> bye,
> Sumit

Pushed to master.