Hi,
Attached is patch for https://fedorahosted.org/sssd/ticket/1786
Patch extends sssd code so it's capable to build with current version of libini_config (0.7 at the time, supported versions up from 0.6.1) and with version 1.0.0, which will be released soon.
Ondra
On Thu, Mar 28, 2013 at 03:59:25PM +0100, Ondrej Kos wrote:
Hi,
Attached is patch for https://fedorahosted.org/sssd/ticket/1786
Patch extends sssd code so it's capable to build with current version of libini_config (0.7 at the time, supported versions up from 0.6.1) and with version 1.0.0, which will be released soon.
I can use this patch as an interim solution in rawhide and F19 until we ack it but it needs some work.
Mainly, please don't use ifdefs in general code. ifdefs should be used in a wrapper module to provide kind of generic methods the user would then call but ifdefs in main code obfuscate it. See src/util/sss_krb5.c or src/util/sss_ldap.c for examples.
We should amend our coding guidelines to specifically advise to put ifdefs into a wrapper module.
I know that there is the netlink module which does the same but arguably the netlink module is a wrapper module mostly and actually we might want to split it in future (feel free to file a ticket).
So there should be a src/util/sss_col.c module or something like that that would export functions or macros with the same prototype no matter the libini version. That module will contain a function that will return ldif which can be then consumed by confdb.
Also please don't use the old debug levels in new code.
On 03/29/2013 01:19 PM, Jakub Hrozek wrote:
On Thu, Mar 28, 2013 at 03:59:25PM +0100, Ondrej Kos wrote:
Hi,
Attached is patch for https://fedorahosted.org/sssd/ticket/1786
Patch extends sssd code so it's capable to build with current version of libini_config (0.7 at the time, supported versions up from 0.6.1) and with version 1.0.0, which will be released soon.
I can use this patch as an interim solution in rawhide and F19 until we ack it but it needs some work.
Mainly, please don't use ifdefs in general code. ifdefs should be used in a wrapper module to provide kind of generic methods the user would then call but ifdefs in main code obfuscate it. See src/util/sss_krb5.c or src/util/sss_ldap.c for examples.
We should amend our coding guidelines to specifically advise to put ifdefs into a wrapper module.
I know that there is the netlink module which does the same but arguably the netlink module is a wrapper module mostly and actually we might want to split it in future (feel free to file a ticket).
So there should be a src/util/sss_col.c module or something like that that would export functions or macros with the same prototype no matter the libini version. That module will contain a function that will return ldif which can be then consumed by confdb.
Also please don't use the old debug levels in new code.
Hi,
New patch is attached, it supplies src/util/sss_ini.c & src/util/sss_ini.h, which are exporting functions used in confdb_setup.
Please, let me know, if this is the approach you meant, and with comments for the coding.
Ondra
On 04/10/2013 10:17 AM, Ondrej Kos wrote:
On 03/29/2013 01:19 PM, Jakub Hrozek wrote:
On Thu, Mar 28, 2013 at 03:59:25PM +0100, Ondrej Kos wrote:
Hi,
Attached is patch for https://fedorahosted.org/sssd/ticket/1786
Patch extends sssd code so it's capable to build with current version of libini_config (0.7 at the time, supported versions up from 0.6.1) and with version 1.0.0, which will be released soon.
I can use this patch as an interim solution in rawhide and F19 until we ack it but it needs some work.
Mainly, please don't use ifdefs in general code. ifdefs should be used in a wrapper module to provide kind of generic methods the user would then call but ifdefs in main code obfuscate it. See src/util/sss_krb5.c or src/util/sss_ldap.c for examples.
We should amend our coding guidelines to specifically advise to put ifdefs into a wrapper module.
I know that there is the netlink module which does the same but arguably the netlink module is a wrapper module mostly and actually we might want to split it in future (feel free to file a ticket).
So there should be a src/util/sss_col.c module or something like that that would export functions or macros with the same prototype no matter the libini version. That module will contain a function that will return ldif which can be then consumed by confdb.
Also please don't use the old debug levels in new code.
Hi,
New patch is attached, it supplies src/util/sss_ini.c & src/util/sss_ini.h, which are exporting functions used in confdb_setup.
Please, let me know, if this is the approach you meant, and with comments for the coding.
I scanned the patch and I generally like the approach. Prefix "iniw_" looks a bit weird because "w" does not ring the bell but this is just me. :-)
Ondra
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (10/04/13 13:54), Dmitri Pal wrote:
On 04/10/2013 10:17 AM, Ondrej Kos wrote:
On 03/29/2013 01:19 PM, Jakub Hrozek wrote:
On Thu, Mar 28, 2013 at 03:59:25PM +0100, Ondrej Kos wrote:
Hi,
Attached is patch for https://fedorahosted.org/sssd/ticket/1786
Patch extends sssd code so it's capable to build with current version of libini_config (0.7 at the time, supported versions up from 0.6.1) and with version 1.0.0, which will be released soon.
I can use this patch as an interim solution in rawhide and F19 until we ack it but it needs some work.
Mainly, please don't use ifdefs in general code. ifdefs should be used in a wrapper module to provide kind of generic methods the user would then call but ifdefs in main code obfuscate it. See src/util/sss_krb5.c or src/util/sss_ldap.c for examples.
We should amend our coding guidelines to specifically advise to put ifdefs into a wrapper module.
I know that there is the netlink module which does the same but arguably the netlink module is a wrapper module mostly and actually we might want to split it in future (feel free to file a ticket).
So there should be a src/util/sss_col.c module or something like that that would export functions or macros with the same prototype no matter the libini version. That module will contain a function that will return ldif which can be then consumed by confdb.
Also please don't use the old debug levels in new code.
Hi,
New patch is attached, it supplies src/util/sss_ini.c & src/util/sss_ini.h, which are exporting functions used in confdb_setup.
Please, let me know, if this is the approach you meant, and with comments for the coding.
I scanned the patch and I generally like the approach. Prefix "iniw_" looks a bit weird because "w" does not ring the bell but this is just me. :-)
Ondra
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-- Thank you, Dmitri Pal
Sr. Engineering Manager for IdM portfolio Red Hat Inc.
I tried just ot compile with both version of libini.
$ make distcheck
...
CC src/util/sss_ini.lo ../src/util/sss_ini.c:31:26: fatal error: util/sss_ini.h: No such file or directory compilation terminated. make[3]: *** [src/util/sss_ini.lo] Error 1 make[3]: Leaving directory `/dev/shm/sssd_build/sssd-1.9.92/_build' make[2]: *** [all-recursive] Error 1 make[2]: Leaving directory `/dev/shm/sssd_build/sssd-1.9.92/_build' make[1]: *** [all] Error 2 make[1]: Leaving directory `/dev/shm/sssd_build/sssd-1.9.92/_build' make: *** [distcheck] Error 1
You forgot to add src/util/sss_ini.h to dist_noinst_HEADERS im Makafile.am
LS
On 04/11/2013 09:02 AM, Lukas Slebodnik wrote:
On (10/04/13 13:54), Dmitri Pal wrote:
On 04/10/2013 10:17 AM, Ondrej Kos wrote:
On 03/29/2013 01:19 PM, Jakub Hrozek wrote:
On Thu, Mar 28, 2013 at 03:59:25PM +0100, Ondrej Kos wrote:
Hi,
Attached is patch for https://fedorahosted.org/sssd/ticket/1786
Patch extends sssd code so it's capable to build with current version of libini_config (0.7 at the time, supported versions up from 0.6.1) and with version 1.0.0, which will be released soon.
I can use this patch as an interim solution in rawhide and F19 until we ack it but it needs some work.
Mainly, please don't use ifdefs in general code. ifdefs should be used in a wrapper module to provide kind of generic methods the user would then call but ifdefs in main code obfuscate it. See src/util/sss_krb5.c or src/util/sss_ldap.c for examples.
We should amend our coding guidelines to specifically advise to put ifdefs into a wrapper module.
I know that there is the netlink module which does the same but arguably the netlink module is a wrapper module mostly and actually we might want to split it in future (feel free to file a ticket).
So there should be a src/util/sss_col.c module or something like that that would export functions or macros with the same prototype no matter the libini version. That module will contain a function that will return ldif which can be then consumed by confdb.
Also please don't use the old debug levels in new code.
Hi,
New patch is attached, it supplies src/util/sss_ini.c & src/util/sss_ini.h, which are exporting functions used in confdb_setup.
Please, let me know, if this is the approach you meant, and with comments for the coding.
I scanned the patch and I generally like the approach. Prefix "iniw_" looks a bit weird because "w" does not ring the bell but this is just me. :-)
This was leftover from previous patch, it was inspired from when I was working on the libnl support. iniw stands for ini wrapper, I changed it to respect style of the rest of the module.
Ondra
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-- Thank you, Dmitri Pal
Sr. Engineering Manager for IdM portfolio Red Hat Inc.
I tried just ot compile with both version of libini.
$ make distcheck
...
CC src/util/sss_ini.lo ../src/util/sss_ini.c:31:26: fatal error: util/sss_ini.h: No such file or directory compilation terminated. make[3]: *** [src/util/sss_ini.lo] Error 1 make[3]: Leaving directory `/dev/shm/sssd_build/sssd-1.9.92/_build' make[2]: *** [all-recursive] Error 1 make[2]: Leaving directory `/dev/shm/sssd_build/sssd-1.9.92/_build' make[1]: *** [all] Error 2 make[1]: Leaving directory `/dev/shm/sssd_build/sssd-1.9.92/_build' make: *** [distcheck] Error 1
You forgot to add src/util/sss_ini.h to dist_noinst_HEADERS im Makafile.am
LS
Thanks, I must've missed it.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Updated patch attached.
Ondra
On (11/04/13 11:52), Ondrej Kos wrote:
On 04/11/2013 09:02 AM, Lukas Slebodnik wrote:
On (10/04/13 13:54), Dmitri Pal wrote:
On 04/10/2013 10:17 AM, Ondrej Kos wrote:
On 03/29/2013 01:19 PM, Jakub Hrozek wrote:
On Thu, Mar 28, 2013 at 03:59:25PM +0100, Ondrej Kos wrote:
Hi,
Attached is patch for https://fedorahosted.org/sssd/ticket/1786
Patch extends sssd code so it's capable to build with current version of libini_config (0.7 at the time, supported versions up from 0.6.1) and with version 1.0.0, which will be released soon.
I can use this patch as an interim solution in rawhide and F19 until we ack it but it needs some work.
Mainly, please don't use ifdefs in general code. ifdefs should be used in a wrapper module to provide kind of generic methods the user would then call but ifdefs in main code obfuscate it. See src/util/sss_krb5.c or src/util/sss_ldap.c for examples.
We should amend our coding guidelines to specifically advise to put ifdefs into a wrapper module.
I know that there is the netlink module which does the same but arguably the netlink module is a wrapper module mostly and actually we might want to split it in future (feel free to file a ticket).
So there should be a src/util/sss_col.c module or something like that that would export functions or macros with the same prototype no matter the libini version. That module will contain a function that will return ldif which can be then consumed by confdb.
Also please don't use the old debug levels in new code.
Hi,
New patch is attached, it supplies src/util/sss_ini.c & src/util/sss_ini.h, which are exporting functions used in confdb_setup.
Please, let me know, if this is the approach you meant, and with comments for the coding.
I scanned the patch and I generally like the approach. Prefix "iniw_" looks a bit weird because "w" does not ring the bell but this is just me. :-)
This was leftover from previous patch, it was inspired from when I was working on the libnl support. iniw stands for ini wrapper, I changed it to respect style of the rest of the module.
Ondra
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-- Thank you, Dmitri Pal
Sr. Engineering Manager for IdM portfolio Red Hat Inc.
I tried just ot compile with both version of libini.
$ make distcheck
...
CC src/util/sss_ini.lo ../src/util/sss_ini.c:31:26: fatal error: util/sss_ini.h: No such file or directory compilation terminated. make[3]: *** [src/util/sss_ini.lo] Error 1 make[3]: Leaving directory `/dev/shm/sssd_build/sssd-1.9.92/_build' make[2]: *** [all-recursive] Error 1 make[2]: Leaving directory `/dev/shm/sssd_build/sssd-1.9.92/_build' make[1]: *** [all] Error 2 make[1]: Leaving directory `/dev/shm/sssd_build/sssd-1.9.92/_build' make: *** [distcheck] Error 1
You forgot to add src/util/sss_ini.h to dist_noinst_HEADERS im Makafile.am
LS
Thanks, I must've missed it.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Updated patch attached.
Ondra
Sorry for late answer.
Your patch works fine with libini_config >=1.0.0.1 (fedora 19). But sssd segfault with libini_config=0.7.0 (fedora 18)
Core was generated by `sssd -i'. Program terminated with signal 11, Segmentation fault. #0 fstat (__statbuf=0xd525d8, __fd=<error reading variable: Cannot access memory at address 0x5>) at /usr/include/sys/stat.h:470 470 return __fxstat (_STAT_VER, __fd, __statbuf); #0 fstat (__statbuf=0xd525d8, __fd=<error reading variable: Cannot access memory at address 0x5>) at /usr/include/sys/stat.h:470 #1 sss_ini_get_stat (file_desc=0x5, cstat=cstat@entry=0xd525d8) at src/util/sss_ini.c:101 #2 0x0000000000414b73 in confdb_init_db (config_file=config_file@entry=0xd4a400 "/etc/sssd/sssd.conf", cdb=0xd4a7e0) at src/confdb/confdb_setup.c:172 #3 0x000000000040485c in load_configuration (monitor=<synthetic pointer>, config_file=0xd4a400 "/etc/sssd/sssd.conf", mem_ctx=0xd4a390) at src/monitor/monitor.c:1613 #4 main (argc=<optimized out>, argv=<optimized out>) at src/monitor/monitor.c:2766
LS
On Fri, Apr 19, 2013 at 12:37:41PM +0200, Lukas Slebodnik wrote:
On (11/04/13 11:52), Ondrej Kos wrote:
On 04/11/2013 09:02 AM, Lukas Slebodnik wrote:
On (10/04/13 13:54), Dmitri Pal wrote:
On 04/10/2013 10:17 AM, Ondrej Kos wrote:
On 03/29/2013 01:19 PM, Jakub Hrozek wrote:
On Thu, Mar 28, 2013 at 03:59:25PM +0100, Ondrej Kos wrote: >Hi, > >Attached is patch for https://fedorahosted.org/sssd/ticket/1786 > >Patch extends sssd code so it's capable to build with current >version of libini_config (0.7 at the time, supported versions up >from 0.6.1) and with version 1.0.0, which will be released soon. >
I can use this patch as an interim solution in rawhide and F19 until we ack it but it needs some work.
Mainly, please don't use ifdefs in general code. ifdefs should be used in a wrapper module to provide kind of generic methods the user would then call but ifdefs in main code obfuscate it. See src/util/sss_krb5.c or src/util/sss_ldap.c for examples.
We should amend our coding guidelines to specifically advise to put ifdefs into a wrapper module.
I know that there is the netlink module which does the same but arguably the netlink module is a wrapper module mostly and actually we might want to split it in future (feel free to file a ticket).
So there should be a src/util/sss_col.c module or something like that that would export functions or macros with the same prototype no matter the libini version. That module will contain a function that will return ldif which can be then consumed by confdb.
Also please don't use the old debug levels in new code.
Hi,
New patch is attached, it supplies src/util/sss_ini.c & src/util/sss_ini.h, which are exporting functions used in confdb_setup.
Please, let me know, if this is the approach you meant, and with comments for the coding.
I scanned the patch and I generally like the approach. Prefix "iniw_" looks a bit weird because "w" does not ring the bell but this is just me. :-)
This was leftover from previous patch, it was inspired from when I was working on the libnl support. iniw stands for ini wrapper, I changed it to respect style of the rest of the module.
Ondra
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-- Thank you, Dmitri Pal
Sr. Engineering Manager for IdM portfolio Red Hat Inc.
I tried just ot compile with both version of libini.
$ make distcheck
...
CC src/util/sss_ini.lo ../src/util/sss_ini.c:31:26: fatal error: util/sss_ini.h: No such file or directory compilation terminated. make[3]: *** [src/util/sss_ini.lo] Error 1 make[3]: Leaving directory `/dev/shm/sssd_build/sssd-1.9.92/_build' make[2]: *** [all-recursive] Error 1 make[2]: Leaving directory `/dev/shm/sssd_build/sssd-1.9.92/_build' make[1]: *** [all] Error 2 make[1]: Leaving directory `/dev/shm/sssd_build/sssd-1.9.92/_build' make: *** [distcheck] Error 1
You forgot to add src/util/sss_ini.h to dist_noinst_HEADERS im Makafile.am
LS
Thanks, I must've missed it.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Updated patch attached.
Ondra
Sorry for late answer.
Your patch works fine with libini_config >=1.0.0.1 (fedora 19). But sssd segfault with libini_config=0.7.0 (fedora 18)
Core was generated by `sssd -i'. Program terminated with signal 11, Segmentation fault. #0 fstat (__statbuf=0xd525d8, __fd=<error reading variable: Cannot access memory at address 0x5>) at /usr/include/sys/stat.h:470 470 return __fxstat (_STAT_VER, __fd, __statbuf); #0 fstat (__statbuf=0xd525d8, __fd=<error reading variable: Cannot access memory at address 0x5>) at /usr/include/sys/stat.h:470 #1 sss_ini_get_stat (file_desc=0x5, cstat=cstat@entry=0xd525d8) at src/util/sss_ini.c:101 #2 0x0000000000414b73 in confdb_init_db (config_file=config_file@entry=0xd4a400 "/etc/sssd/sssd.conf", cdb=0xd4a7e0) at src/confdb/confdb_setup.c:172 #3 0x000000000040485c in load_configuration (monitor=<synthetic pointer>, config_file=0xd4a400 "/etc/sssd/sssd.conf", mem_ctx=0xd4a390) at src/monitor/monitor.c:1613 #4 main (argc=<optimized out>, argv=<optimized out>) at src/monitor/monitor.c:2766
LS
We also did a short review in person. tl;dr -- do not use void* but make the structure opaque and always pass the pointer to the structure regardless of its contents.
On 04/19/2013 01:29 PM, Jakub Hrozek wrote:
On Fri, Apr 19, 2013 at 12:37:41PM +0200, Lukas Slebodnik wrote:
On (11/04/13 11:52), Ondrej Kos wrote:
On 04/11/2013 09:02 AM, Lukas Slebodnik wrote:
On (10/04/13 13:54), Dmitri Pal wrote:
On 04/10/2013 10:17 AM, Ondrej Kos wrote:
On 03/29/2013 01:19 PM, Jakub Hrozek wrote: > On Thu, Mar 28, 2013 at 03:59:25PM +0100, Ondrej Kos wrote: >> Hi, >> >> Attached is patch for https://fedorahosted.org/sssd/ticket/1786 >> >> Patch extends sssd code so it's capable to build with current >> version of libini_config (0.7 at the time, supported versions up > >from 0.6.1) and with version 1.0.0, which will be released soon. >> > > I can use this patch as an interim solution in rawhide and F19 until we > ack it but it needs some work. > > Mainly, please don't use ifdefs in general code. ifdefs should be > used in > a wrapper module to provide kind of generic methods the user would then > call but ifdefs in main code obfuscate it. See src/util/sss_krb5.c or > src/util/sss_ldap.c for examples. > > We should amend our coding guidelines to specifically advise to put > ifdefs into a wrapper module. > > I know that there is the netlink module which does the same but arguably > the netlink module is a wrapper module mostly and actually we might want > to split it in future (feel free to file a ticket). > > So there should be a src/util/sss_col.c module or something like that > that would export functions or macros with the same prototype no matter > the libini version. That module will contain a function that will return > ldif which can be then consumed by confdb. > > Also please don't use the old debug levels in new code.
Hi,
New patch is attached, it supplies src/util/sss_ini.c & src/util/sss_ini.h, which are exporting functions used in confdb_setup.
Please, let me know, if this is the approach you meant, and with comments for the coding.
I scanned the patch and I generally like the approach. Prefix "iniw_" looks a bit weird because "w" does not ring the bell but this is just me. :-)
This was leftover from previous patch, it was inspired from when I was working on the libnl support. iniw stands for ini wrapper, I changed it to respect style of the rest of the module.
Ondra
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-- Thank you, Dmitri Pal
Sr. Engineering Manager for IdM portfolio Red Hat Inc.
I tried just ot compile with both version of libini.
$ make distcheck
...
CC src/util/sss_ini.lo ../src/util/sss_ini.c:31:26: fatal error: util/sss_ini.h: No such file or directory compilation terminated. make[3]: *** [src/util/sss_ini.lo] Error 1 make[3]: Leaving directory `/dev/shm/sssd_build/sssd-1.9.92/_build' make[2]: *** [all-recursive] Error 1 make[2]: Leaving directory `/dev/shm/sssd_build/sssd-1.9.92/_build' make[1]: *** [all] Error 2 make[1]: Leaving directory `/dev/shm/sssd_build/sssd-1.9.92/_build' make: *** [distcheck] Error 1
You forgot to add src/util/sss_ini.h to dist_noinst_HEADERS im Makafile.am
LS
Thanks, I must've missed it.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Updated patch attached.
Ondra
Sorry for late answer.
Your patch works fine with libini_config >=1.0.0.1 (fedora 19). But sssd segfault with libini_config=0.7.0 (fedora 18)
Core was generated by `sssd -i'. Program terminated with signal 11, Segmentation fault. #0 fstat (__statbuf=0xd525d8, __fd=<error reading variable: Cannot access memory at address 0x5>) at /usr/include/sys/stat.h:470 470 return __fxstat (_STAT_VER, __fd, __statbuf); #0 fstat (__statbuf=0xd525d8, __fd=<error reading variable: Cannot access memory at address 0x5>) at /usr/include/sys/stat.h:470 #1 sss_ini_get_stat (file_desc=0x5, cstat=cstat@entry=0xd525d8) at src/util/sss_ini.c:101 #2 0x0000000000414b73 in confdb_init_db (config_file=config_file@entry=0xd4a400 "/etc/sssd/sssd.conf", cdb=0xd4a7e0) at src/confdb/confdb_setup.c:172 #3 0x000000000040485c in load_configuration (monitor=<synthetic pointer>, config_file=0xd4a400 "/etc/sssd/sssd.conf", mem_ctx=0xd4a390) at src/monitor/monitor.c:1613 #4 main (argc=<optimized out>, argv=<optimized out>) at src/monitor/monitor.c:2766
LS
This was really weird, it worked correctly in my environment, even if i rebased on the current master, but started segfaulting when i backed few patches. The code was wrong, but it wasn't failing.
We also did a short review in person. tl;dr -- do not use void* but make the structure opaque and always pass the pointer to the structure regardless of its contents.
My first idea was for the functions to accept just the necessary variables, but this will be better, the code is more readable now and the maintenance will be easier, thanks for advice!
New patch is attached
Ondra
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Fri, Apr 19, 2013 at 04:05:49PM +0200, Ondrej Kos wrote:
This was really weird, it worked correctly in my environment, even if i rebased on the current master, but started segfaulting when i backed few patches. The code was wrong, but it wasn't failing.
We also did a short review in person. tl;dr -- do not use void* but make the structure opaque and always pass the pointer to the structure regardless of its contents.
My first idea was for the functions to accept just the necessary variables, but this will be better, the code is more readable now and the maintenance will be easier, thanks for advice!
New patch is attached
Ondra
Thank you, some more comments are inline:
int confdb_init_db(const char *config_file, struct confdb_ctx *cdb) {
- TALLOC_CTX *tmp_ctx; int ret;
- int fd = -1;
- struct collection_item *sssd_config = NULL;
- struct collection_item *error_list = NULL;
- struct collection_item *item = NULL;
- int version;
- char timestr[21];
- char *lasttimestr; const char *config_ldif;
- struct ldb_ldif *ldif;
- TALLOC_CTX *tmp_ctx;
- char *lasttimestr, timestr[21]; const char *vals[2] = { timestr, NULL };
- struct stat cstat;
- int version;
- struct ldb_ldif *ldif;
- struct sss_ini_initdata *init_data;
I know that the transaction logic in this function predates this patch, but I think we should fix it when we are changing the code anyway. Can you take a look at how we use the "in_transaction" booleans elsewhere for sysdb transaction and put the same logic here?
Then you would always be able to just goto done and have the cleanup done on one place.
tmp_ctx = talloc_new(cdb);
- if (tmp_ctx == NULL) return ENOMEM;
- if (tmp_ctx == NULL) {
DEBUG(SSSDBG_FATAL_FAILURE, ("Out of memory.\n"));
return ENOMEM;
- }
- ret = check_and_open_readonly(config_file, &fd, 0, 0, (S_IRUSR|S_IWUSR),
CHECK_REG);
- init_data = sss_ini_initdata_init(tmp_ctx);
- if (!init_data) {
DEBUG(SSSDBG_FATAL_FAILURE, ("Out of memory.\n"));
You're leaking tmp_ctx here and on several other places. The advice above would fix this problem.
return ENOMEM;
- }
[snip]
- ret = fstat(fd, &cstat);
- if (ret != 0) {
DEBUG(0, ("Unable to stat config file [%s]! (%d [%s])\n",
config_file, errno, strerror(errno)));
close(fd);
- ret = sss_ini_get_stat(init_data);
- if (ret != EOK) {
DEBUG(SSSDBG_FATAL_FAILURE,
("No stat data though stat collection was requested.\n"));
I don't understand this error message, can you reword it?
}sss_ini_close_file(init_data); talloc_zfree(tmp_ctx); return errno;
- ret = snprintf(timestr, 21, "%llu", (long long unsigned)cstat.st_mtime);
- errno = 0;
- ret = sss_ini_get_mtime(timestr, init_data);
This function really should have a size_t parameter, an API cannot make any assumptions about the size of an input buffer, sorry.
if (ret <= 0 || ret >= 21) {
you can use sizeof(timestr) here in the odd case timestr actually changed.
[snip]
- /* Purge existing database */
- ret = confdb_purge(cdb);
- if (ret != EOK) {
DEBUG(0, ("Could not purge existing configuration\n"));
close(fd);
- } else {
DEBUG(SSSDBG_FATAL_FAILURE, ("Failed to get lastUpdate attribute.\n")); goto done;
goto done is currently wrong here, it would attempt to cancel a transaction that hasn't started yet.
[snip]
- version = sss_ini_get_int_config_value(init_data, 1, -1, &ret); if (ret != EOK) {
DEBUG(0, ("Config file version could not be determined\n"));
DEBUG(SSSDBG_FATAL_FAILURE,
("Config file version could not be determined\n"));
All previous error handlers called sss_ini_close_file() and sss_ini_config_destroy() is being called from now on. Is that correct?
diff --git a/src/external/libini_config.m4 b/src/external/libini_config.m4 index f41f31917d1cf12dda49ec2e1f2411e39dbcc759..fc2c2a4456e05b7fb1354eb96fe124192f411d85 100644 --- a/src/external/libini_config.m4 +++ b/src/external/libini_config.m4 @@ -1,10 +1,22 @@ +PKG_CHECK_MODULES(INI_CONFIG, [
- ini_config >= 1.0.0], [
HAVE_LIBINI_CONFIG_V1=1
This assignment seems redundant to me. AC_DEFINE_UNQUOTED accepts an identifier as the first parameter.
AC_DEFINE_UNQUOTED(HAVE_LIBINI_CONFIG_V1, 1, [libini_config version greater than 1.0.0])
- ], [
AC_MSG_WARN([libini_config-devel >= 1.0.0 not available, trying older version])
PKG_CHECK_MODULES(INI_CONFIG, [
ini_config >= 0.6.1], [
HAVE_LIBINI_CONFIG_V0=1
Not needed either.
AC_DEFINE_UNQUOTED(HAVE_LIBINI_CONFIG_V0, 1, [libini_config version lesser than 1.0.0])
], [
AC_MSG_ERROR([Please install libini_config-devel])
]
)
- ]
+) --- /dev/null +++ b/src/util/sss_ini.c @@ -0,0 +1,466 @@ +/*
- SSSD
- sss_ini.c
- Authors:
Ondrej Kos <okos@redhat.com>
- Copyright (C) 2013 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#include <stdio.h> +#include <errno.h> +#include <talloc.h>
+#include "config.h" +#include "util/util.h" +#include "util/sss_ini.h" +#include "confdb/confdb_setup.h" +#include "confdb/confdb_private.h"
+#ifdef HAVE_LIBINI_CONFIG_V1 +#include "ini_configobj.h" +#endif +#include "ini_config.h"
+#ifdef HAVE_LIBINI_CONFIG_V0 +#include "collection.h" +#include "collection_tools.h" +#endif
We should be paranoid here and have an #else that would contain an #error directive. It is OK to add this just once, I guess, the compiler would error out.
+/* Initialize data structure */
+struct sss_ini_initdata* sss_ini_initdata_init(TALLOC_CTX *tmp_ctx)
Memory context that allocates output directly should not be named tmp_ctx but mem_ctx probably.
+{
- return talloc_zero(tmp_ctx, struct sss_ini_initdata);
+}
+/* Close file descriptor */
+void sss_ini_close_file(struct sss_ini_initdata *init_data) +{
It is a custom with any close/free functions in most projects to gracefully handle if the object being closed is not initialized.
So please add here: if (init_data == NULL || init_data->return == NULL) { return; }
The same needs to be done for sss_ini_free_ini_config and sss_ini_config_destroy.
+#ifdef HAVE_LIBINI_CONFIG_V1
- ini_config_file_destroy(init_data->file);
+#elif HAVE_LIBINI_CONFIG_V0
- close(init_data->file);
+#endif +}
+/* Open configuration file */
+int sss_ini_config_file_open(const char *config_file,
struct sss_ini_initdata *init_data)
+{ +#ifdef HAVE_LIBINI_CONFIG_V1
- return ini_config_file_open(config_file,
INI_META_STATS,
&init_data->file);
+#elif HAVE_LIBINI_CONFIG_V0
- return check_and_open_readonly(config_file, &init_data->file, 0, 0,
(S_IRUSR|S_IWUSR), CHECK_REG);
+#endif +}
+/* Check configuration file permissions */
+int sss_ini_config_access_check(struct sss_ini_initdata *init_data) +{ +#ifdef HAVE_LIBINI_CONFIG_V1
- return ini_config_access_check(init_data->file,
INI_ACCESS_CHECK_MODE |
INI_ACCESS_CHECK_UID |
INI_ACCESS_CHECK_GID,
0, /* owned by root */
0, /* owned by root */
(S_IRUSR|S_IWUSR), /* rw------- */
0); /* check all there parts */
+#elif HAVE_LIBINI_CONFIG_V0
- return EOK;
+#endif +}
+/* Get cstat */
+int sss_ini_get_stat(struct sss_ini_initdata *init_data) +{ +#ifdef HAVE_LIBINI_CONFIG_V1
- init_data->cstat = ini_config_get_stat(file_desc);
- if (!init_data->cstat) return EIO;
- return EOK;
+#elif HAVE_LIBINI_CONFIG_V0
- return fstat(init_data->file, &init_data->cstat);
+#endif +}
+/* Get mtime */
+int sss_ini_get_mtime(char *timestr, struct sss_ini_initdata *init_data) +{ +#ifdef HAVE_LIBINI_CONFIG_V1
- return snprintf(timestr, 21, "%llu",
^^^^^^^ This function should either have a big fat comment saying that timestr is at least 21 bytes large, or (better) accept also a size_t parameter that specifies the actual size.
(long long unsigned)init_data->cstat->st_mtime);
+#elif HAVE_LIBINI_CONFIG_V0
- return snprintf(timestr, 21, "%llu",
(long long unsigned)init_data->cstat.st_mtime);
+#endif +}
+/* Load configuration */
+int sss_ini_get_config(const char *config_file,
struct sss_ini_initdata *init_data)
+{
- int ret;
+#ifdef HAVE_LIBINI_CONFIG_V1
- /* Create config object */
- ret = ini_config_create(&(init_data->sssd_config));
- if (ret != EOK) {
DEBUG(SSSDBG_FATAL_FAILURE,
("Failed to create config object. Error %d.\n", ret));
return ret;
- }
- /* Parse file */
- ret = ini_config_parse(init_data->file,
INI_STOP_ON_ANY,
0, /* <- check that you really want this */
What does this comment mean?
0,
init_data->sssd_config);
- if (ret != EOK) {
DEBUG(SSSDBG_FATAL_FAILURE,
("Failed to parse configuration. Error %d.\n", ret));
if (ini_config_error_count(init_data->sssd_config)) {
DEBUG(SSSDBG_FATAL_FAILURE,
("Errors detected while parsing: %s\n",
ini_config_get_filename(init_data->file));
ini_config_get_errors(init_data->sssd_config,
&init_data->error_list);
ini_config_print_errors(stderr, init_data->error_list));
You can't unconditionally use stderr here, if sssd was running as a deamon, stderr would be closed. Use debug_file instead.
ini_config_free_errors(init_data->error_list);
}
ini_config_destroy(init_data->sssd_config);
return ret;
- }
- return ret;
+#elif HAVE_LIBINI_CONFIG_V0
- /* Read the configuration into a collection */
- ret = config_from_fd("sssd",
init_data->file,
config_file,
&init_data->sssd_config,
INI_STOP_ON_ANY,
&init_data->error_list);
- close(init_data->file);
- if (ret != EOK) {
DEBUG(SSSDBG_FATAL_FAILURE,
("Parse error reading configuration file [%s]\n",
config_file));
print_file_parsing_errors(stderr, init_data->error_list);
free_ini_config_errors(init_data->error_list);
free_ini_config(init_data->sssd_config);
return ret;
- }
- return ret;
You can safely return EOK here, this path is reachable on success only right?
+#endif +}
[snip]
+/* Free ini config (v0) */
+void sss_ini_free_ini_config(struct sss_ini_initdata *init_data) +{ +#ifdef HAVE_LIBINI_CONFIG_V1
- return;
+#elif HAVE_LIBINI_CONFIG_V0
- free_ini_config(init_data->sssd_config);
+#endif +}
+/* Destroy ini config (v1) */
+void sss_ini_config_destroy(struct sss_ini_initdata *init_data) +{ +#ifdef HAVE_LIBINI_CONFIG_V1
- ini_config_destroy(init_data->sssd_config);
+#elif HAVE_LIBINI_CONFIG_V0
- return;
+#endif +}
I don't get why these functions are separate, sounds like you should merge them to me?
+/* Create LDIF */
+int sss_confdb_create_ldif(TALLOC_CTX *mem_ctx,
struct sss_ini_initdata *init_data,
const char **config_ldif)
There are some old-style DEBUG messages left in this function.
[snip]
+#ifndef __SSS_INI_H__ +#define __SSS_INI_H__
+/* Structure declarations */
+/* INI data structure */ +struct sss_ini_initdata;
+/* Function declarations */
+/* Initialize data structure */ +struct sss_ini_initdata* sss_ini_initdata_init(TALLOC_CTX *tmp_ctx);
+/* Close file descriptor */ +void sss_ini_close_file(struct sss_ini_initdata *init_data);
+/* Open config file */ +int sss_ini_config_file_open(const char *config_file,
struct sss_ini_initdata *init_data);
+/* Check file permissions */ +int sss_ini_config_access_check(struct sss_ini_initdata *init_data);
+/* Cstat */ +int sss_ini_get_stat(struct sss_ini_initdata *init_data);
This new wrapper interface looks much better to me, but in general when designing such quasi-object interfaces that operate on a structure, the structure should always be the first (with the exception of talloc context) parameter and especially any output parameters should always come last.
sss_confdb_create_ldif() got it right.
+/* Get mtime */ +int sss_ini_get_mtime(char *timestr, struct sss_ini_initdata *init_data);
Functionally the code seems to be working well.
On 04/19/2013 09:00 PM, Jakub Hrozek wrote:
On Fri, Apr 19, 2013 at 04:05:49PM +0200, Ondrej Kos wrote:
I know that the transaction logic in this function predates this patch, but I think we should fix it when we are changing the code anyway. Can you take a look at how we use the "in_transaction" booleans elsewhere for sysdb transaction and put the same logic here?
Then you would always be able to just goto done and have the cleanup done on one place.
fixed, thanks for pointing out
You're leaking tmp_ctx here and on several other places. The advice above would fix this problem.
fixed
DEBUG(SSSDBG_FATAL_FAILURE,
("No stat data though stat collection was requested.\n"));
I don't understand this error message, can you reword it?
- ret = sss_ini_get_mtime(timestr, init_data);
This function really should have a size_t parameter, an API cannot make any assumptions about the size of an input buffer, sorry.
fixed
if (ret <= 0 || ret >= 21) {
you can use sizeof(timestr) here in the odd case timestr actually changed.
quoting the reference: Return Value The number of characters that would have been written if n had been sufficiently large, not counting the terminating null character. If an encoding error occurs, a negative number is returned. Notice that only when this returned value is non-negative and less than n, the string has been completely written. (where N is the sizeof parameter)
so I'd rather leave it this way, seems more defensive
- /* Purge existing database */
- ret = confdb_purge(cdb);
- if (ret != EOK) {
DEBUG(0, ("Could not purge existing configuration\n"));
close(fd);
- } else {
DEBUG(SSSDBG_FATAL_FAILURE, ("Failed to get lastUpdate attribute.\n")); goto done;
goto done is currently wrong here, it would attempt to cancel a transaction that hasn't started yet.
fixed
[snip]
- version = sss_ini_get_int_config_value(init_data, 1, -1, &ret); if (ret != EOK) {
DEBUG(0, ("Config file version could not be determined\n"));
DEBUG(SSSDBG_FATAL_FAILURE,
("Config file version could not be determined\n"));
All previous error handlers called sss_ini_close_file() and sss_ini_config_destroy() is being called from now on. Is that correct?
this was no longer destroying the file descriptor, but the loaded configuration object, since I now restructured the error handling, it's no longer there.
+PKG_CHECK_MODULES(INI_CONFIG, [
- ini_config >= 1.0.0], [
HAVE_LIBINI_CONFIG_V1=1
This assignment seems redundant to me. AC_DEFINE_UNQUOTED accepts an identifier as the first parameter.
you're right, fixed
+#include <stdio.h> +#include <errno.h> +#include <talloc.h>
+#include "config.h" +#include "util/util.h" +#include "util/sss_ini.h" +#include "confdb/confdb_setup.h" +#include "confdb/confdb_private.h"
+#ifdef HAVE_LIBINI_CONFIG_V1 +#include "ini_configobj.h" +#endif +#include "ini_config.h"
+#ifdef HAVE_LIBINI_CONFIG_V0 +#include "collection.h" +#include "collection_tools.h" +#endif
We should be paranoid here and have an #else that would contain an #error directive. It is OK to add this just once, I guess, the compiler would error out.
I don't think it's necessary, but fixed.
+/* Initialize data structure */
+struct sss_ini_initdata* sss_ini_initdata_init(TALLOC_CTX *tmp_ctx)
Memory context that allocates output directly should not be named tmp_ctx but mem_ctx probably.
fixed
+/* Get mtime */
+int sss_ini_get_mtime(char *timestr, struct sss_ini_initdata *init_data) +{ +#ifdef HAVE_LIBINI_CONFIG_V1
- return snprintf(timestr, 21, "%llu",
^^^^^^^ This function should either have a big fat comment saying that timestr is at least 21 bytes large, or (better) accept also a size_t parameter that specifies the actual size.
fixed with size_t function parameter
- /* Parse file */
- ret = ini_config_parse(init_data->file,
INI_STOP_ON_ANY,
0, /* <- check that you really want this */
What does this comment mean?
I forgot to remove the comment and replace the value with macro INI_MV1S_OVERWRITE, it's flag for handling collisions, in this case, duplicate is overwritten.
+/* Free ini config (v0) */
+void sss_ini_free_ini_config(struct sss_ini_initdata *init_data) +{ +#ifdef HAVE_LIBINI_CONFIG_V1
- return;
+#elif HAVE_LIBINI_CONFIG_V0
- free_ini_config(init_data->sssd_config);
+#endif +}
+/* Destroy ini config (v1) */
+void sss_ini_config_destroy(struct sss_ini_initdata *init_data) +{ +#ifdef HAVE_LIBINI_CONFIG_V1
- ini_config_destroy(init_data->sssd_config);
+#elif HAVE_LIBINI_CONFIG_V0
- return;
+#endif +}
I don't get why these functions are separate, sounds like you should merge them to me?
in the previous code flow the config-free occured in different parts of code, because the structure data was needed further between versions.
This new wrapper interface looks much better to me, but in general when designing such quasi-object interfaces that operate on a structure, the structure should always be the first (with the exception of talloc context) parameter and especially any output parameters should always come last.
sss_confdb_create_ldif() got it right.
+/* Get mtime */ +int sss_ini_get_mtime(char *timestr, struct sss_ini_initdata *init_data);
Functionally the code seems to be working well.
new patch attached.
Ondra
On 04/22/2013 10:09 AM, Ondrej Kos wrote:
if (ret <= 0 || ret >= 21) {
you can use sizeof(timestr) here in the odd case timestr actually changed.
quoting the reference: Return Value The number of characters that would have been written if n had been sufficiently large, not counting the terminating null character. If an encoding error occurs, a negative number is returned. Notice that only when this returned value is non-negative and less than n, the string has been completely written. (where N is the sizeof parameter)
so I'd rather leave it this way, seems more defensive
I am not sure it is more defensive.
If you do not want to to use sizeof I am fine but I would at least use:
#define TIME_STRING_SIZE 21
...
char timebuf[TIME_STRING_SIZE];
...
if (ret <= 0 || ret >= TIME_STRING_SIZE) {
...
Seems more defensive to me that literally "21".
On Mon, Apr 22, 2013 at 12:23:20PM -0400, Dmitri Pal wrote:
On 04/22/2013 10:09 AM, Ondrej Kos wrote:
if (ret <= 0 || ret >= 21) {
you can use sizeof(timestr) here in the odd case timestr actually changed.
quoting the reference: Return Value The number of characters that would have been written if n had been sufficiently large, not counting the terminating null character. If an encoding error occurs, a negative number is returned. Notice that only when this returned value is non-negative and less than n, the string has been completely written. (where N is the sizeof parameter)
so I'd rather leave it this way, seems more defensive
I am not sure it is more defensive.
If you do not want to to use sizeof I am fine but I would at least use:
#define TIME_STRING_SIZE 21
...
char timebuf[TIME_STRING_SIZE];
...
if (ret <= 0 || ret >= TIME_STRING_SIZE) {
...
Seems more defensive to me that literally "21".
Yes, that's pretty much what I meant. sizeof would be OK too, since the array is in the same scope:
if (ret <= 0 || ret >= sizeof(timebuf)) { /* Error */ }
But as Dmitri said, using magic constans is calling for trouble. Maybe not in this code, because the size is unlikely to change, but let's follow best practices everywhere :-)
On Mon, Apr 22, 2013 at 04:09:22PM +0200, Ondrej Kos wrote:
if (ret <= 0 || ret >= 21) {
you can use sizeof(timestr) here in the odd case timestr actually changed.
quoting the reference: Return Value The number of characters that would have been written if n had been sufficiently large, not counting the terminating null character. If an encoding error occurs, a negative number is returned. Notice that only when this returned value is non-negative and less than n, the string has been completely written. (where N is the sizeof parameter)
so I'd rather leave it this way, seems more defensive
Discussed elsewhere in the thread.
[snip]
+#ifdef HAVE_LIBINI_CONFIG_V1 +#include "ini_configobj.h" +#endif +#include "ini_config.h"
+#ifdef HAVE_LIBINI_CONFIG_V0 +#include "collection.h" +#include "collection_tools.h" +#endif
We should be paranoid here and have an #else that would contain an #error directive. It is OK to add this just once, I guess, the compiler would error out.
I don't think it's necessary, but fixed.
It may not be needed now, but what if LIBINI_CONFIG_V2 comes out? We should make sure that the code either compiles where it can or errors out with a reasonable message.
I also noticed one more thing I'd like to get fixed. "ini_config.h" is always included, but it's kind of in between the ifdefs. Can you shuffle the includes to read:
----------- #ifdef HAVE_LIBINI_CONFIG_V1 #include "ini_configobj.h" #elif HAVE_LIBINI_CONFIG_V0 #include "collection.h" #include "collection_tools.h" #else #error "Unsupported INI version" #endif
#include "ini_config.h" -----------
The #error doesn't have to be used from then on, I think, the compilation would error out anyway.
The other changes look good to me now. To sum it up: 1) Use either sizeof, #define, or, since we already switched to C99, a const int instead of hardcoded array length for the timebuf.
2) the ifdef change above
3) sss_confdb_create_ldif() has style issues. They predate your changes, most probably but we should fix them when we're moving the code. In particual, there should be a whitespace after a keyword -- use "if (" not "if(" and "for (" not "for(".
4) sss_ini_check_config_obj() could be made more readable to:
int sss_ini_check_config_obj(struct sss_ini_initdata *init_data) { if (init_data->obj == NULL) { return ENOENT; }
return EOK; }
A code shouldn't only return from if-else staments.
I think the patch will be good to go when we fix these last nitpicks.
On 04/24/2013 10:13 PM, Jakub Hrozek wrote:
On Mon, Apr 22, 2013 at 04:09:22PM +0200, Ondrej Kos wrote:
if (ret <= 0 || ret >= 21) {
you can use sizeof(timestr) here in the odd case timestr actually changed.
quoting the reference: Return Value The number of characters that would have been written if n had been sufficiently large, not counting the terminating null character. If an encoding error occurs, a negative number is returned. Notice that only when this returned value is non-negative and less than n, the string has been completely written. (where N is the sizeof parameter)
so I'd rather leave it this way, seems more defensive
Discussed elsewhere in the thread.
[snip]
+#ifdef HAVE_LIBINI_CONFIG_V1 +#include "ini_configobj.h" +#endif +#include "ini_config.h"
+#ifdef HAVE_LIBINI_CONFIG_V0 +#include "collection.h" +#include "collection_tools.h" +#endif
We should be paranoid here and have an #else that would contain an #error directive. It is OK to add this just once, I guess, the compiler would error out.
I don't think it's necessary, but fixed.
It may not be needed now, but what if LIBINI_CONFIG_V2 comes out? We should make sure that the code either compiles where it can or errors out with a reasonable message.
I also noticed one more thing I'd like to get fixed. "ini_config.h" is always included, but it's kind of in between the ifdefs. Can you shuffle the includes to read:
#ifdef HAVE_LIBINI_CONFIG_V1 #include "ini_configobj.h" #elif HAVE_LIBINI_CONFIG_V0 #include "collection.h" #include "collection_tools.h" #else #error "Unsupported INI version" #endif
#include "ini_config.h"
The #error doesn't have to be used from then on, I think, the compilation would error out anyway.
The other changes look good to me now. To sum it up:
- Use either sizeof, #define, or, since we already switched to C99, a
const int instead of hardcoded array length for the timebuf.
the ifdef change above
sss_confdb_create_ldif() has style issues. They predate your
changes, most probably but we should fix them when we're moving the code. In particual, there should be a whitespace after a keyword -- use "if (" not "if(" and "for (" not "for(".
- sss_ini_check_config_obj() could be made more readable to:
int sss_ini_check_config_obj(struct sss_ini_initdata *init_data) { if (init_data->obj == NULL) { return ENOENT; }
return EOK;
}
A code shouldn't only return from if-else staments.
I think the patch will be good to go when we fix these last nitpicks. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Patch with fixes attached.
On (25/04/13 11:45), Ondrej Kos wrote:
On 04/24/2013 10:13 PM, Jakub Hrozek wrote:
On Mon, Apr 22, 2013 at 04:09:22PM +0200, Ondrej Kos wrote:
if (ret <= 0 || ret >= 21) {
you can use sizeof(timestr) here in the odd case timestr actually changed.
quoting the reference: Return Value The number of characters that would have been written if n had been sufficiently large, not counting the terminating null character. If an encoding error occurs, a negative number is returned. Notice that only when this returned value is non-negative and less than n, the string has been completely written. (where N is the sizeof parameter)
so I'd rather leave it this way, seems more defensive
Discussed elsewhere in the thread.
[snip]
+#ifdef HAVE_LIBINI_CONFIG_V1 +#include "ini_configobj.h" +#endif +#include "ini_config.h"
+#ifdef HAVE_LIBINI_CONFIG_V0 +#include "collection.h" +#include "collection_tools.h" +#endif
We should be paranoid here and have an #else that would contain an #error directive. It is OK to add this just once, I guess, the compiler would error out.
I don't think it's necessary, but fixed.
It may not be needed now, but what if LIBINI_CONFIG_V2 comes out? We should make sure that the code either compiles where it can or errors out with a reasonable message.
I also noticed one more thing I'd like to get fixed. "ini_config.h" is always included, but it's kind of in between the ifdefs. Can you shuffle the includes to read:
#ifdef HAVE_LIBINI_CONFIG_V1 #include "ini_configobj.h" #elif HAVE_LIBINI_CONFIG_V0 #include "collection.h" #include "collection_tools.h" #else #error "Unsupported INI version" #endif
#include "ini_config.h"
The #error doesn't have to be used from then on, I think, the compilation would error out anyway.
The other changes look good to me now. To sum it up:
- Use either sizeof, #define, or, since we already switched to C99, a
const int instead of hardcoded array length for the timebuf.
the ifdef change above
sss_confdb_create_ldif() has style issues. They predate your
changes, most probably but we should fix them when we're moving the code. In particual, there should be a whitespace after a keyword -- use "if (" not "if(" and "for (" not "for(".
- sss_ini_check_config_obj() could be made more readable to:
int sss_ini_check_config_obj(struct sss_ini_initdata *init_data) { if (init_data->obj == NULL) { return ENOENT; }
return EOK;
}
A code shouldn't only return from if-else staments.
I think the patch will be good to go when we fix these last nitpicks. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Patch with fixes attached.
-- Ondrej Kos Associate Software Engineer Identity Management Red Hat Czech
I tested patches with libini_config 0.7 (f18) and libini_config 1.0 (f19). There aren't compile time warnings after applying patches and sssd works well with both versions of libini_config.
LS
On Thu, Apr 25, 2013 at 07:42:24PM +0200, Lukas Slebodnik wrote:
I tested patches with libini_config 0.7 (f18) and libini_config 1.0 (f19). There aren't compile time warnings after applying patches and sssd works well with both versions of libini_config.
LS
Thank you very much for testing Lukas. The code looks good to me now.
Ack
On Fri, Apr 26, 2013 at 12:26:15AM +0200, Jakub Hrozek wrote:
On Thu, Apr 25, 2013 at 07:42:24PM +0200, Lukas Slebodnik wrote:
I tested patches with libini_config 0.7 (f18) and libini_config 1.0 (f19). There aren't compile time warnings after applying patches and sssd works well with both versions of libini_config.
LS
Thank you very much for testing Lukas. The code looks good to me now.
Ack
Pushed to master.
sssd-devel@lists.fedorahosted.org