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

Ondrej Kos okos at redhat.com
Mon Apr 22 14:09:22 UTC 2013


On 04/19/2013 09:00 PM, Jakub Hrozek wrote:
> On Fri, Apr 19, 2013 at 04:05:49PM +0200, Ondrej Kos wrote:

> I know that the transaction logic in this function predates this patch,
> but I think we should fix it when we are changing the code anyway. Can
> you take a look at how we use the "in_transaction" booleans elsewhere
> for sysdb transaction and put the same logic here?
>
> Then you would always be able to just goto done and have the cleanup
> done on one place.

fixed, thanks for pointing out

>
> You're leaking tmp_ctx here and on several other places. The advice
> above would fix this problem.
>

fixed

>> +        DEBUG(SSSDBG_FATAL_FAILURE,
>> +                ("No stat data though stat collection was requested.\n"));
>
> I don't understand this error message, can you reword it?



>> +    ret = sss_ini_get_mtime(timestr, init_data);
>
> This function really should have a size_t parameter, an API cannot
> make any assumptions about the size of an input buffer, sorry.

fixed

>
>>       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

>> -    /* Purge existing database */
>> -    ret = confdb_purge(cdb);
>> -    if (ret != EOK) {
>> -        DEBUG(0, ("Could not purge existing configuration\n"));
>> -        close(fd);
>> +    } else {
>> +        DEBUG(SSSDBG_FATAL_FAILURE, ("Failed to get lastUpdate attribute.\n"));
>>           goto done;
>
> goto done is currently wrong here, it would attempt to cancel a
> transaction that hasn't started yet.

fixed

>
> [snip]
>
>> +    version = sss_ini_get_int_config_value(init_data, 1, -1, &ret);
>>       if (ret != EOK) {
>> -        DEBUG(0, ("Config file version could not be determined\n"));
>> +        DEBUG(SSSDBG_FATAL_FAILURE,
>> +                ("Config file version could not be determined\n"));
>
> All previous error handlers called sss_ini_close_file() and
> sss_ini_config_destroy() is being called from now on. Is that correct?

this was no longer destroying the file descriptor, but the loaded 
configuration object, since I now restructured the error handling, it's 
no longer there.

>> +PKG_CHECK_MODULES(INI_CONFIG, [
>> +    ini_config >= 1.0.0], [
>> +
>> +        HAVE_LIBINI_CONFIG_V1=1
>
> This assignment seems redundant to me. AC_DEFINE_UNQUOTED accepts an
> identifier as the first parameter.

you're right, fixed

>> +#include <stdio.h>
>> +#include <errno.h>
>> +#include <talloc.h>
>> +
>> +#include "config.h"
>> +#include "util/util.h"
>> +#include "util/sss_ini.h"
>> +#include "confdb/confdb_setup.h"
>> +#include "confdb/confdb_private.h"
>> +
>> +#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.

>> +/* Initialize data structure */
>> +
>> +struct sss_ini_initdata* sss_ini_initdata_init(TALLOC_CTX *tmp_ctx)
>
> Memory context that allocates output directly should not be named
> tmp_ctx but mem_ctx probably.

fixed

>> +/* Get mtime */
>> +
>> +int sss_ini_get_mtime(char *timestr, struct sss_ini_initdata *init_data)
>> +{
>> +#ifdef HAVE_LIBINI_CONFIG_V1
>> +    return snprintf(timestr, 21, "%llu",
>                                ^^^^^^^
>              This function should either have a big fat comment saying that
>              timestr is at least 21 bytes large, or (better) accept also
>              a size_t parameter that specifies the actual size.

fixed with size_t function parameter
>> +    /* Parse file */
>> +    ret = ini_config_parse(init_data->file,
>> +                           INI_STOP_ON_ANY,
>> +                           0, /* <- check that you really want this */
>
> What does this comment mean?

I forgot to remove the comment and replace the value with macro 
INI_MV1S_OVERWRITE, it's flag for handling collisions, in this case, 
duplicate is overwritten.

>> +/* Free ini config (v0) */
>> +
>> +void sss_ini_free_ini_config(struct sss_ini_initdata *init_data)
>> +{
>> +#ifdef HAVE_LIBINI_CONFIG_V1
>> +    return;
>> +#elif HAVE_LIBINI_CONFIG_V0
>> +    free_ini_config(init_data->sssd_config);
>> +#endif
>> +}
>> +
>> +
>> +
>> +/* Destroy ini config (v1) */
>> +
>> +void sss_ini_config_destroy(struct sss_ini_initdata *init_data)
>> +{
>> +#ifdef HAVE_LIBINI_CONFIG_V1
>> +    ini_config_destroy(init_data->sssd_config);
>> +#elif HAVE_LIBINI_CONFIG_V0
>> +    return;
>> +#endif
>> +}
>> +
>
> I don't get why these functions are separate, sounds like you should
> merge them to me?

in the previous code flow the config-free occured in different parts of 
code, because the structure data was needed further between versions.

> This new wrapper interface looks much better to me, but in general when
> designing such quasi-object interfaces that operate on a structure, the
> structure should always be the first (with the exception of talloc context)
> parameter and especially any output parameters should always come last.
>
> sss_confdb_create_ldif() got it right.
>
>> +/* Get mtime */
>> +int sss_ini_get_mtime(char *timestr, struct sss_ini_initdata *init_data);
>
> Functionally the code seems to be working well.


new patch attached.

Ondra


-- 
Ondrej Kos
Associate Software Engineer
Identity Management
Red Hat Czech
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-DB-Switch-to-new-libini_config-API.patch
Type: text/x-patch
Size: 34232 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130422/6f607832/attachment.bin>


More information about the sssd-devel mailing list