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