[SSSD] [PATCHES] Convert the SSSD to config_file_version 2

Stephen Gallagher sgallagh at redhat.com
Fri Sep 25 10:22:26 UTC 2009


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Nack. See below

On 09/25/2009 06:01 AM, Jakub Hrozek wrote:
> On 09/25/2009 10:48 AM, Sumit Bose wrote:
>> Hi,
> 
>> here are my comments:
> 
>> On Thu, Sep 24, 2009 at 09:32:27PM -0400, Stephen Gallagher wrote:
>>> Patch 0001 (sgallagh): Convert the existing options to the new names.
> 
>> --- a/server/providers/krb5/krb5_auth.c
>> +++ b/server/providers/krb5/krb5_auth.c
>> @@ -895,7 +895,7 @@ int sssm_krb5_auth_init(struct be_ctx *bectx,
>>      ctx->realm = value;
> 
>>      ret = confdb_get_string(bectx->cdb, ctx, bectx->conf_path,
>> -                            CONFDB_KRB5_REALM, "/tmp", &value);
>> +                            CONFDB_KRB5_CCACHEDIR, "/tmp", &value);
>>      if (ret != EOK) goto fail;
>>      ret = lstat(value, &stat_buf);
>>      if (ret != EOK) {
> 
> 
> 
> 
> 
> 
> Good catch! Fixed
> 
>>> Patch 0002 (jhrozek): Update the manpages to reflect the new names.
> 
>> - there is too much whitespace at line 771 of the patch
> 
> Fixed
> 
>> - in the example the sssd-krb5 the '[domains] section' is referred to and
>>   the example uses [domains/FOO] instead of [domain/FOO]. Maybe it
>>   should be underlined that this example is incomplete (missing
>>   id_proiver)
> 
> 
> OK, noted that the example does not show any id_provider, fixed the
> [domains] stuff
> 
> 
>>> Patch 0003 (jhrozek+sgallagh): Create a python script to convert
>>> existing config files to the new format and run it automatically during
>>> %post in the sssd.spec
>>>
> 
>> - is it planned to use upgrade_config.py for future upgrade, too, or is
>>   it planned to have separate tools for each upgrade. In the latter case
>>   I would call to something like upgrade_config_v1_v2.py
> 
> Probably true, renamed

Actually, I would prefer that we use this tool for potential future
upgrades (that I hope never to have...). It would be better for it to
detect which version is currently in place and upgrade it from 1 to 2,
then 2 to 3, etc. internally and then write out the current version.

This way we don't need to change the spec file again for that, as well.

> 
>> - when using '-f /somewhere/my.conf' the backup is created in /somewhere
>>   but the new config is written to /etc/sss/sssd.conf. My expectation was
>>   that the conversion would be done in-place. If you prefer it this way
>>   '--help' should say that the default output file is
>>   /etc/sss/sssd.conf
> 
> 
> Fixed, thanks

Uh, this doesn't look right to me:
+    if options.filename and not options.outfile:
+        options.outfile = options.filename
+    if not options.outfile:
+        options.outfile = '/etc/sssd/sssd.conf'

I think that second if needs to be elif, otherwise you're always going
to override setting options.outfile = options.filename

> 
>> - upgrade_config.py has problems with '%' used in the
>>   krb5_ccname_template:
> 
>> ERROR: '%' must be followed by '%' or '(', found: '%h/krb5cc_%u_%U_%p_%r_%P_%%_XXXXXX'
>> Traceback (most recent call last):
>>   File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 327, in main
>>     config.upgrade_v2(options.outfile, options.backup)
>>   File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 274, in upgrade_v2
>>     self._migrate_domains()
>>   File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 210, in _migrate_domains
>>     self._migrate_domain(domain)
>>   File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 200, in _migrate_domain
>>     self._migrate_kw(new_domsec, old_domsec, krb5_kw)
>>   File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 101, in _migrate_kw
>>     self._migrate_if_exists(to_section, new, from_section, old)
>>   File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 93, in _migrate_if_exists
>>     self._config.get(from_section, from_option))
>>   File "/usr/lib/python2.6/ConfigParser.py", line 545, in get
>>     return self._interpolate(section, option, value, d)
>>   File "/usr/lib/python2.6/ConfigParser.py", line 613, in _interpolate
>>     self._interpolate_some(option, L, rawval, section, vars, 1)
>>   File "/usr/lib/python2.6/ConfigParser.py", line 655, in _interpolate_some
>>     "'%%' must be followed by '%%' or '(', found: %r" % (rest,))
>> InterpolationSyntaxError: '%' must be followed by '%' or '(', found: '%h/krb5cc_%u_%U_%p_%r_%P_%%_XXXXXX'
> 
> 
> I switched to using RawConfigParser that does not perform this
> interpolation - we don't use it anyway and really want raw data.
> 
> 	Jakub

- ------------------------------------------------------------------------

_______________________________________________
sssd-devel mailing list
sssd-devel at lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel


- -- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkq8md4ACgkQeiVVYja6o6NFAQCgjH1vkCZUnyHOXXRYGBU9sqKA
CtIAnjN5klt2WWkAFO/Ra95X6Wt+28SG
=BypZ
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list