[asterisk-dev] [svn-commits] tilghman: branch tilghman/res_odbc_astobj2 r101392 - /team/tilghman/res_odbc_...

[Russell Bryant]

I know this is in a branch and is just the first pass, but I wanted to
look at it anyway.  Hopefully it's helpful ...


SVN commits to the Digium repositories wrote:
> Author: tilghman
> Date: Thu Jan 31 01:32:54 2008
> New Revision: 101392
> 
> URL: http://svn.digium.com/view/asterisk?view=rev&rev=101392
> Log:
> Initial conversion
> 
> Modified:
>     team/tilghman/res_odbc_astobj2/res/res_odbc.c

> +static void odbc_class_destructor(void *data)
> +{
> +	struct odbc_class *class = data;
> +	/* Due to refcounts, we can safely assume that any objects with a reference
> +	 * to us will prevent our destruction, so we don't need to worry about them.
> +	 */
> +	if (class->username)
> +		ast_free(class->username);
> +	if (class->password)
> +		ast_free(class->password);
> +	if (class->sanitysql)
> +		ast_free(class->sanitysql);

I would changes this fields to use the stringfields API.  That way you
don't have to worry about making sure to free each individual one.  This
is especially nice in case you need to add any more in the future.

> +	ao2_ref(class->obj_container, -1);
> +	SQLFreeHandle(SQL_HANDLE_ENV, class->env);
> +	ast_free(class);
> +}

You don't free the object itself inside the destructor.  The astobj2
code does that for you.  This will crash.

> +
> +static int return0(const void *obj, const int flags)
> +{
> +	return 0;
> +}

I would rather see this function clearly named as a hash function, and
that it simply returns zero since you currently only use 1 bucket.

> +
> +static void odbc_obj_destructor(void *data)
> +{
> +	struct odbc_obj *obj = data;
> +	odbc_obj_disconnect(obj);
> +	ast_mutex_destroy(&obj->lock);
> +	ao2_ref(obj->parent, -1);
> +	ast_free(obj);
> +}

Same thing in this destructor as with the first one.  Don't free the
object, as it will go boom.

>  				if (sanitysql)
> -					ast_copy_string(new->sanitysql, sanitysql, sizeof(new->sanitysql));
> +					new->sanitysql = ast_strdup(sanitysql);

Don't forget to check for memory allocation failures here as well as
other places in this code.

> @@ -334,6 +351,7 @@
>  
>  				odbc_register_class(new, connect);
>  				ast_log(LOG_NOTICE, "Registered ODBC class '%s' dsn->[%s]\n", cat, dsn);
> +				ao2_ref(new, -1);
>  			}
>  		}
>  	}

It's good practice to always NULL out pointers after you unref them.  It
turns subtle issues into blatantly obvious ones if you access objects
that have already been unreferenced.

> +	while ((class = ao2_iterator_next(&aoi))) {
>  		if ((a->argc == 2) || (a->argc == 3 && !strcmp(a->argv[2], "all")) || (!strcmp(a->argv[2], class->name))) {
>  			int count = 0;
>  			ast_cli(a->fd, "  Name:   %s\n  DSN:    %s\n", class->name, class->dsn);
>  
>  			if (class->haspool) {
> +				struct ao2_iterator aoi2 = ao2_iterator_init(class->obj_container, 0);
> +
>  				ast_cli(a->fd, "  Pooled: Yes\n  Limit:  %d\n  Connections in use: %d\n", class->limit, class->count);
>  
> -				AST_LIST_TRAVERSE(&(class->odbc_obj), current, list) {
> -					ast_cli(a->fd, "    - Connection %d: %s\n", ++count, current->used ? "in use" : current->up && ast_odbc_sanity_check(current) ? "connected" : "disconnected");
> +				while ((current = ao2_iterator_next(&aoi2))) {
> +					ast_cli(a->fd, "    - Connection %d: %s\n", ++count,
> +						current->used ? "in use" :
> +						current->up && ast_odbc_sanity_check(current) ? "connected" : "disconnected");
> +					ao2_ref(current, -1);
>  				}
>  			} else {
>  				/* Should only ever be one of these */
> -				AST_LIST_TRAVERSE(&(class->odbc_obj), current, list) {
> +				struct ao2_iterator aoi2 = ao2_iterator_init(class->obj_container, 0);
> +				while ((current = ao2_iterator_next(&aoi2))) {
>  					ast_cli(a->fd, "  Pooled: No\n  Connected: %s\n", current->up && ast_odbc_sanity_check(current) ? "Yes" : "No");
> +					ao2_ref(current, -1);
>  				}
>  			}
>  			ast_cli(a->fd, "\n");
>  		}
>  	}
> -	AST_LIST_UNLOCK(&odbc_list);
>  
>  	return CLI_SUCCESS;
>  }

You missed unreferencing 'class' in that big while loop.  You can just
change the while to something like:

for (class = ao2_iterator_next(&aoi); class = ao2_iterator_next(&aoi);
ao2_ref(class, -1)) {


> @@ -411,9 +436,7 @@
>  {
>  	struct odbc_obj *obj;
>  	if (class) {
> -		AST_LIST_LOCK(&odbc_list);
> -		AST_LIST_INSERT_HEAD(&odbc_list, class, list);
> -		AST_LIST_UNLOCK(&odbc_list);
> +		ao2_link(class_container, class);

It looks like you're missing an unreference here.

>  			} else {
>  				obj->used = 1;
> -				AST_LIST_INSERT_TAIL(&class->odbc_obj, obj, list);
> +				ao2_link(class->obj_container, obj);
>  			}

Looks like you're leaking a reference here, too.  Don't forget the
change that we made such that ao2_link() automatically increasing the
reference count.  It no longer inherits a reference.

> -	}
> -	AST_LIST_UNLOCK(&class->odbc_obj);
> +				ao2_link(class->obj_container, obj);
> +			}
> +		}
> +	}

Same unref issue

>  	/* First, mark all to be purged */
> -	AST_LIST_LOCK(&odbc_list);
> -	AST_LIST_TRAVERSE(&odbc_list, class, list) {
> +	while ((class = ao2_iterator_next(&aoi))) {
>  		class->delme = 1;
>  	}

You forgot to unref every class in this loop.

> +	/* Purge remaining classes */
> +	aoi = ao2_iterator_init(class_container, OBJ_UNLINK);
> +	while ((class = ao2_iterator_next(&aoi))) {
> +		if (class->delme) {
> +			struct ao2_iterator aoi2 = ao2_iterator_init(class->obj_container, OBJ_UNLINK);
> +			while ((current = ao2_iterator_next(&aoi2))) {
> +				ao2_ref(current, -2);
> +			}
> +			ao2_ref(class, -1);
> +		}
> +		ao2_ref(class, -1);
> +	}

This loop seems is concerning for a number of reasons.  Really, you
should be able to simply unref to top level container and everything
should go away from that.  It also fixes the mem leak of
class_container.  :)

-- 
Russell Bryant
Senior Software Engineer
Open Source Team Lead
Digium, Inc.