I decided not to only document the behaviour, but also add option to configure it (so I could remove one FIXME from the code).
https://fedorahosted.org/sssd/ticket/1718
Michal
On Fri, 2013-10-25 at 17:42 +0200, Michal Židek wrote:
I decided not to only document the behaviour, but also add option to configure it (so I could remove one FIXME from the code).
Thanks a lot, glad to see a long standing FIXME go away.
Ack, Simo.
On Fri, 2013-10-25 at 20:11 -0400, Simo Sorce wrote:
On Fri, 2013-10-25 at 17:42 +0200, Michal Židek wrote:
I decided not to only document the behaviour, but also add option to configure it (so I could remove one FIXME from the code).
Thanks a lot, glad to see a long standing FIXME go away.
Ack, Simo.
The code itself is good, but you also need to add the parameter to the confAPI. In particular, src/config/etc/sssd.api.conf and then make sure make check passes.
On 10/27/2013 07:32 PM, Jakub Hrozek wrote:
On Fri, 2013-10-25 at 20:11 -0400, Simo Sorce wrote:
On Fri, 2013-10-25 at 17:42 +0200, Michal Židek wrote:
I decided not to only document the behaviour, but also add option to configure it (so I could remove one FIXME from the code).
Thanks a lot, glad to see a long standing FIXME go away.
Ack, Simo.
The code itself is good, but you also need to add the parameter to the confAPI. In particular, src/config/etc/sssd.api.conf and then make sure make check passes.
Done.
Michal
On Thu, Oct 31, 2013 at 12:54:04PM +0100, Michal Židek wrote:
On 10/27/2013 07:32 PM, Jakub Hrozek wrote:
On Fri, 2013-10-25 at 20:11 -0400, Simo Sorce wrote:
On Fri, 2013-10-25 at 17:42 +0200, Michal Židek wrote:
I decided not to only document the behaviour, but also add option to configure it (so I could remove one FIXME from the code).
Thanks a lot, glad to see a long standing FIXME go away.
Ack, Simo.
The code itself is good, but you also need to add the parameter to the confAPI. In particular, src/config/etc/sssd.api.conf and then make sure make check passes.
Done.
Michal
Sorry I didn't think about it the first time around, but I think the code should also initialize offline_timeout to the default value so that if confdb_get_int fails for whatever reason, we still decide based on the default value. Ignoring the result if confdb_get_int is fine, but we could warn with a DEBUG message.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/31/2013 01:40 PM, Jakub Hrozek wrote:
On Thu, Oct 31, 2013 at 12:54:04PM +0100, Michal Židek wrote:
On 10/27/2013 07:32 PM, Jakub Hrozek wrote:
On Fri, 2013-10-25 at 20:11 -0400, Simo Sorce wrote:
On Fri, 2013-10-25 at 17:42 +0200, Michal Židek wrote:
I decided not to only document the behaviour, but also add option to configure it (so I could remove one FIXME from the code).
Thanks a lot, glad to see a long standing FIXME go away.
Ack, Simo.
The code itself is good, but you also need to add the parameter to the confAPI. In particular, src/config/etc/sssd.api.conf and then make sure make check passes.
Done.
Michal
Sorry I didn't think about it the first time around, but I think the code should also initialize offline_timeout to the default value so that if confdb_get_int fails for whatever reason, we still decide based on the default value. Ignoring the result if confdb_get_int is fine, but we could warn with a DEBUG message.
If confdb_get_int() fails, it can pretty much only mean that the value in the config file was not an integer. In that case, we should *really* be quitting.
I'd rather see us check this option at startup and store it in the be_ctx, that way we can abort (or loudly log that it's wrong and fall back to defaults) at startup.
On Fri, Nov 01, 2013 at 07:33:00AM -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/31/2013 01:40 PM, Jakub Hrozek wrote:
On Thu, Oct 31, 2013 at 12:54:04PM +0100, Michal Židek wrote:
On 10/27/2013 07:32 PM, Jakub Hrozek wrote:
On Fri, 2013-10-25 at 20:11 -0400, Simo Sorce wrote:
On Fri, 2013-10-25 at 17:42 +0200, Michal Židek wrote:
I decided not to only document the behaviour, but also add option to configure it (so I could remove one FIXME from the code).
Thanks a lot, glad to see a long standing FIXME go away.
Ack, Simo.
The code itself is good, but you also need to add the parameter to the confAPI. In particular, src/config/etc/sssd.api.conf and then make sure make check passes.
Done.
Michal
Sorry I didn't think about it the first time around, but I think the code should also initialize offline_timeout to the default value so that if confdb_get_int fails for whatever reason, we still decide based on the default value. Ignoring the result if confdb_get_int is fine, but we could warn with a DEBUG message.
If confdb_get_int() fails, it can pretty much only mean that the value in the config file was not an integer. In that case, we should *really* be quitting.
Not really, there can be a number of internal errors as well. I think the proper solution for the problem you describe is a configuration file validator, but that's really out of scope of this change.
I'd rather see us check this option at startup and store it in the be_ctx, that way we can abort (or loudly log that it's wrong and fall back to defaults) at startup.
I don't like extending be_ctx with per-option attributes. What if we converted all generic backend options into a dp_option array instead?
On 11/01/2013 01:42 PM, Jakub Hrozek wrote:
On Fri, Nov 01, 2013 at 07:33:00AM -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/31/2013 01:40 PM, Jakub Hrozek wrote:
On Thu, Oct 31, 2013 at 12:54:04PM +0100, Michal Židek wrote:
On 10/27/2013 07:32 PM, Jakub Hrozek wrote:
On Fri, 2013-10-25 at 20:11 -0400, Simo Sorce wrote:
On Fri, 2013-10-25 at 17:42 +0200, Michal Židek wrote: > I decided not to only document the behaviour, but also add > option to configure it (so I could remove one FIXME from > the code). > > https://fedorahosted.org/sssd/ticket/1718
Thanks a lot, glad to see a long standing FIXME go away.
Ack, Simo.
The code itself is good, but you also need to add the parameter to the confAPI. In particular, src/config/etc/sssd.api.conf and then make sure make check passes.
Done.
Michal
Sorry I didn't think about it the first time around, but I think the code should also initialize offline_timeout to the default value so that if confdb_get_int fails for whatever reason, we still decide based on the default value. Ignoring the result if confdb_get_int is fine, but we could warn with a DEBUG message.
If confdb_get_int() fails, it can pretty much only mean that the value in the config file was not an integer. In that case, we should *really* be quitting.
Not really, there can be a number of internal errors as well. I think the proper solution for the problem you describe is a configuration file validator, but that's really out of scope of this change.
I'd rather see us check this option at startup and store it in the be_ctx, that way we can abort (or loudly log that it's wrong and fall back to defaults) at startup.
I don't like extending be_ctx with per-option attributes. What if we
I was about to create a patch with offline_timeout in be_ctx, but it did not look very nice to me as well.
converted all generic backend options into a dp_option array instead?
This should be done separately. I filed a ticket for it: https://fedorahosted.org/sssd/ticket/2141
Michal
On Fri, Nov 01, 2013 at 05:36:54PM +0100, Michal Židek wrote:
On 11/01/2013 01:42 PM, Jakub Hrozek wrote:
On Fri, Nov 01, 2013 at 07:33:00AM -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/31/2013 01:40 PM, Jakub Hrozek wrote:
On Thu, Oct 31, 2013 at 12:54:04PM +0100, Michal Židek wrote:
On 10/27/2013 07:32 PM, Jakub Hrozek wrote:
On Fri, 2013-10-25 at 20:11 -0400, Simo Sorce wrote: >On Fri, 2013-10-25 at 17:42 +0200, Michal Židek wrote: >>I decided not to only document the behaviour, but also add >>option to configure it (so I could remove one FIXME from >>the code). >> >>https://fedorahosted.org/sssd/ticket/1718 > >Thanks a lot, glad to see a long standing FIXME go away. > >Ack, Simo. >
The code itself is good, but you also need to add the parameter to the confAPI. In particular, src/config/etc/sssd.api.conf and then make sure make check passes.
Done.
Michal
Sorry I didn't think about it the first time around, but I think the code should also initialize offline_timeout to the default value so that if confdb_get_int fails for whatever reason, we still decide based on the default value. Ignoring the result if confdb_get_int is fine, but we could warn with a DEBUG message.
If confdb_get_int() fails, it can pretty much only mean that the value in the config file was not an integer. In that case, we should *really* be quitting.
Not really, there can be a number of internal errors as well. I think the proper solution for the problem you describe is a configuration file validator, but that's really out of scope of this change.
I'd rather see us check this option at startup and store it in the be_ctx, that way we can abort (or loudly log that it's wrong and fall back to defaults) at startup.
I don't like extending be_ctx with per-option attributes. What if we
I was about to create a patch with offline_timeout in be_ctx, but it did not look very nice to me as well.
converted all generic backend options into a dp_option array instead?
This should be done separately. I filed a ticket for it: https://fedorahosted.org/sssd/ticket/2141
Michal
Stephen, do you agree that converting the generic options into dp_option array would be better than putting a single timeout into be_ctx ?
If so, I would accept Michal's patch for now and convert the options in a later release.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 11/05/2013 05:48 AM, Jakub Hrozek wrote:
On Fri, Nov 01, 2013 at 05:36:54PM +0100, Michal Židek wrote:
On 11/01/2013 01:42 PM, Jakub Hrozek wrote:
On Fri, Nov 01, 2013 at 07:33:00AM -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/31/2013 01:40 PM, Jakub Hrozek wrote:
On Thu, Oct 31, 2013 at 12:54:04PM +0100, Michal Židek wrote:
On 10/27/2013 07:32 PM, Jakub Hrozek wrote: > On Fri, 2013-10-25 at 20:11 -0400, Simo Sorce wrote: >> On Fri, 2013-10-25 at 17:42 +0200, Michal Židek >> wrote: >>> I decided not to only document the behaviour, but >>> also add option to configure it (so I could remove >>> one FIXME from the code). >>> >>> https://fedorahosted.org/sssd/ticket/1718 >> >> Thanks a lot, glad to see a long standing FIXME go >> away. >> >> Ack, Simo. >> > > The code itself is good, but you also need to add the > parameter to the confAPI. In particular, > src/config/etc/sssd.api.conf and then make sure make > check passes. >
Done.
Michal
Sorry I didn't think about it the first time around, but I think the code should also initialize offline_timeout to the default value so that if confdb_get_int fails for whatever reason, we still decide based on the default value. Ignoring the result if confdb_get_int is fine, but we could warn with a DEBUG message.
If confdb_get_int() fails, it can pretty much only mean that the value in the config file was not an integer. In that case, we should *really* be quitting.
Not really, there can be a number of internal errors as well. I think the proper solution for the problem you describe is a configuration file validator, but that's really out of scope of this change.
I'd rather see us check this option at startup and store it in the be_ctx, that way we can abort (or loudly log that it's wrong and fall back to defaults) at startup.
I don't like extending be_ctx with per-option attributes. What if we
I was about to create a patch with offline_timeout in be_ctx, but it did not look very nice to me as well.
converted all generic backend options into a dp_option array instead?
This should be done separately. I filed a ticket for it: https://fedorahosted.org/sssd/ticket/2141
Michal
Stephen, do you agree that converting the generic options into dp_option array would be better than putting a single timeout into be_ctx ?
If so, I would accept Michal's patch for now and convert the options in a later release.
Yes, I think that's probably a good move. Ack to this patch for now.
On Tue, Nov 05, 2013 at 08:14:26AM -0500, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 11/05/2013 05:48 AM, Jakub Hrozek wrote:
On Fri, Nov 01, 2013 at 05:36:54PM +0100, Michal Židek wrote:
On 11/01/2013 01:42 PM, Jakub Hrozek wrote:
On Fri, Nov 01, 2013 at 07:33:00AM -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/31/2013 01:40 PM, Jakub Hrozek wrote:
On Thu, Oct 31, 2013 at 12:54:04PM +0100, Michal Židek wrote: > On 10/27/2013 07:32 PM, Jakub Hrozek wrote: >> On Fri, 2013-10-25 at 20:11 -0400, Simo Sorce wrote: >>> On Fri, 2013-10-25 at 17:42 +0200, Michal Židek >>> wrote: >>>> I decided not to only document the behaviour, but >>>> also add option to configure it (so I could remove >>>> one FIXME from the code). >>>> >>>> https://fedorahosted.org/sssd/ticket/1718 >>> >>> Thanks a lot, glad to see a long standing FIXME go >>> away. >>> >>> Ack, Simo. >>> >> >> The code itself is good, but you also need to add the >> parameter to the confAPI. In particular, >> src/config/etc/sssd.api.conf and then make sure make >> check passes. >> > > Done. > > Michal >
Sorry I didn't think about it the first time around, but I think the code should also initialize offline_timeout to the default value so that if confdb_get_int fails for whatever reason, we still decide based on the default value. Ignoring the result if confdb_get_int is fine, but we could warn with a DEBUG message.
If confdb_get_int() fails, it can pretty much only mean that the value in the config file was not an integer. In that case, we should *really* be quitting.
Not really, there can be a number of internal errors as well. I think the proper solution for the problem you describe is a configuration file validator, but that's really out of scope of this change.
I'd rather see us check this option at startup and store it in the be_ctx, that way we can abort (or loudly log that it's wrong and fall back to defaults) at startup.
I don't like extending be_ctx with per-option attributes. What if we
I was about to create a patch with offline_timeout in be_ctx, but it did not look very nice to me as well.
converted all generic backend options into a dp_option array instead?
This should be done separately. I filed a ticket for it: https://fedorahosted.org/sssd/ticket/2141
Michal
Stephen, do you agree that converting the generic options into dp_option array would be better than putting a single timeout into be_ctx ?
If so, I would accept Michal's patch for now and convert the options in a later release.
Yes, I think that's probably a good move. Ack to this patch for now.
Pushed to master.
On 10/31/2013 06:40 PM, Jakub Hrozek wrote:
On Thu, Oct 31, 2013 at 12:54:04PM +0100, Michal Židek wrote:
On 10/27/2013 07:32 PM, Jakub Hrozek wrote:
On Fri, 2013-10-25 at 20:11 -0400, Simo Sorce wrote:
On Fri, 2013-10-25 at 17:42 +0200, Michal Židek wrote:
I decided not to only document the behaviour, but also add option to configure it (so I could remove one FIXME from the code).
Thanks a lot, glad to see a long standing FIXME go away.
Ack, Simo.
The code itself is good, but you also need to add the parameter to the confAPI. In particular, src/config/etc/sssd.api.conf and then make sure make check passes.
Done.
Michal
Sorry I didn't think about it the first time around, but I think the code should also initialize offline_timeout to the default value so that if confdb_get_int fails for whatever reason, we still decide based on the default value. Ignoring the result if confdb_get_int is fine, but we could warn with a DEBUG message.
Yes, sorry, I completely forgot about this, it would also add coverity warning, since I ignored the return value. But I think it is better to init the value after the confdb_get_int fails. I prefer to assume that output parameters of failed functions are always undefined (even if in this case, it would be safe to expect that the parameter value was not changed).
Thanks,
Michal
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Jakub Hrozek -
Michal Židek -
Simo Sorce -
Stephen Gallagher