<div dir="ltr">Hi <span style="color:rgb(80,0,80)">Stéphane,</span><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Sep 16, 2013 at 3:04 PM, Stéphane Graber <span dir="ltr"><<a href="mailto:stgraber@ubuntu.com" target="_blank">stgraber@ubuntu.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">On Mon, Sep 16, 2013 at 02:54:44PM -0400, S.Çağlar Onur wrote:<br>
> On Mon, Sep 16, 2013 at 2:39 PM, Stéphane Graber <<a href="mailto:stgraber@ubuntu.com">stgraber@ubuntu.com</a>>wrote:<br>
><br>
> > On Mon, Sep 16, 2013 at 02:26:47PM -0400, S.Çağlar Onur wrote:<br>
> > > get_ips accepts an interface name as a parameter but there was no<br>
> > > way to get the interfaces names from the container. This patch<br>
> > > introduces a new get_interfaces call to the API so that users<br>
> > > can obtain the name of the interfaces.<br>
> > ><br>
> > > Signed-off-by: S.Çağlar Onur <<a href="mailto:caglar@10ur.org">caglar@10ur.org</a>><br>
> ><br>
> > I think I'm fine with this version:<br>
> ><br>
> > One quick comment though, wouldn't we expect get_interfaces() to return<br>
> > all the interfaces by default, including loopback, including those<br>
> > without addresses and those that aren't standard packet based IP<br>
> > interfaces?<br>
> ><br>
> > I still think we don't want to list loopback addresses in get_ips(), at<br>
> > least by default, but I think I'd expect get_interfaces to return me all<br>
> > the existing interfaces without filtering. Does that make sense?<br>
><br>
><br>
> Guess so. I've no objection including loopback in get_interfaces. I<br>
> excluded it thinking that there is no point returning it as get_ips will<br>
> ignore it. What about something like below as a replacement to v2 (this<br>
> version starts to use if_nameindex instead of getifaddrs)?<br>
<br>
</div>I'd rather not use if_nameindex unless someone re-implements a bionic<br>
version of it under lxc/includes/ as I just confirmed it doesn't exist<br>
on bionic and so commiting that change would break our Android builds (I<br>
know bionic is a real pain).<br>
<br>
It took me a couple of days to find an slightly adapt a reasonable<br>
implementation of getifaddrs for bionic, so I'd like not to have to go<br>
through that again (but don't mind it if someone else contributes a<br>
bionic compatible implementation).<br></blockquote><div><br></div><div>Oh right, I completely forget Android. I'll stick to getifaddrs</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
So just to clarify what I think should happen in a perfect worls :)<br>
1) get_interfaces() would always return all the network interfaces<br>
visible by the guest, so I'd expect those to match the entries from<br>
ifconfig -a (or ip link show)<br>
2) get_ips() should default to only listing global IP addresses, so by<br>
default ignore the loopback device and only return scope-global for<br>
IPv6 (its current behaviour)<br>
3) If specifically passed interface="lo", then I'd expect get_ips to<br>
return the loopback IP addresses (not currently the case, but<br>
probably should be the case)<br>
<br>
Does that make sense?</blockquote><div><br></div><div>Yes, it does. I'll work on these and iterate the patch one more time.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><div class="h5">
><br>
> +static char** lxcapi_get_interfaces(struct lxc_container *c)<br>
> +{<br>
> + int count = 0;<br>
> + struct if_nameindex *if_ni = NULL, *i;<br>
> + char **interfaces = NULL, **temp;<br>
> + int old_netns = -1, new_netns = -1;<br>
> +<br>
> + if (!enter_to_ns(c, &old_netns, &new_netns))<br>
> + goto out;<br>
> +<br>
> + if_ni = if_nameindex();<br>
> + if (if_ni == NULL) {<br>
> + SYSERROR("failed to get interfaces list");<br>
> + goto out;<br>
> + }<br>
> +<br>
> + for (i = if_ni; ! (i->if_index == 0 && i->if_name == NULL); i++) {<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(i->if_name);<br>
> + }<br>
> +<br>
> +out:<br>
> + if_freenameindex(if_ni);<br>
> +<br>
> + exit_from_ns(c, &old_netns, &new_netns);<br>
> +<br>
> + /* Append NULL to the array */<br>
> + interfaces = (char **)lxc_append_null_to_array((void **)interfaces,<br>
> count);<br>
> +<br>
> + return interfaces;<br>
> +}<br>
><br>
> And if we need get_ips to return ips then I guess we can always add a flag<br>
> to get_ips as you suggested before?<br>
><br>
> > ---<br>
> > > src/lxc/lxccontainer.c | 117<br>
> > ++++++++++++++++++++++++++++++++++---------------<br>
> > > src/lxc/lxccontainer.h | 1 +<br>
> > > src/lxc/utils.c | 22 +++++++++-<br>
> > > src/lxc/utils.h | 1 +<br>
> > > 4 files changed, 105 insertions(+), 36 deletions(-)<br>
> > ><br>
> > > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c<br>
> > > index 79237df..6aa59fc 100644<br>
> > > --- a/src/lxc/lxccontainer.c<br>
> > > +++ b/src/lxc/lxccontainer.c<br>
> > > @@ -1179,23 +1179,26 @@ static bool lxcapi_clear_config_item(struct<br>
> > lxc_container *c, const char *key)<br>
> > > return ret == 0;<br>
> > > }<br>
> > ><br>
> > > -char** lxcapi_get_ips(struct lxc_container *c, char* interface, char*<br>
> > family, int scope)<br>
> > > -{<br>
> > > - int count = 0;<br>
> > > - struct ifaddrs *interfaceArray = NULL, *tempIfAddr = NULL;<br>
> > > - char addressOutputBuffer[INET6_ADDRSTRLEN];<br>
> > > - void *tempAddrPtr = NULL;<br>
> > > - char **addresses = NULL, **temp;<br>
> > > - char *address = NULL;<br>
> > > +static inline void exit_from_ns(struct lxc_container *c, int<br>
> > *old_netns, int *new_netns) {<br>
> > > + /* Switch back to original netns */<br>
> > > + if (*old_netns >= 0 && setns(*old_netns, CLONE_NEWNET))<br>
> > > + SYSERROR("failed to setns");<br>
> > > + if (*new_netns >= 0)<br>
> > > + close(*new_netns);<br>
> > > + if (*old_netns >= 0)<br>
> > > + close(*old_netns);<br>
> > > +}<br>
> > > +<br>
> > > +static inline bool enter_to_ns(struct lxc_container *c, int *old_netns,<br>
> > int *new_netns) {<br>
> > > + int ret = 0;<br>
> > > char new_netns_path[MAXPATHLEN];<br>
> > > - int old_netns = -1, new_netns = -1, ret = 0;<br>
> > ><br>
> > > if (!c->is_running(c))<br>
> > > goto out;<br>
> > ><br>
> > > /* Save reference to old netns */<br>
> > > - old_netns = open("/proc/self/ns/net", O_RDONLY);<br>
> > > - if (old_netns < 0) {<br>
> > > + *old_netns = open("/proc/self/ns/net", O_RDONLY);<br>
> > > + if (*old_netns < 0) {<br>
> > > SYSERROR("failed to open /proc/self/ns/net");<br>
> > > goto out;<br>
> > > }<br>
> > > @@ -1205,16 +1208,78 @@ char** lxcapi_get_ips(struct lxc_container *c,<br>
> > char* interface, char* family, in<br>
> > > if (ret < 0 || ret >= MAXPATHLEN)<br>
> > > goto out;<br>
> > ><br>
> > > - new_netns = open(new_netns_path, O_RDONLY);<br>
> > > - if (new_netns < 0) {<br>
> > > + *new_netns = open(new_netns_path, O_RDONLY);<br>
> > > + if (*new_netns < 0) {<br>
> > > SYSERROR("failed to open %s", new_netns_path);<br>
> > > goto out;<br>
> > > }<br>
> > ><br>
> > > - if (setns(new_netns, CLONE_NEWNET)) {<br>
> > > + if (setns(*new_netns, CLONE_NEWNET)) {<br>
> > > SYSERROR("failed to setns");<br>
> > > goto out;<br>
> > > }<br>
> > > + return true;<br>
> > > +out:<br>
> > > + exit_from_ns(c, old_netns, new_netns);<br>
> > > + return false;<br>
> > > +}<br>
> > > +<br>
> > > +static char** lxcapi_get_interfaces(struct lxc_container *c)<br>
> > > +{<br>
> > > + int count = 0;<br>
> > > + struct ifaddrs *interfaceArray = NULL, *tempIfAddr = NULL;<br>
> > > + char **interfaces = NULL, **temp;<br>
> > > + int old_netns = -1, new_netns = -1;<br>
> > > +<br>
> > > + if (!enter_to_ns(c, &old_netns, &new_netns))<br>
> > > + goto out;<br>
> > > +<br>
> > > + /* Grab the list of interfaces */<br>
> > > + if (getifaddrs(&interfaceArray)) {<br>
> > > + SYSERROR("failed to get interfaces list");<br>
> > > + goto out;<br>
> > > + }<br>
> > > +<br>
> > > + /* Iterate through the interfaces */<br>
> > > + for (tempIfAddr = interfaceArray; tempIfAddr != NULL; tempIfAddr =<br>
> > tempIfAddr->ifa_next) {<br>
> > > + /* filter out loopback interface */<br>
> > > + if(tempIfAddr->ifa_addr == NULL ||<br>
> > tempIfAddr->ifa_addr->sa_family != AF_PACKET || strcmp("lo",<br>
> > tempIfAddr->ifa_name) == 0)<br>
> > > + continue;<br>
> > > +<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>
> > > + }<br>
> > > +<br>
> > > +out:<br>
> > > + if(interfaceArray)<br>
> > > + freeifaddrs(interfaceArray);<br>
> > > +<br>
> > > + exit_from_ns(c, &old_netns, &new_netns);<br>
> > > +<br>
> > > + /* Append NULL to the array */<br>
> > > + interfaces = (char **)lxc_append_null_to_array((void<br>
> > **)interfaces, count);<br>
> > > +<br>
> > > + return interfaces;<br>
> > > +}<br>
> > > +<br>
> > > +static char** lxcapi_get_ips(struct lxc_container *c, char* interface,<br>
> > char* family, int scope)<br>
> > > +{<br>
> > > + int count = 0;<br>
> > > + struct ifaddrs *interfaceArray = NULL, *tempIfAddr = NULL;<br>
> > > + char addressOutputBuffer[INET6_ADDRSTRLEN];<br>
> > > + void *tempAddrPtr = NULL;<br>
> > > + char **addresses = NULL, **temp;<br>
> > > + char *address = NULL;<br>
> > > + int old_netns = -1, new_netns = -1;<br>
> > > +<br>
> > > + if (!enter_to_ns(c, &old_netns, &new_netns))<br>
> > > + goto out;<br>
> > ><br>
> > > /* Grab the list of interfaces */<br>
> > > if (getifaddrs(&interfaceArray)) {<br>
> > > @@ -1243,7 +1308,6 @@ char** lxcapi_get_ips(struct lxc_container *c,<br>
> > char* interface, char* family, in<br>
> > > continue;<br>
> > > else if (!interface && strcmp("lo", tempIfAddr->ifa_name)<br>
> > == 0)<br>
> > > continue;<br>
> > > -<br>
> > > address = (char<br>
> > *)inet_ntop(tempIfAddr->ifa_addr->sa_family,<br>
> > > tempAddrPtr,<br>
> > > addressOutputBuffer,<br>
> > > @@ -1265,28 +1329,10 @@ out:<br>
> > > if(interfaceArray)<br>
> > > freeifaddrs(interfaceArray);<br>
> > ><br>
> > > - /* Switch back to original netns */<br>
> > > - if (old_netns >= 0 && setns(old_netns, CLONE_NEWNET))<br>
> > > - SYSERROR("failed to setns");<br>
> > > - if (new_netns >= 0)<br>
> > > - close(new_netns);<br>
> > > - if (old_netns >= 0)<br>
> > > - close(old_netns);<br>
> > > + exit_from_ns(c, &old_netns, &new_netns);<br>
> > ><br>
> > > /* Append NULL to the array */<br>
> > > - if (count) {<br>
> > > - count++;<br>
> > > - temp = realloc(addresses, count * sizeof(*addresses));<br>
> > > - if (!temp) {<br>
> > > - int i;<br>
> > > - for (i = 0; i < count-1; i++)<br>
> > > - free(addresses[i]);<br>
> > > - free(addresses);<br>
> > > - return NULL;<br>
> > > - }<br>
> > > - addresses = temp;<br>
> > > - addresses[count - 1] = NULL;<br>
> > > - }<br>
> > > + addresses = (char **)lxc_append_null_to_array((void **)addresses,<br>
> > count);<br>
> > ><br>
> > > return addresses;<br>
> > > }<br>
> > > @@ -2576,6 +2622,7 @@ struct lxc_container *lxc_container_new(const char<br>
> > *name, const char *configpath<br>
> > > c->get_config_path = lxcapi_get_config_path;<br>
> > > c->set_config_path = lxcapi_set_config_path;<br>
> > > c->clone = lxcapi_clone;<br>
> > > + c->get_interfaces = lxcapi_get_interfaces;<br>
> > > c->get_ips = lxcapi_get_ips;<br>
> > > c->attach = lxcapi_attach;<br>
> > > c->attach_run_wait = lxcapi_attach_run_wait;<br>
> > > diff --git a/src/lxc/lxccontainer.h b/src/lxc/lxccontainer.h<br>
> > > index f9ae43b..f17d975 100644<br>
> > > --- a/src/lxc/lxccontainer.h<br>
> > > +++ b/src/lxc/lxccontainer.h<br>
> > > @@ -90,6 +90,7 @@ struct lxc_container {<br>
> > > * the length which was our would be printed. */<br>
> > > int (*get_config_item)(struct lxc_container *c, const char *key,<br>
> > char *retv, int inlen);<br>
> > > int (*get_keys)(struct lxc_container *c, const char *key, char<br>
> > *retv, int inlen);<br>
> > > + char** (*get_interfaces)(struct lxc_container *c);<br>
> > > char** (*get_ips)(struct lxc_container *c, char* interface, char*<br>
> > family, int scope);<br>
> > > /*<br>
> > > * get_cgroup_item returns the number of bytes read, or an error<br>
> > (<0).<br>
> > > diff --git a/src/lxc/utils.c b/src/lxc/utils.c<br>
> > > index 78b234d..924cc19 100644<br>
> > > --- a/src/lxc/utils.c<br>
> > > +++ b/src/lxc/utils.c<br>
> > > @@ -852,7 +852,7 @@ int lxc_write_to_file(const char *filename, const<br>
> > void* buf, size_t count, bool<br>
> > > return -1;<br>
> > > ret = lxc_write_nointr(fd, buf, count);<br>
> > > if (ret < 0)<br>
> > > - goto out_error;<br>
> > > + goto out_error;<br>
> > > if ((size_t)ret != count)<br>
> > > goto out_error;<br>
> > > if (add_newline) {<br>
> > > @@ -899,3 +899,23 @@ int lxc_read_from_file(const char *filename, void*<br>
> > buf, size_t count)<br>
> > > errno = saved_errno;<br>
> > > return ret;<br>
> > > }<br>
> > > +<br>
> > > +void **lxc_append_null_to_array(void **array, size_t count)<br>
> > > +{<br>
> > > + void **temp;<br>
> > > +<br>
> > > + /* Append NULL to the array */<br>
> > > + if (count) {<br>
> > > + temp = realloc(array, (count + 1) * sizeof(*array));<br>
> > > + if (!temp) {<br>
> > > + int i;<br>
> > > + for (i = 0; i < count; i++)<br>
> > > + free(array[i]);<br>
> > > + free(array);<br>
> > > + return NULL;<br>
> > > + }<br>
> > > + array = temp;<br>
> > > + array[count] = NULL;<br>
> > > + }<br>
> > > + return array;<br>
> > > +}<br>
> > > diff --git a/src/lxc/utils.h b/src/lxc/utils.h<br>
> > > index ba7cfb3..55f98fa 100644<br>
> > > --- a/src/lxc/utils.h<br>
> > > +++ b/src/lxc/utils.h<br>
> > > @@ -236,4 +236,5 @@ extern void lxc_free_array(void **array, lxc_free_fn<br>
> > element_free_fn);<br>
> > > extern size_t lxc_array_len(void **array);<br>
> > > extern void **lxc_dup_array(void **array, lxc_dup_fn element_dup_fn,<br>
> > lxc_free_fn element_free_fn);<br>
> > ><br>
> > > +extern void **lxc_append_null_to_array(void **array, size_t count);<br>
> > > #endif<br>
> > > --<br>
> > > 1.8.1.2<br>
> > ><br>
> > ><br>
> > ><br>
> > ------------------------------------------------------------------------------<br>
> > > LIMITED TIME SALE - Full Year of Microsoft Training For Just $49.99!<br>
> > > 1,500+ hours of tutorials including VisualStudio 2012, Windows 8,<br>
> > SharePoint<br>
> > > 2013, SQL 2012, MVC 4, more. BEST VALUE: New Multi-Library Power Pack<br>
> > includes<br>
> > > Mobile, Cloud, Java, and UX Design. Lowest price ever! Ends 9/20/13.<br>
> > ><br>
> > <a href="http://pubads.g.doubleclick.net/gampad/clk?id=58041151&iu=/4140/ostg.clktrk" target="_blank">http://pubads.g.doubleclick.net/gampad/clk?id=58041151&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>
> ><br>
> > --<br>
> > Stéphane Graber<br>
> > Ubuntu developer<br>
> > <a href="http://www.ubuntu.com" target="_blank">http://www.ubuntu.com</a><br>
> ><br>
><br>
><br>
><br>
> --<br>
> S.Çağlar Onur <<a href="mailto:caglar@10ur.org">caglar@10ur.org</a>><br>
<br>
--<br>
Stéphane Graber<br>
Ubuntu developer<br>
<a href="http://www.ubuntu.com" target="_blank">http://www.ubuntu.com</a><br>
</div></div></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>