[lxc-devel] [PATCH 1/1] config: fix the handling of lxc.hook and hwaddrs in unexpanded config
Stéphane Graber
stgraber at ubuntu.com
Fri Sep 19 20:53:13 UTC 2014
On Mon, Sep 01, 2014 at 08:01:20PM +0000, Serge Hallyn wrote:
> And add a testcase.
>
> The code to update hwaddrs in a clone was walking through the container
> configuration and re-printing all network entries. However network
> entries from an include file which should not be printed out were being
> added to the unexpanded config. With this patch, at clone we simply
> update the hwaddr in-place in the unexpanded configuration file, making
> sure to make the same update to the expanded network configuration.
>
> The code to update out lxc.hook statements had the same problem.
> We also update it in-place in the unexpanded configuration, though
> we mirror the logic we use when updating the expanded configuration.
> (Perhaps that should be changed, to simplify future updates)
>
> This code isn't particularly easy to review, so testcases are added
> to make sure that (1) extra lxc.network entries are not added (or
> removed), even if they are present in an included file, (2) lxc.hook
> entries are not added, (3) hwaddr entries are updated, and (4)
> the lxc.hook entries are properly updated (only when they should be).
>
> Reported-by: Stéphane Graber <stgraber at ubuntu.com>
> Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>
I suspect we'll need to add the testcase to extra_DISTS but I'll take
care of that if needed.
Acked-by: Stéphane Graber <stgraber at ubuntu.com>
> ---
> src/lxc/confile.c | 231 +++++++++++++++++++++++++----------------
> src/lxc/confile.h | 5 +-
> src/lxc/lxccontainer.c | 37 +------
> src/tests/Makefile.am | 2 +-
> src/tests/lxc-test-cloneconfig | 139 +++++++++++++++++++++++++
> 5 files changed, 291 insertions(+), 123 deletions(-)
> create mode 100755 src/tests/lxc-test-cloneconfig
>
> diff --git a/src/lxc/confile.c b/src/lxc/confile.c
> index 5de1241..9b1fba8 100644
> --- a/src/lxc/confile.c
> +++ b/src/lxc/confile.c
> @@ -36,6 +36,7 @@
> #include <arpa/inet.h>
> #include <netinet/in.h>
> #include <net/if.h>
> +#include <time.h>
>
> #include "parse.h"
> #include "config.h"
> @@ -2407,16 +2408,84 @@ void clear_unexp_config_line(struct lxc_conf *conf, const char *key, bool rm_sub
> }
> }
>
> -bool clone_update_unexp_hooks(struct lxc_conf *c)
> +bool clone_update_unexp_hooks(struct lxc_conf *conf, const char *oldpath,
> + const char *newpath, const char *oldname, const char *newname)
> {
> - struct lxc_list *it;
> - int i;
> -
> - clear_unexp_config_line(c, "lxc.hook", true);
> - for (i=0; i<NUM_LXC_HOOKS; i++) {
> - lxc_list_for_each(it, &c->hooks[i]) {
> - if (!do_append_unexp_config_line(c, lxchook_names[i], (char *)it->elem))
> + const char *key = "lxc.hook";
> + int ret;
> + char *lstart = conf->unexpanded_config, *lend, *p;
> + size_t newdirlen = strlen(newpath) + strlen(newname) + 1;
> + size_t olddirlen = strlen(oldpath) + strlen(oldname) + 1;
> + char *olddir = alloca(olddirlen + 1);
> + char *newdir = alloca(newdirlen + 1);
> +
> + ret = snprintf(olddir, olddirlen+1, "%s/%s", oldpath, oldname);
> + if (ret < 0 || ret >= olddirlen+1) {
> + ERROR("Bug in %s", __func__);
> + return false;
> + }
> + ret = snprintf(newdir, newdirlen+1, "%s/%s", newpath, newname);
> + if (ret < 0 || ret >= newdirlen+1) {
> + ERROR("Bug in %s", __func__);
> + return false;
> + }
> + if (!conf->unexpanded_config)
> + return true;
> + while (*lstart) {
> + lend = strchr(lstart, '\n');
> + if (!lend)
> + lend = lstart + strlen(lstart);
> + else
> + lend++;
> + if (strncmp(lstart, key, strlen(key)) != 0) {
> + lstart = lend;
> + continue;
> + }
> + p = strchr(lstart+strlen(key), '=');
> + if (!p) {
> + lstart = lend;
> + continue;
> + }
> + p++;
> + while (isblank(*p))
> + p++;
> + if (!*p)
> + return true;
> + if (strncmp(p, olddir, strlen(olddir)) != 0) {
> + lstart = lend;
> + continue;
> + }
> + /* replace the olddir with newdir */
> + if (olddirlen >= newdirlen) {
> + size_t diff = olddirlen - newdirlen;
> + memcpy(p, newdir, newdirlen);
> + if (olddirlen != newdirlen) {
> + memmove(lend-diff, lend, strlen(lend)+1);
> + lend -= diff;
> + conf->unexpanded_len -= diff;
> + }
> + lstart = lend;
> + } else {
> + char *new;
> + size_t diff = newdirlen - olddirlen;
> + size_t oldlen = conf->unexpanded_len;
> + size_t newlen = oldlen + diff;
> + size_t poffset = p - conf->unexpanded_config;
> + new = realloc(conf->unexpanded_config, newlen);
> + if (!new) {
> + ERROR("Out of memory");
> return false;
> + }
> + conf->unexpanded_len = newlen;
> + new[newlen-1] = '\0';
> + lend = new + (lend - conf->unexpanded_config);
> + /* move over the remainder, /$hookname\n$rest */
> + memmove(new+poffset+newdirlen,
> + new+poffset+olddirlen,
> + oldlen-poffset-olddirlen);
> + conf->unexpanded_config = new;
> + memcpy(new+poffset, newdir, newdirlen);
> + lstart = lend + diff;
> }
> }
> return true;
> @@ -2429,93 +2498,81 @@ bool clone_update_unexp_hooks(struct lxc_conf *c)
> } \
> }
>
> -bool clone_update_unexp_network(struct lxc_conf *c)
> +static void new_hwaddr(char *hwaddr)
> +{
> + FILE *f;
> + f = fopen("/dev/urandom", "r");
> + if (f) {
> + unsigned int seed;
> + int ret = fread(&seed, sizeof(seed), 1, f);
> + if (ret != 1)
> + seed = time(NULL);
> + fclose(f);
> + srand(seed);
> + } else
> + srand(time(NULL));
> + snprintf(hwaddr, 18, "00:16:3e:%02x:%02x:%02x",
> + rand() % 255, rand() % 255, rand() % 255);
> +}
> +
> +/*
> + * This is called only from clone.
> + * We wish to update all hwaddrs in the unexpanded config file. We
> + * can't/don't want to update any which come from lxc.includes (there
> + * shouldn't be any).
> + * We can't just walk the c->lxc-conf->network list because that includes
> + * netifs from the include files. So we update the ones which we find in
> + * the unexp config file, then find the original macaddr in the
> + * conf->network, and update that to the same value.
> + */
> +bool network_new_hwaddrs(struct lxc_conf *conf)
> {
> struct lxc_list *it;
>
> - clear_unexp_config_line(c, "lxc.network", true);
> + const char *key = "lxc.network.hwaddr";
> + char *lstart = conf->unexpanded_config, *lend, *p, *p2;
>
> - lxc_list_for_each(it, &c->network) {
> - struct lxc_netdev *n = it->elem;
> - const char *t = lxc_net_type_to_str(n->type);
> - struct lxc_list *it2;
> - DO(do_append_unexp_config_line(c, "lxc.network.type", t ? t : "(invalid)"));
> - if (n->flags & IFF_UP)
> - DO(do_append_unexp_config_line(c, "lxc.network.flags", "up"));
> - if (n->link)
> - DO(do_append_unexp_config_line(c, "lxc.network.link", n->link));
> - if (n->name)
> - DO(do_append_unexp_config_line(c, "lxc.network.name", n->name));
> - if (n->type == LXC_NET_MACVLAN) {
> - const char *mode;
> - switch (n->priv.macvlan_attr.mode) {
> - case MACVLAN_MODE_PRIVATE: mode = "private"; break;
> - case MACVLAN_MODE_VEPA: mode = "vepa"; break;
> - case MACVLAN_MODE_BRIDGE: mode = "bridge"; break;
> - default: mode = "(invalid)"; break;
> - }
> - DO(do_append_unexp_config_line(c, "lxc.network.macvlan.mode", mode));
> - } else if (n->type == LXC_NET_VETH) {
> - if (n->priv.veth_attr.pair)
> - DO(do_append_unexp_config_line(c, "lxc.network.veth.pair", n->priv.veth_attr.pair));
> - } else if (n->type == LXC_NET_VLAN) {
> - char vid[20];
> - sprintf(vid, "%d", n->priv.vlan_attr.vid);
> - DO(do_append_unexp_config_line(c, "lxc.network.vlan.id", vid));
> - }
> - if (n->upscript)
> - DO(do_append_unexp_config_line(c, "lxc.network.script.up", n->upscript));
> - if (n->downscript)
> - DO(do_append_unexp_config_line(c, "lxc.network.script.down", n->downscript));
> - if (n->hwaddr)
> - DO(do_append_unexp_config_line(c, "lxc.network.hwaddr", n->hwaddr));
> - if (n->mtu)
> - DO(do_append_unexp_config_line(c, "lxc.network.mtu", n->mtu));
> - if (n->ipv4_gateway_auto) {
> - DO(do_append_unexp_config_line(c, "lxc.network.ipv4.gateway", "auto"));
> - } else if (n->ipv4_gateway) {
> - char buf[INET_ADDRSTRLEN];
> - inet_ntop(AF_INET, n->ipv4_gateway, buf, sizeof(buf));
> - DO(do_append_unexp_config_line(c, "lxc.network.ipv4.gateway", buf));
> + if (!conf->unexpanded_config)
> + return true;
> + while (*lstart) {
> + char newhwaddr[18], oldhwaddr[17];
> + lend = strchr(lstart, '\n');
> + if (!lend)
> + lend = lstart + strlen(lstart);
> + else
> + lend++;
> + if (strncmp(lstart, key, strlen(key)) != 0) {
> + lstart = lend;
> + continue;
> }
> - lxc_list_for_each(it2, &n->ipv4) {
> - struct lxc_inetdev *i = it2->elem;
> - char buf[2*INET_ADDRSTRLEN+20], buf2[INET_ADDRSTRLEN], prefix[20];
> - inet_ntop(AF_INET, &i->addr, buf, INET_ADDRSTRLEN);
> -
> - if (i->prefix) {
> - sprintf(prefix, "/%d", i->prefix);
> - strcat(buf, prefix);
> - }
> -
> - if (i->bcast.s_addr != (i->addr.s_addr |
> - htonl(INADDR_BROADCAST >> i->prefix))) {
> -
> - inet_ntop(AF_INET, &i->bcast, buf2, sizeof(buf2));
> - strcat(buf, " ");
> - strcat(buf, buf2);
> - }
> - DO(do_append_unexp_config_line(c, "lxc.network.ipv4", buf));
> + p = strchr(lstart+strlen(key), '=');
> + if (!p) {
> + lstart = lend;
> + continue;
> }
> - if (n->ipv6_gateway_auto) {
> - DO(do_append_unexp_config_line(c, "lxc.network.ipv6.gateway", "auto"));
> - } else if (n->ipv6_gateway) {
> - char buf[INET6_ADDRSTRLEN];
> - inet_ntop(AF_INET6, n->ipv6_gateway, buf, sizeof(buf));
> - DO(do_append_unexp_config_line(c, "lxc.network.ipv6.gateway", buf));
> + p++;
> + while (isblank(*p))
> + p++;
> + if (!*p)
> + return true;
> + p2 = p;
> + while (*p2 && !isblank(*p2) && *p2 != '\n')
> + p2++;
> + if (p2-p != 17) {
> + WARN("Bad hwaddr entry");
> + lstart = lend;
> + continue;
> }
> - lxc_list_for_each(it2, &n->ipv6) {
> - struct lxc_inet6dev *i = it2->elem;
> - char buf[INET6_ADDRSTRLEN + 20], prefix[20];
> - inet_ntop(AF_INET6, &i->addr, buf, INET6_ADDRSTRLEN);
> - if (i->prefix) {
> - sprintf(prefix, "/%d", i->prefix);
> - strcat(buf, prefix);
> - }
> - DO(do_append_unexp_config_line(c, "lxc.network.ipv6", buf));
> + memcpy(oldhwaddr, p, 17);
> + new_hwaddr(newhwaddr);
> + memcpy(p, newhwaddr, 17);
> + lxc_list_for_each(it, &conf->network) {
> + struct lxc_netdev *n = it->elem;
> + if (n->hwaddr && memcmp(oldhwaddr, n->hwaddr, 17) == 0)
> + memcpy(n->hwaddr, newhwaddr, 17);
> }
> +
> + lstart = lend;
> }
> - lxc_list_for_each(it, &c->environment)
> - DO(do_append_unexp_config_line(c, "lxc.environment", (char *)it->elem));
> return true;
> }
> diff --git a/src/lxc/confile.h b/src/lxc/confile.h
> index 6a81e02..8da3699 100644
> --- a/src/lxc/confile.h
> +++ b/src/lxc/confile.h
> @@ -59,6 +59,7 @@ extern bool do_append_unexp_config_line(struct lxc_conf *conf, const char *key,
>
> /* These are used when cloning a container */
> extern void clear_unexp_config_line(struct lxc_conf *conf, const char *key, bool rm_subkeys);
> -extern bool clone_update_unexp_hooks(struct lxc_conf *c);
> -extern bool clone_update_unexp_network(struct lxc_conf *c);
> +extern bool clone_update_unexp_hooks(struct lxc_conf *conf, const char *oldpath,
> + const char *newpath, const char *oldname, const char *newmame);
> +extern bool network_new_hwaddrs(struct lxc_conf *conf);
> #endif
> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> index ee8f491..07a63e7 100644
> --- a/src/lxc/lxccontainer.c
> +++ b/src/lxc/lxccontainer.c
> @@ -415,8 +415,6 @@ static bool load_config_locked(struct lxc_container *c, const char *fname)
> return false;
> if (lxc_config_read(fname, c->lxc_conf, false) != 0)
> return false;
> - if (!clone_update_unexp_network(c->lxc_conf))
> - return false;
> return true;
> }
>
> @@ -2434,7 +2432,8 @@ static int copyhooks(struct lxc_container *oldc, struct lxc_container *c)
> }
> }
>
> - if (!clone_update_unexp_hooks(c->lxc_conf)) {
> + if (!clone_update_unexp_hooks(c->lxc_conf, oldc->config_path,
> + c->config_path, oldc->name, c->name)) {
> ERROR("Error saving new hooks in clone");
> return -1;
> }
> @@ -2442,33 +2441,6 @@ static int copyhooks(struct lxc_container *oldc, struct lxc_container *c)
> return 0;
> }
>
> -static void new_hwaddr(char *hwaddr)
> -{
> - FILE *f;
> - f = fopen("/dev/urandom", "r");
> - if (f) {
> - unsigned int seed;
> - int ret = fread(&seed, sizeof(seed), 1, f);
> - if (ret != 1)
> - seed = time(NULL);
> - fclose(f);
> - srand(seed);
> - } else
> - srand(time(NULL));
> - snprintf(hwaddr, 18, "00:16:3e:%02x:%02x:%02x",
> - rand() % 255, rand() % 255, rand() % 255);
> -}
> -
> -static void network_new_hwaddrs(struct lxc_container *c)
> -{
> - struct lxc_list *it;
> -
> - lxc_list_for_each(it, &c->lxc_conf->network) {
> - struct lxc_netdev *n = it->elem;
> - if (n->hwaddr)
> - new_hwaddr(n->hwaddr);
> - }
> -}
>
> static int copy_fstab(struct lxc_container *oldc, struct lxc_container *c)
> {
> @@ -2844,9 +2816,8 @@ static struct lxc_container *lxcapi_clone(struct lxc_container *c, const char *n
>
> // update macaddrs
> if (!(flags & LXC_CLONE_KEEPMACADDR)) {
> - network_new_hwaddrs(c2);
> - if (!clone_update_unexp_network(c2->lxc_conf)) {
> - ERROR("Error updating network for clone");
> + if (!network_new_hwaddrs(c2->lxc_conf)) {
> + ERROR("Error updating mac addresses");
> goto out;
> }
> }
> diff --git a/src/tests/Makefile.am b/src/tests/Makefile.am
> index 35bc1b3..ed2be98 100644
> --- a/src/tests/Makefile.am
> +++ b/src/tests/Makefile.am
> @@ -48,7 +48,7 @@ bin_PROGRAMS = lxc-test-containertests lxc-test-locktests lxc-test-startone \
> lxc-test-reboot lxc-test-list lxc-test-attach lxc-test-device-add-remove \
> lxc-test-apparmor
>
> -bin_SCRIPTS = lxc-test-autostart
> +bin_SCRIPTS = lxc-test-autostart lxc-test-cloneconfig
>
> if DISTRO_UBUNTU
> bin_SCRIPTS += \
> diff --git a/src/tests/lxc-test-cloneconfig b/src/tests/lxc-test-cloneconfig
> new file mode 100755
> index 0000000..0f38369
> --- /dev/null
> +++ b/src/tests/lxc-test-cloneconfig
> @@ -0,0 +1,139 @@
> +#!/bin/bash
> +
> +# lxc: linux Container library
> +
> +# Authors:
> +# Serge Hallyn <serge.hallyn at ubuntu.com>
> +#
> +# This is a test script for the lxc-user-nic program
> +
> +# This library is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU Lesser General Public
> +# License as published by the Free Software Foundation; either
> +# version 2.1 of the License, or (at your option) any later version.
> +
> +# This library is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +# Lesser General Public License for more details.
> +
> +# You should have received a copy of the GNU Lesser General Public
> +# License along with this library; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> +
> +set -e
> +
> +s=`mktemp -d /tmp/lxctest-XXXXXX`
> +
> +DONE=0
> +
> +verify_unchanged_number() {
> + key=$1
> + desc=$2
> + n1=`grep ^$key $CONTAINER_PATH/config | wc -l`
> + n2=`grep ^$key $CONTAINER2_PATH/config | wc -l`
> + if [ $n1 -ne $n2 ]; then
> + echo "Test $testnum failed"
> + echo "unequal number of $desc"
> + echo "Original has $n1, clone has $n2"
> + false
> + fi
> +}
> +
> +cleanup() {
> + lxc-destroy -n lxctestb || true
> + lxc-destroy -n lxctestb2 || true
> + rm -rf $s
> + [ $DONE -eq 1 ] && echo "PASS" || echo "FAIL"
> +}
> +
> +verify_numnics() {
> + verify_unchanged_number lxc.network.type "network defs"
> +}
> +
> +verify_hwaddr() {
> + verify_unchanged_number lxc.network.hwaddr "hwaddr defs"
> + grep ^lxc.network.hwaddr $CONTAINER_PATH/config | while read line; do
> + addr=`echo $line | awk -F= { print $2 }`
> + echo "looking for $addr in $CONTAINER2_PATH/config"
> + if grep -q $addr $CONTAINER2_PATH/config; then
> + echo "Test $testnum failed"
> + echo "hwaddr $addr was not changed"
> + false
> + fi
> + done
> +}
> +
> +verify_hooks() {
> + verify_unchanged_number lxc.hook "hooks"
> + grep ^lxc.hook $CONTAINER_PATH/config | while read line; do
> + nline=${line/$CONTAINER_PATH/$CONTAINER2_PATH}
> + if ! grep -q "$nline" $CONTAINER2_PATH/config; then
> + echo "Test $testnum failed"
> + echo "Failed to find $nline in:"
> + cat $CONTAINER2_PATH/config
> + fi
> + done
> +}
> +
> +trap cleanup EXIT
> +
> +# Simple nic
> +cat > $s/1.conf << EOF
> +lxc.network.type = veth
> +lxc.network.link = lxcbr0
> +EOF
> +
> +# Simple nic with hwaddr; verify hwaddr changed
> +cat > $s/2.conf << EOF
> +lxc.network.type = veth
> +lxc.network.link = lxcbr0
> +EOF
> +
> +# No nics, but nic from include
> +cat > $s/1.include << EOF
> +lxc.network.type = veth
> +lxc.network.link = lxcbr0
> +lxc.hook.start = /bin/bash
> +EOF
> +cat > $s/3.conf << EOF
> +lxc.include = $s/1.include
> +EOF
> +
> +# No nics, one clone hook in /bin
> +cat > $s/4.conf << EOF
> +lxc.hook.start = /bin/bash
> +EOF
> +
> +# Figure out container dirname
> +# We need this in 5.conf
> +lxc-destroy -n lxctestb || true
> +lxc-create -t busybox -n lxctestb
> +CONTAINER_PATH=$(dirname $(lxc-info -n lxctestb -c lxc.rootfs -H))
> +lxc-destroy -n lxctestb
> +
> +# No nics, one clone hook in $container
> +cat > $s/5.conf << EOF
> +lxc.hook.start = $CONTAINER_PATH/x1
> +EOF
> +
> +CONTAINER2_PATH="${CONTAINER_PATH}2"
> +
> +testnum=1
> +for f in $s/*.conf; do
> + echo "Test $testnum starting ($f)"
> + lxc-create -t busybox -f $f -n lxctestb
> + touch $CONTAINER_PATH/x1
> + lxc-clone -s -o lxctestb -n lxctestb2
> + # verify that # nics did not change
> + verify_numnics
> + # verify hwaddr, if any changed
> + verify_hwaddr
> + # verify hooks are correct
> + verify_hooks
> + lxc-destroy -n lxctestb2 || true
> + lxc-destroy -n lxctestb || true
> + testnum=$((testnum+1))
> +done
> +
> +DONE=1
> --
> 2.1.0
>
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel
--
Stéphane Graber
Ubuntu developer
http://www.ubuntu.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20140919/1286f494/attachment.sig>
More information about the lxc-devel
mailing list