[lxc-devel] [PATCH] Eliminate duplicate entries from list_active_containers

Serge Hallyn serge.hallyn at ubuntu.com
Tue Oct 22 00:22:47 UTC 2013


Quoting S.Çağlar Onur (caglar at 10ur.org):
> list_active_containers parses /proc/net/unix which can contain multiple entries for the same container;
> 
> 0000000000000000: 00000002 00000000 00010000 0001 01 273672 @/var/lib/lxc/6/command
> 0000000000000000: 00000002 00000000 00010000 0001 01 274395 @/var/lib/lxc/5/command
> 0000000000000000: 00000002 00000000 00010000 0001 01 273890 @/var/lib/lxc/4/command
> 0000000000000000: 00000002 00000000 00010000 0001 01 273141 @/var/lib/lxc/3/command
> 0000000000000000: 00000002 00000000 00010000 0001 01 273915 @/var/lib/lxc/2/command
> 0000000000000000: 00000002 00000000 00010000 0001 01 273683 @/var/lib/lxc/1/command
> 0000000000000000: 00000002 00000000 00010000 0001 01 273074 @/var/lib/lxc/0/command
> 0000000000000000: 00000002 00000000 00010000 0001 01 273931 @/var/lib/lxc/9/command
> 0000000000000000: 00000002 00000000 00010000 0001 01 273110 @/var/lib/lxc/8/command
> 0000000000000000: 00000002 00000000 00010000 0001 01 273390 @/var/lib/lxc/7/command
> 0000000000000000: 00000003 00000000 00000000 0001 03 275903 @/var/lib/lxc/8/command
> 0000000000000000: 00000003 00000000 00000000 0001 03 276043 @/var/lib/lxc/1/command
> 0000000000000000: 00000003 00000000 00000000 0001 03 273301 @/var/lib/lxc/0/command
> 0000000000000000: 00000003 00000000 00000000 0001 03 275650 @/var/lib/lxc/4/command
> 
> On this system list_active_containers returns 14 containers while only 10 containers are running.
> 
> Following patch;
> 
> * Introduces array_contains function to do a binary search on given array,
> * Starts to sort arrays inside the add_to_clist and add_to_names functions,
> * Consumes array_contains in list_active_containers to eliminate duplicates,
> * Replaces the linear search code in lxcapi_get_interfaces with the new function.

Thanks - that patch on the whole is good, except that you move the
adding to *names in list_active_containers() to after the attempt to
load the container.  Not loading the container if a container list is
not passed in is deliberately done to avoid a potentially large
amount of work.  (The very low potential for differing results when
passing in *cret and not is deemed worthwhile)

