These patches make use of the krb5.conf for setting the default ccache template instead of relying on a pre-configured default set a package build time.
The build time parameter is still used as a fallback default if krb5.conf and libkrb5 profile library returns no value.
Tested on my machine and works just fine (requires restart of sssd if you change krb5.conf).
Simo.
On Mon, 2013-08-26 at 18:00 -0400, Simo Sorce wrote:
These patches make use of the krb5.conf for setting the default ccache template instead of relying on a pre-configured default set a package build time.
The build time parameter is still used as a fallback default if krb5.conf and libkrb5 profile library returns no value.
Tested on my machine and works just fine (requires restart of sssd if you change krb5.conf).
At Jakub's request on IRC, attached revised patches that also change the sssd-krb man page and fix the contrib/ patch used by make rpms.
Simo.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 08/27/2013 01:31 PM, Simo Sorce wrote:
On Mon, 2013-08-26 at 18:00 -0400, Simo Sorce wrote:
These patches make use of the krb5.conf for setting the default ccache template instead of relying on a pre-configured default set a package build time.
The build time parameter is still used as a fallback default if krb5.conf and libkrb5 profile library returns no value.
Tested on my machine and works just fine (requires restart of sssd if you change krb5.conf).
At Jakub's request on IRC, attached revised patches that also change the sssd-krb man page and fix the contrib/ patch used by make rpms.
Patch 0001: Ack
Patch 0002: Nack
As discussed on IRC, please split out the patch that removes the contrib patch into a separate patch in the set and modify it so that it reverts to using the built-in defaults (which means updating the sssd.spec.in as well).
In sss_get_system_ccname_template(), you fail to check the result of the talloc_strdup(). Also, by convention you should have an explicit ret = EOK in the success case just before the done: label.
Wouldn't it make more sense to return ENOENT than a NULL ccname? from sss_get_system_ccname_template()? It's not critical, but we tend to use that approach in the rest of the code. It would be more clear in the code what you're doing here too. I had to read it again after reading the consuming function to understand it properly.
Otherwise, it looks good to me.
On Tue, 2013-08-27 at 14:05 -0400, Stephen Gallagher wrote:
Patch 0002: Nack
As discussed on IRC, please split out the patch that removes the contrib patch into a separate patch in the set and modify it so that it reverts to using the built-in defaults (which means updating the sssd.spec.in as well).
Ah I had all in one patch, splitting ...
In sss_get_system_ccname_template(), you fail to check the result of the talloc_strdup(). Also, by convention you should have an explicit ret = EOK in the success case just before the done: label.
It was actually intentional, but your comment made me think that we probably want to fail if the strdup fails, changed.
Wouldn't it make more sense to return ENOENT than a NULL ccname? from sss_get_system_ccname_template()? It's not critical, but we tend to use that approach in the rest of the code. It would be more clear in the code what you're doing here too. I had to read it again after reading the consuming function to understand it properly.
I was a bit doubtful because ENOENT always result ina "File not found error" which is quite misleading. So I added a new sssd error of ERR_NOT_FOUND, hpe this is ok.
Simo.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 08/27/2013 03:22 PM, Simo Sorce wrote:
On Tue, 2013-08-27 at 14:05 -0400, Stephen Gallagher wrote:
Patch 0002: Nack
As discussed on IRC, please split out the patch that removes the contrib patch into a separate patch in the set and modify it so that it reverts to using the built-in defaults (which means updating the sssd.spec.in as well).
Ah I had all in one patch, splitting ...
In sss_get_system_ccname_template(), you fail to check the result of the talloc_strdup(). Also, by convention you should have an explicit ret = EOK in the success case just before the done: label.
It was actually intentional, but your comment made me think that we probably want to fail if the strdup fails, changed.
Wouldn't it make more sense to return ENOENT than a NULL ccname? from sss_get_system_ccname_template()? It's not critical, but we tend to use that approach in the rest of the code. It would be more clear in the code what you're doing here too. I had to read it again after reading the consuming function to understand it properly.
I was a bit doubtful because ENOENT always result ina "File not found error" which is quite misleading. So I added a new sssd error of ERR_NOT_FOUND, hpe this is ok.
Simo.
I think you sent the wrong patch 0002 here, Simo.
On Tue, 2013-08-27 at 15:22 -0400, Simo Sorce wrote:
On Tue, 2013-08-27 at 14:05 -0400, Stephen Gallagher wrote:
Patch 0002: Nack
As discussed on IRC, please split out the patch that removes the contrib patch into a separate patch in the set and modify it so that it reverts to using the built-in defaults (which means updating the sssd.spec.in as well).
Ah I had all in one patch, splitting ...
In sss_get_system_ccname_template(), you fail to check the result of the talloc_strdup(). Also, by convention you should have an explicit ret = EOK in the success case just before the done: label.
It was actually intentional, but your comment made me think that we probably want to fail if the strdup fails, changed.
Wouldn't it make more sense to return ENOENT than a NULL ccname? from sss_get_system_ccname_template()? It's not critical, but we tend to use that approach in the rest of the code. It would be more clear in the code what you're doing here too. I had to read it again after reading the consuming function to understand it properly.
I was a bit doubtful because ENOENT always result ina "File not found error" which is quite misleading. So I added a new sssd error of ERR_NOT_FOUND, hpe this is ok.
And now with the right patches.
Simo.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 08/27/2013 04:42 PM, Simo Sorce wrote:
On Tue, 2013-08-27 at 15:22 -0400, Simo Sorce wrote:
On Tue, 2013-08-27 at 14:05 -0400, Stephen Gallagher wrote:
Patch 0002: Nack
As discussed on IRC, please split out the patch that removes the contrib patch into a separate patch in the set and modify it so that it reverts to using the built-in defaults (which means updating the sssd.spec.in as well).
Ah I had all in one patch, splitting ...
In sss_get_system_ccname_template(), you fail to check the result of the talloc_strdup(). Also, by convention you should have an explicit ret = EOK in the success case just before the done: label.
It was actually intentional, but your comment made me think that we probably want to fail if the strdup fails, changed.
Wouldn't it make more sense to return ENOENT than a NULL ccname? from sss_get_system_ccname_template()? It's not critical, but we tend to use that approach in the rest of the code. It would be more clear in the code what you're doing here too. I had to read it again after reading the consuming function to understand it properly.
I was a bit doubtful because ENOENT always result ina "File not found error" which is quite misleading. So I added a new sssd error of ERR_NOT_FOUND, hpe this is ok.
And now with the right patches.
Ack to all three.
On Tue, Aug 27, 2013 at 04:48:55PM -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 08/27/2013 04:42 PM, Simo Sorce wrote:
On Tue, 2013-08-27 at 15:22 -0400, Simo Sorce wrote:
On Tue, 2013-08-27 at 14:05 -0400, Stephen Gallagher wrote:
Patch 0002: Nack
As discussed on IRC, please split out the patch that removes the contrib patch into a separate patch in the set and modify it so that it reverts to using the built-in defaults (which means updating the sssd.spec.in as well).
Ah I had all in one patch, splitting ...
In sss_get_system_ccname_template(), you fail to check the result of the talloc_strdup(). Also, by convention you should have an explicit ret = EOK in the success case just before the done: label.
It was actually intentional, but your comment made me think that we probably want to fail if the strdup fails, changed.
Wouldn't it make more sense to return ENOENT than a NULL ccname? from sss_get_system_ccname_template()? It's not critical, but we tend to use that approach in the rest of the code. It would be more clear in the code what you're doing here too. I had to read it again after reading the consuming function to understand it properly.
I was a bit doubtful because ENOENT always result ina "File not found error" which is quite misleading. So I added a new sssd error of ERR_NOT_FOUND, hpe this is ok.
And now with the right patches.
Ack to all three.
Pushed to master and sssd-1-10
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Simo Sorce -
Stephen Gallagher