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

Jakub Hrozek jhrozek at redhat.com
Wed Apr 24 20:13:10 UTC 2013


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.



More information about the sssd-devel mailing list