Re: [SSSD] [PATCH] DB: Switch to new libini_config API

[Lukas Slebodnik]

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
> > [e-mail]
> > 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