From 67702c2129c462b5e8124020a496fbf6b7ae5540 Mon Sep 17 00:00:00 2001 From: Serge Hallyn Date: Mon, 1 Sep 2014 20:01:20 +0000 Subject: [PATCH] config: fix the handling of lxc.hook and hwaddrs in unexpanded config MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Serge Hallyn Acked-by: Stéphane Graber --- src/lxc/confile.c | 231 ++++++++++++++++++++------------- src/lxc/confile.h | 5 +- src/lxc/lxccontainer.c | 37 +----- src/tests/Makefile.am | 3 +- src/tests/lxc-test-cloneconfig | 139 ++++++++++++++++++++ 5 files changed, 292 insertions(+), 123 deletions(-) create mode 100755 src/tests/lxc-test-cloneconfig diff --git a/src/lxc/confile.c b/src/lxc/confile.c index 5de1241f0..9b1fba882 100644 --- a/src/lxc/confile.c +++ b/src/lxc/confile.c @@ -36,6 +36,7 @@ #include #include #include +#include #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; ihooks[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 6a81e02c7..8da369907 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 ee8f491ad..07a63e7ec 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 35bc1b3dd..67f83838b 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 += \ @@ -76,6 +76,7 @@ EXTRA_DIST = \ lxcpath.c \ lxc-test-autostart \ lxc-test-checkpoint-restore \ + lxc-test-cloneconfig \ lxc-test-ubuntu \ lxc-test-unpriv \ lxc-test-usernic \ diff --git a/src/tests/lxc-test-cloneconfig b/src/tests/lxc-test-cloneconfig new file mode 100755 index 000000000..0f38369d9 --- /dev/null +++ b/src/tests/lxc-test-cloneconfig @@ -0,0 +1,139 @@ +#!/bin/bash + +# lxc: linux Container library + +# Authors: +# Serge Hallyn +# +# 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.47.2