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:
>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.
>_______________________________________________
>sssd-devel mailing list
>sssd-devel(a)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