<div dir="ltr">Hi,<div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Oct 21, 2013 at 8:22 PM, Serge Hallyn <span dir="ltr"><<a href="mailto:serge.hallyn@ubuntu.com" target="_blank">serge.hallyn@ubuntu.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">Quoting S.Çağlar Onur (<a href="mailto:caglar@10ur.org">caglar@10ur.org</a>):<br>
> list_active_containers parses /proc/net/unix which can contain multiple entries for the same container;<br>
><br>
> 0000000000000000: 00000002 00000000 00010000 0001 01 273672 @/var/lib/lxc/6/command<br>
> 0000000000000000: 00000002 00000000 00010000 0001 01 274395 @/var/lib/lxc/5/command<br>
> 0000000000000000: 00000002 00000000 00010000 0001 01 273890 @/var/lib/lxc/4/command<br>
> 0000000000000000: 00000002 00000000 00010000 0001 01 273141 @/var/lib/lxc/3/command<br>
> 0000000000000000: 00000002 00000000 00010000 0001 01 273915 @/var/lib/lxc/2/command<br>
> 0000000000000000: 00000002 00000000 00010000 0001 01 273683 @/var/lib/lxc/1/command<br>
> 0000000000000000: 00000002 00000000 00010000 0001 01 273074 @/var/lib/lxc/0/command<br>
> 0000000000000000: 00000002 00000000 00010000 0001 01 273931 @/var/lib/lxc/9/command<br>
> 0000000000000000: 00000002 00000000 00010000 0001 01 273110 @/var/lib/lxc/8/command<br>
> 0000000000000000: 00000002 00000000 00010000 0001 01 273390 @/var/lib/lxc/7/command<br>
> 0000000000000000: 00000003 00000000 00000000 0001 03 275903 @/var/lib/lxc/8/command<br>
> 0000000000000000: 00000003 00000000 00000000 0001 03 276043 @/var/lib/lxc/1/command<br>
> 0000000000000000: 00000003 00000000 00000000 0001 03 273301 @/var/lib/lxc/0/command<br>
> 0000000000000000: 00000003 00000000 00000000 0001 03 275650 @/var/lib/lxc/4/command<br>
><br>
> On this system list_active_containers returns 14 containers while only 10 containers are running.<br>
><br>
> Following patch;<br>
><br>
> * Introduces array_contains function to do a binary search on given array,<br>
> * Starts to sort arrays inside the add_to_clist and add_to_names functions,<br>
> * Consumes array_contains in list_active_containers to eliminate duplicates,<br>
> * Replaces the linear search code in lxcapi_get_interfaces with the new function.<br>
<br>
</div>Thanks - that patch on the whole is good, except that you move the<br>
adding to *names in list_active_containers() to after the attempt to<br>
load the container. Not loading the container if a container list is<br>
not passed in is deliberately done to avoid a potentially large<br>
amount of work. (The very low potential for differing results when<br>
passing in *cret and not is deemed worthwhile)</blockquote><div><br></div><div>OK, I was just trying to make sure both cases return same result :) I'll iterate one more time with that change.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5">
> Signed-off-by: S.Çağlar Onur <<a href="mailto:caglar@10ur.org">caglar@10ur.org</a>><br>
> ---<br>
> src/lxc/lxccontainer.c | 161 ++++++++++++++++++++++++++-----------------------<br>
> 1 file changed, 86 insertions(+), 75 deletions(-)<br>
><br>
> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c<br>
> index c8ecef3..46389ab 100644<br>
> --- a/src/lxc/lxccontainer.c<br>
> +++ b/src/lxc/lxccontainer.c<br>
> @@ -1240,12 +1240,67 @@ out:<br>
> return false;<br>
> }<br>
><br>
> +// used by qsort and bsearch functions<br>
> +static inline int string_cmp(char **first, char **second)<br>
> +{<br>
> + return strcmp(*first, *second);<br>
> +}<br>
> +<br>
> +static bool add_to_names(char ***names, char *cname, int pos)<br>
> +{<br>
> + char **newnames = realloc(*names, (pos+1) * sizeof(char *));<br>
> + if (!newnames) {<br>
> + ERROR("Out of memory");<br>
> + return false;<br>
> + }<br>
> +<br>
> + *names = newnames;<br>
> + newnames[pos] = strdup(cname);<br>
> + if (!newnames[pos])<br>
> + return false;<br>
> +<br>
> + // sort the arrray as we will use binary search on it<br>
> + qsort(newnames, pos + 1, sizeof(char *), (int (*)(const void *,const void *))string_cmp);<br>
> +<br>
> + return true;<br>
> +}<br>
> +<br>
> +// used by qsort and bsearch functions<br>
> +static inline int container_cmp(struct lxc_container **first, struct lxc_container **second)<br>
> +{<br>
> + return strcmp((*first)->name, (*second)->name);<br>
> +}<br>
> +<br>
> +static bool add_to_clist(struct lxc_container ***list, struct lxc_container *c, int pos)<br>
> +{<br>
> + struct lxc_container **newlist = realloc(*list, (pos+1) * sizeof(struct lxc_container *));<br>
> + if (!newlist) {<br>
> + ERROR("Out of memory");<br>
> + return false;<br>
> + }<br>
> +<br>
> + *list = newlist;<br>
> + newlist[pos] = c;<br>
> +<br>
> + // sort the arrray as we will use binary search on it<br>
> + qsort(newlist, pos + 1, sizeof(struct lxc_container *), (int (*)(const void *,const void *))container_cmp);<br>
> +<br>
> + return true;<br>
> +}<br>
> +<br>
> +static bool array_contains(char **names, char *cname, int size) {<br>
> + char *result = NULL;<br>
> + result = (char *)bsearch((char *)&cname, (char *)names, size, sizeof(char *), (int (*)(const void *,const void *))string_cmp);<br>
> + if (result != NULL)<br>
> + return true;<br>
> + return false;<br>
> +}<br>
> +<br>
> static char** lxcapi_get_interfaces(struct lxc_container *c)<br>
> {<br>
> - int count = 0, i;<br>
> - bool found = false;<br>
> + int count = 0;<br>
> struct ifaddrs *interfaceArray = NULL, *tempIfAddr = NULL;<br>
> - char **interfaces = NULL, **temp;<br>
> + char **interfaces = NULL;<br>
> int old_netns = -1, new_netns = -1;<br>
><br>
> if (!enter_to_ns(c, &old_netns, &new_netns))<br>
> @@ -1259,30 +1314,10 @@ static char** lxcapi_get_interfaces(struct lxc_container *c)<br>
><br>
> /* Iterate through the interfaces */<br>
> for (tempIfAddr = interfaceArray; tempIfAddr != NULL; tempIfAddr = tempIfAddr->ifa_next) {<br>
> - /*<br>
> - * WARNING: Following "for" loop does a linear search over the interfaces array<br>
> - * For the containers with lots of interfaces this may be problematic.<br>
> - * I'm not expecting this to be the common usage but if it turns out to be<br>
> - * than using binary search or a hash table could be more elegant solution.<br>
> - */<br>
> - for (i = 0; i < count; i++) {<br>
> - if (strcmp(interfaces[i], tempIfAddr->ifa_name) == 0) {<br>
> - found = true;<br>
> - break;<br>
> - }<br>
> - }<br>
> -<br>
> - if (!found) {<br>
> - count += 1;<br>
> - temp = realloc(interfaces, count * sizeof(*interfaces));<br>
> - if (!temp) {<br>
> - count -= 1;<br>
> - goto out;<br>
> - }<br>
> - interfaces = temp;<br>
> - interfaces[count - 1] = strdup(tempIfAddr->ifa_name);<br>
> + if (!array_contains(interfaces, tempIfAddr->ifa_name, count)) {<br>
> + add_to_names(&interfaces, tempIfAddr->ifa_name, count);<br>
> + count++;<br>
> }<br>
> - found = false;<br>
> }<br>
><br>
> out:<br>
> @@ -2796,34 +2831,6 @@ int lxc_get_wait_states(const char **states)<br>
> return MAX_STATE;<br>
> }<br>
><br>
> -<br>
> -static bool add_to_names(char ***names, char *cname, int pos)<br>
> -{<br>
> - char **newnames = realloc(*names, (pos+1) * sizeof(char *));<br>
> - if (!newnames) {<br>
> - ERROR("Out of memory");<br>
> - return false;<br>
> - }<br>
> - *names = newnames;<br>
> - newnames[pos] = strdup(cname);<br>
> - if (!newnames[pos])<br>
> - return false;<br>
> - return true;<br>
> -}<br>
> -<br>
> -static bool add_to_clist(struct lxc_container ***list, struct lxc_container *c, int pos)<br>
> -{<br>
> - struct lxc_container **newlist = realloc(*list, (pos+1) * sizeof(struct lxc_container *));<br>
> - if (!newlist) {<br>
> - ERROR("Out of memory");<br>
> - return false;<br>
> - }<br>
> -<br>
> - *list = newlist;<br>
> - newlist[pos] = c;<br>
> - return true;<br>
> -}<br>
> -<br>
> /*<br>
> * These next two could probably be done smarter with reusing a common function<br>
> * with different iterators and tests...<br>
> @@ -2922,9 +2929,10 @@ free_bad:<br>
><br>
> int list_active_containers(const char *lxcpath, char ***names, struct lxc_container ***cret)<br>
> {<br>
> - int i, cfound = 0, nfound = 0;<br>
> + int i, found = 0;<br>
> int lxcpath_len;<br>
> char *line = NULL;<br>
> + char **unique_names = NULL;<br>
> size_t len = 0;<br>
> struct lxc_container *c;<br>
><br>
> @@ -2963,52 +2971,55 @@ int list_active_containers(const char *lxcpath, char ***names, struct lxc_contai<br>
> continue;<br>
> *p2 = '\0';<br>
><br>
> - if (names) {<br>
> - if (!add_to_names(names, p, nfound))<br>
> - goto free_bad;<br>
> - }<br>
> - cfound++;<br>
> -<br>
> - if (!cret) {<br>
> - nfound++;<br>
> + /*<br>
> + * /proc/net/unix may contain multiple entries for command socket<br>
> + * Check the existence before adding to the list<br>
> + */<br>
> + if (array_contains(unique_names, p, found)) {<br>
> + INFO("Container %s:%s is seen before, skipping", lxcpath, p);<br>
> continue;<br>
> }<br>
><br>
> + // try loading the container<br>
> c = lxc_container_new(p, lxcpath);<br>
> if (!c) {<br>
> - INFO("Container %s:%s is running but could not be loaded",<br>
> - lxcpath, p);<br>
> - if (names)<br>
> - free((*names)[cfound--]);<br>
> + INFO("Container %s:%s is running but could not be loaded, skipping", lxcpath, p);<br>
> continue;<br>
> }<br>
><br>
> + // all tests passed, we now can add this to names list<br>
> + if(!add_to_names(&unique_names, p, found))<br>
> + goto free_bad;<br>
> +<br>
> /*<br>
> * If this is an anonymous container, then is_defined *can*<br>
> * return false. So we don't do that check. Count on the<br>
> * fact that the command socket exists.<br>
> */<br>
><br>
> - if (!add_to_clist(cret, c, nfound)) {<br>
> + if (cret && !add_to_clist(cret, c, found)) {<br>
> lxc_container_put(c);<br>
> goto free_bad;<br>
> }<br>
> - nfound++;<br>
> + found++;<br>
> }<br>
><br>
> + if (names)<br>
> + *names = unique_names;<br>
> +<br>
> process_lock();<br>
> fclose(f);<br>
> process_unlock();<br>
> - return nfound;<br>
> + return found;<br>
><br>
> free_bad:<br>
> - if (names && *names) {<br>
> - for (i=0; i<cfound; i++)<br>
> - free((*names)[i]);<br>
> - free(*names);<br>
> + if(unique_names) {<br>
> + for (i=0; i<found; i++)<br>
> + free(unique_names[i]);<br>
> + free(unique_names);<br>
> }<br>
> if (cret && *cret) {<br>
> - for (i=0; i<nfound; i++)<br>
> + for (i=0; i<found; i++)<br>
> lxc_container_put((*cret)[i]);<br>
> free(*cret);<br>
> }<br>
> --<br>
> 1.8.3.2<br>
><br>
><br>
</div></div>> ------------------------------------------------------------------------------<br>
> October Webinars: Code for Performance<br>
> Free Intel webinars can help you accelerate application performance.<br>
> Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from<br>
> the latest Intel processors and coprocessors. See abstracts and register ><br>
> <a href="http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk" target="_blank">http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk</a><br>
> _______________________________________________<br>
> Lxc-devel mailing list<br>
> <a href="mailto:Lxc-devel@lists.sourceforge.net">Lxc-devel@lists.sourceforge.net</a><br>
> <a href="https://lists.sourceforge.net/lists/listinfo/lxc-devel" target="_blank">https://lists.sourceforge.net/lists/listinfo/lxc-devel</a><br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br>S.Çağlar Onur <<a href="mailto:caglar@10ur.org">caglar@10ur.org</a>>
</div></div>