On Fri, Sep 13, 2013 at 06:44:34PM -0400, S.Çağlar Onur wrote: > Hey Stéphane, > > On Fri, Sep 13, 2013 at 6:32 PM, Stéphane Graber <stgra...@ubuntu.com>wrote: > > > On Fri, Sep 13, 2013 at 06:21:20PM -0400, S.Çağlar Onur wrote: > > > Signed-off-by: S.Çağlar Onur <cag...@10ur.org> > > > > The loopback filtering was there so that all the software using get_ips > > to wait for the container to be reachable would work. > > Your change will essentially force everyone to do two calls, one for all > > interfaces and one just for lo, then substract the second set from the > > first, that seems suboptimal. > > > > I guess your plan is that most API users will iterate through > > get_interfaces then call get_ips for the interfaces they want, though > > that's just making things harder for little benefit. > > > > > > So I see two ways not to regress in that regard: > > 1) Make scope filtering work with IPv4, so that get_ips() user can pass > > scope=0 and only get global IPv4 and IPv6 addresses. > > 2) Add a no_loopback argument to get_ips. > > > > To be honest I removed filtering just because I thought get_ips and > get_interfaces should return everything in the name of the completeness not > because I saw something wrong with them. For that reason I can add > filtering back to get_ips and start to filter loopback on get_interfaces. > > My real question is what do you think having a get_interfaces method in the > API? I added that cause there was no way to get interface names from the > container (and get_ips accepts an interface parameter).
I have no problem with the added get_interfaces() and I think it's going
to be quite useful for any tools that want to show the list of IPs as a
dictionary (interface_name => list-of-ips).
My Nack was only because of the regressions the loopback change would
cause (I know I have at least a dozen scripts that rely on get_ips() not
returning anything until a non-local IP appears).
>
>
> > As it's:
> > Nacked-by: Stéphane Graber <stgra...@ubuntu.com>
> >
> > > ---
> > > src/lxc/lxccontainer.c | 119
> > +++++++++++++++++++++++++++++++++++++++----------
> > > src/lxc/lxccontainer.h | 1 +
> > > 2 files changed, 97 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> > > index 79237df..14b6942 100644
> > > --- a/src/lxc/lxccontainer.c
> > > +++ b/src/lxc/lxccontainer.c
> > > @@ -1179,23 +1179,26 @@ static bool lxcapi_clear_config_item(struct
> > lxc_container *c, const char *key)
> > > return ret == 0;
> > > }
> > >
> > > -char** lxcapi_get_ips(struct lxc_container *c, char* interface, char*
> > family, int scope)
> > > -{
> > > - int count = 0;
> > > - struct ifaddrs *interfaceArray = NULL, *tempIfAddr = NULL;
> > > - char addressOutputBuffer[INET6_ADDRSTRLEN];
> > > - void *tempAddrPtr = NULL;
> > > - char **addresses = NULL, **temp;
> > > - char *address = NULL;
> > > +static void exit_from_ns(struct lxc_container *c, int *old_netns, int
> > *new_netns) {
> > > + /* Switch back to original netns */
> > > + if (*old_netns >= 0 && setns(*old_netns, CLONE_NEWNET))
> > > + SYSERROR("failed to setns");
> > > + if (*new_netns >= 0)
> > > + close(*new_netns);
> > > + if (*old_netns >= 0)
> > > + close(*old_netns);
> > > +}
> > > +
> > > +static bool enter_to_ns(struct lxc_container *c, int *old_netns, int
> > *new_netns) {
> > > + int ret = 0;
> > > char new_netns_path[MAXPATHLEN];
> > > - int old_netns = -1, new_netns = -1, ret = 0;
> > >
> > > if (!c->is_running(c))
> > > goto out;
> > >
> > > /* Save reference to old netns */
> > > - old_netns = open("/proc/self/ns/net", O_RDONLY);
> > > - if (old_netns < 0) {
> > > + *old_netns = open("/proc/self/ns/net", O_RDONLY);
> > > + if (*old_netns < 0) {
> > > SYSERROR("failed to open /proc/self/ns/net");
> > > goto out;
> > > }
> > > @@ -1205,16 +1208,93 @@ char** lxcapi_get_ips(struct lxc_container *c,
> > char* interface, char* family, in
> > > if (ret < 0 || ret >= MAXPATHLEN)
> > > goto out;
> > >
> > > - new_netns = open(new_netns_path, O_RDONLY);
> > > - if (new_netns < 0) {
> > > + *new_netns = open(new_netns_path, O_RDONLY);
> > > + if (*new_netns < 0) {
> > > SYSERROR("failed to open %s", new_netns_path);
> > > goto out;
> > > }
> > >
> > > - if (setns(new_netns, CLONE_NEWNET)) {
> > > + if (setns(*new_netns, CLONE_NEWNET)) {
> > > SYSERROR("failed to setns");
> > > goto out;
> > > }
> > > + return true;
> > > +out:
> > > + exit_from_ns(c, old_netns, new_netns);
> > > + return false;
> > > +}
> > > +
> > > +char** lxcapi_get_interfaces(struct lxc_container *c)
> > > +{
> > > + int count = 0;
> > > + struct ifaddrs *interfaceArray = NULL, *tempIfAddr = NULL;
> > > + char **interfaces = NULL, **temp;
> > > + int old_netns = -1, new_netns = -1;
> > > +
> > > + if (!enter_to_ns(c, &old_netns, &new_netns))
> > > + goto out;
> > > +
> > > + /* Grab the list of interfaces */
> > > + if (getifaddrs(&interfaceArray)) {
> > > + SYSERROR("failed to get interfaces list");
> > > + goto out;
> > > + }
> > > +
> > > + /* Iterate through the interfaces */
> > > + for (tempIfAddr = interfaceArray; tempIfAddr != NULL; tempIfAddr =
> > tempIfAddr->ifa_next) {
> > > + if (tempIfAddr->ifa_addr == NULL)
> > > + continue;
> > > +
> > > + if(tempIfAddr->ifa_addr->sa_family != AF_PACKET) {
> > > + continue;
> > > + }
> > > +
> > > + count += 1;
> > > + temp = realloc(interfaces, count * sizeof(*interfaces));
> > > + if (!temp) {
> > > + count -= 1;
> > > + goto out;
> > > + }
> > > + interfaces = temp;
> > > + interfaces[count - 1] = strdup(tempIfAddr->ifa_name);
> > > + }
> > > +
> > > +out:
> > > + if(interfaceArray)
> > > + freeifaddrs(interfaceArray);
> > > +
> > > + exit_from_ns(c, &old_netns, &new_netns);
> > > +
> > > + /* Append NULL to the array */
> > > + if (count) {
> > > + count++;
> > > + temp = realloc(interfaces, count * sizeof(*interfaces));
> > > + if (!temp) {
> > > + int i;
> > > + for (i = 0; i < count-1; i++)
> > > + free(interfaces[i]);
> > > + free(interfaces);
> > > + return NULL;
> > > + }
> > > + interfaces = temp;
> > > + interfaces[count - 1] = NULL;
> > > + }
> > > +
> > > + return interfaces;
> > > +}
> > > +
> > > +char** lxcapi_get_ips(struct lxc_container *c, char* interface, char*
> > family, int scope)
> > > +{
> > > + int count = 0;
> > > + struct ifaddrs *interfaceArray = NULL, *tempIfAddr = NULL;
> > > + char addressOutputBuffer[INET6_ADDRSTRLEN];
> > > + void *tempAddrPtr = NULL;
> > > + char **addresses = NULL, **temp;
> > > + char *address = NULL;
> > > + int old_netns = -1, new_netns = -1;
> > > +
> > > + if (!enter_to_ns(c, &old_netns, &new_netns))
> > > + goto out;
> > >
> > > /* Grab the list of interfaces */
> > > if (getifaddrs(&interfaceArray)) {
> > > @@ -1241,8 +1321,6 @@ char** lxcapi_get_ips(struct lxc_container *c,
> > char* interface, char* family, in
> > >
> > > if (interface && strcmp(interface, tempIfAddr->ifa_name))
> > > continue;
> > > - else if (!interface && strcmp("lo", tempIfAddr->ifa_name)
> > == 0)
> > > - continue;
> > >
> > > address = (char
> > *)inet_ntop(tempIfAddr->ifa_addr->sa_family,
> > > tempAddrPtr,
> > > @@ -1265,13 +1343,7 @@ out:
> > > if(interfaceArray)
> > > freeifaddrs(interfaceArray);
> > >
> > > - /* Switch back to original netns */
> > > - if (old_netns >= 0 && setns(old_netns, CLONE_NEWNET))
> > > - SYSERROR("failed to setns");
> > > - if (new_netns >= 0)
> > > - close(new_netns);
> > > - if (old_netns >= 0)
> > > - close(old_netns);
> > > + exit_from_ns(c, &old_netns, &new_netns);
> > >
> > > /* Append NULL to the array */
> > > if (count) {
> > > @@ -2576,6 +2648,7 @@ struct lxc_container *lxc_container_new(const char
> > *name, const char *configpath
> > > c->get_config_path = lxcapi_get_config_path;
> > > c->set_config_path = lxcapi_set_config_path;
> > > c->clone = lxcapi_clone;
> > > + c->get_interfaces = lxcapi_get_interfaces;
> > > c->get_ips = lxcapi_get_ips;
> > > c->attach = lxcapi_attach;
> > > c->attach_run_wait = lxcapi_attach_run_wait;
> > > diff --git a/src/lxc/lxccontainer.h b/src/lxc/lxccontainer.h
> > > index f9ae43b..f17d975 100644
> > > --- a/src/lxc/lxccontainer.h
> > > +++ b/src/lxc/lxccontainer.h
> > > @@ -90,6 +90,7 @@ struct lxc_container {
> > > * the length which was our would be printed. */
> > > int (*get_config_item)(struct lxc_container *c, const char *key,
> > char *retv, int inlen);
> > > int (*get_keys)(struct lxc_container *c, const char *key, char
> > *retv, int inlen);
> > > + char** (*get_interfaces)(struct lxc_container *c);
> > > char** (*get_ips)(struct lxc_container *c, char* interface, char*
> > family, int scope);
> > > /*
> > > * get_cgroup_item returns the number of bytes read, or an error
> > (<0).
> > > --
> > > 1.8.1.2
> > >
> > >
> > >
> > ------------------------------------------------------------------------------
> > > How ServiceNow helps IT people transform IT departments:
> > > 1. Consolidate legacy IT systems to a single system of record for IT
> > > 2. Standardize and globalize service processes across IT
> > > 3. Implement zero-touch automation to replace manual, redundant tasks
> > >
> > http://pubads.g.doubleclick.net/gampad/clk?id=51271111&iu=/4140/ostg.clktrk
> > > _______________________________________________
> > > Lxc-devel mailing list
> > > Lxc-devel@lists.sourceforge.net
> > > https://lists.sourceforge.net/lists/listinfo/lxc-devel
> >
> > --
> > Stéphane Graber
> > Ubuntu developer
> > http://www.ubuntu.com
> >
>
>
>
> --
> S.Çağlar Onur <cag...@10ur.org>
--
Stéphane Graber
Ubuntu developer
http://www.ubuntu.com
signature.asc
Description: Digital signature
------------------------------------------------------------------------------ How ServiceNow helps IT people transform IT departments: 1. Consolidate legacy IT systems to a single system of record for IT 2. Standardize and globalize service processes across IT 3. Implement zero-touch automation to replace manual, redundant tasks http://pubads.g.doubleclick.net/gampad/clk?id=51271111&iu=/4140/ostg.clktrk
_______________________________________________ Lxc-devel mailing list Lxc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lxc-devel