> Signed-off-by: S.Çağlar Onur <caglar at 10ur.org>
> ---
>  src/lxc/lxccontainer.c | 161 ++++++++++++++++++++++++++-----------------------
>  1 file changed, 86 insertions(+), 75 deletions(-)
> 
> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> index c8ecef3..46389ab 100644
> --- a/src/lxc/lxccontainer.c
> +++ b/src/lxc/lxccontainer.c
> @@ -1240,12 +1240,67 @@ out:
>  	return false;
>  }
>  
> +// used by qsort and bsearch functions
> +static inline int string_cmp(char **first, char **second)
> +{
> +	return strcmp(*first, *second);
> +}
> +
> +static bool add_to_names(char ***names, char *cname, int pos)
> +{
> +	char **newnames = realloc(*names, (pos+1) * sizeof(char *));
> +	if (!newnames) {
> +		ERROR("Out of memory");
> +		return false;
> +	}
> +
> +	*names = newnames;
> +	newnames[pos] = strdup(cname);
> +	if (!newnames[pos])
> +		return false;
> +
> +	// sort the arrray as we will use binary search on it
> +	qsort(newnames, pos + 1, sizeof(char *), (int (*)(const void *,const void *))string_cmp);
> +
> +	return true;
> +}
> +
> +// used by qsort and bsearch functions
> +static inline int container_cmp(struct lxc_container **first, struct lxc_container **second)
> +{
> +	return strcmp((*first)->name, (*second)->name);
> +}
> +
> +static bool add_to_clist(struct lxc_container ***list, struct lxc_container *c, int pos)
> +{
> +	struct lxc_container **newlist = realloc(*list, (pos+1) * sizeof(struct lxc_container *));
> +	if (!newlist) {
> +		ERROR("Out of memory");
> +		return false;
> +	}
> +
> +	*list = newlist;
> +	newlist[pos] = c;
> +
> +	// sort the arrray as we will use binary search on it
> +	qsort(newlist, pos + 1, sizeof(struct lxc_container *), (int (*)(const void *,const void *))container_cmp);
> +
> +	return true;
> +}
> +
> +static bool array_contains(char **names, char *cname, int size) {
> +	char *result = NULL;
> +	result = (char *)bsearch((char *)&cname, (char *)names, size, sizeof(char *), (int (*)(const void *,const void *))string_cmp);
> +	if (result != NULL)
> +		return true;
> +	return false;
> +}
> +
>  static char** lxcapi_get_interfaces(struct lxc_container *c)
>  {
> -	int count = 0, i;
> -	bool found = false;
> +	int count = 0;
>  	struct ifaddrs *interfaceArray = NULL, *tempIfAddr = NULL;
> -	char **interfaces = NULL, **temp;
> +	char **interfaces = NULL;
>  	int old_netns = -1, new_netns = -1;
>  
>  	if (!enter_to_ns(c, &old_netns, &new_netns))
> @@ -1259,30 +1314,10 @@ static char** lxcapi_get_interfaces(struct lxc_container *c)
>  
>  	/* Iterate through the interfaces */
>  	for (tempIfAddr = interfaceArray; tempIfAddr != NULL; tempIfAddr = tempIfAddr->ifa_next) {
> -		/*
> -		 * WARNING: Following "for" loop does a linear search over the interfaces array
> -		 * For the containers with lots of interfaces this may be problematic.
> -		 * I'm not expecting this to be the common usage but if it turns out to be
> -		 *     than using binary search or a hash table could be more elegant solution.
> -		 */
> -		for (i = 0; i < count; i++) {
> -			if (strcmp(interfaces[i], tempIfAddr->ifa_name) == 0) {
> -				found = true;
> -				break;
> -			}
> -		}
> -
> -		if (!found) {
> -			count += 1;
> -			temp = realloc(interfaces, count * sizeof(*interfaces));
> -			if (!temp) {
> -				count -= 1;
> -				goto out;
> -			}
> -			interfaces = temp;
> -			interfaces[count - 1] = strdup(tempIfAddr->ifa_name);
> +		if (!array_contains(interfaces, tempIfAddr->ifa_name, count)) {
> +			add_to_names(&interfaces, tempIfAddr->ifa_name, count);
> +			count++;
>  		}
> -		found = false;
>      }
>  
>  out:
> @@ -2796,34 +2831,6 @@ int lxc_get_wait_states(const char **states)
>  	return MAX_STATE;
>  }
>  
> -
> -static bool add_to_names(char ***names, char *cname, int pos)
> -{
> -	char **newnames = realloc(*names, (pos+1) * sizeof(char *));
> -	if (!newnames) {
> -		ERROR("Out of memory");
> -		return false;
> -	}
> -	*names = newnames;
> -	newnames[pos] = strdup(cname);
> -	if (!newnames[pos])
> -		return false;
> -	return true;
> -}
> -
> -static bool add_to_clist(struct lxc_container ***list, struct lxc_container *c, int pos)
> -{
> -	struct lxc_container **newlist = realloc(*list, (pos+1) * sizeof(struct lxc_container *));
> -	if (!newlist) {
> -		ERROR("Out of memory");
> -		return false;
> -	}
> -
> -	*list = newlist;
> -	newlist[pos] = c;
> -	return true;
> -}
> -
>  /*
>   * These next two could probably be done smarter with reusing a common function
>   * with different iterators and tests...
> @@ -2922,9 +2929,10 @@ free_bad:
>  
>  int list_active_containers(const char *lxcpath, char ***names, struct lxc_container ***cret)
>  {
> -	int i, cfound = 0, nfound = 0;
> +	int i, found = 0;
>  	int lxcpath_len;
>  	char *line = NULL;
> +	char **unique_names = NULL;
>  	size_t len = 0;
>  	struct lxc_container *c;
>  
> @@ -2963,52 +2971,55 @@ int list_active_containers(const char *lxcpath, char ***names, struct lxc_contai
>  			continue;
>  		*p2 = '\0';
>  
> -		if (names) {
> -			if (!add_to_names(names, p, nfound))
> -				goto free_bad;
> -		}
> -		cfound++;
> -
> -		if (!cret) {
> -			nfound++;
> +		/*
> +		 * /proc/net/unix may contain multiple entries for command socket 
> +		 * Check the existence before adding to the list
> +		 */
> +		if (array_contains(unique_names, p, found)) {
> +			INFO("Container %s:%s is seen before, skipping", lxcpath, p);
>  			continue;
>  		}
>  
> +		// try loading the container
>  		c = lxc_container_new(p, lxcpath);
>  		if (!c) {
> -			INFO("Container %s:%s is running but could not be loaded",
> -				lxcpath, p);
> -			if (names)
> -				free((*names)[cfound--]);
> +			INFO("Container %s:%s is running but could not be loaded, skipping", lxcpath, p);
>  			continue;
>  		}
>  
> +		// all tests passed, we now can add this to names list
> +		if(!add_to_names(&unique_names, p, found))
> +			goto free_bad;
> +
>  		/*
>  		 * If this is an anonymous container, then is_defined *can*
>  		 * return false.  So we don't do that check.  Count on the
>  		 * fact that the command socket exists.
>  		 */
>  
> -		if (!add_to_clist(cret, c, nfound)) {
> +		if (cret && !add_to_clist(cret, c, found)) {
>  			lxc_container_put(c);
>  			goto free_bad;
>  		}
> -		nfound++;
> +		found++;
>  	}
>  
> +	if (names)
> +		*names = unique_names;
> +
>  	process_lock();
>  	fclose(f);
>  	process_unlock();
> -	return nfound;
> +	return found;
>  
>  free_bad:
> -	if (names && *names) {
> -		for (i=0; i<cfound; i++)
> -			free((*names)[i]);
> -		free(*names);
> +	if(unique_names) {
> +		for (i=0; i<found; i++)
> +			free(unique_names[i]);
> +		free(unique_names);
>  	}
>  	if (cret && *cret) {
> -		for (i=0; i<nfound; i++)
> +		for (i=0; i<found; i++)
>  			lxc_container_put((*cret)[i]);
>  		free(*cret);
>  	}
> -- 
> 1.8.3.2
> 
> 
> ------------------------------------------------------------------------------
> October Webinars: Code for Performance
> Free Intel webinars can help you accelerate application performance.
> Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from 
> the latest Intel processors and coprocessors. See abstracts and register >
> http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk
> _______________________________________________
> Lxc-devel mailing list
> Lxc-devel at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/lxc-devel




More information about the lxc-devel mailing list