config: fix the handling of lxc.hook and hwaddrs in unexpanded config

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@ubuntu.com>
Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
Acked-by: Stéphane Graber <stgraber@ubuntu.com>

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;
+	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);
 
-	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))
+	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 (!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;
 		}
-		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));
+		p = strchr(lstart+strlen(key), '=');
+		if (!p) {
+			lstart = lend;
+			continue;
+		}
+		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;
+		}
+		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);
 		}
-		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));
-		}
-		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));
-		}
-		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));
-		}
+		lstart = lend;
 	}
-	lxc_list_for_each(it, &c->environment)
-		DO(do_append_unexp_config_line(c, "lxc.environment", (char *)it->elem));
 	return true;
 }

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

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;
 		}
 	}

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 \

src/tests/lxc-test-cloneconfig

@@ -0,0 +1,139 @@
+#!/bin/bash
+
+# lxc: linux Container library
+
+# Authors:
+# Serge Hallyn <serge.hallyn@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