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. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
