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

[Ondrej Kos]

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