[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