[libvirt] [PATCH] tests: dynamically replace dnsmasq path
The path to the dnsmasq binary can be configured while in the test data the path is hard-coded to /usr/bin/. This break the test suite if a the binary is located in a different location, like /usr/local/sbin/. Replace the hard coded path in the test data by a token, which is dynamically replaced in networkxml2argvtest with the configured path after the test data has been loaded. (Another option would have been to modify configure.ac to generate the test data during configure, but I do not know of an easy way do trick configure into mass-generate those test files without listing every single one, which I consider less flexible.) Signed-off-by: Philipp Hahn <hahn@univention.de> --- tests/networkxml2argvdata/isolated-network.argv | 2 +- .../networkxml2argvdata/nat-network-dns-hosts.argv | 2 +- .../nat-network-dns-srv-record-minimal.argv | 2 +- .../nat-network-dns-srv-record.argv | 2 +- .../nat-network-dns-txt-record.argv | 2 +- tests/networkxml2argvdata/nat-network.argv | 2 +- tests/networkxml2argvdata/netboot-network.argv | 2 +- .../networkxml2argvdata/netboot-proxy-network.argv | 2 +- tests/networkxml2argvdata/routed-network.argv | 2 +- tests/networkxml2argvtest.c | 42 ++++++++++++++++++++ 10 files changed, 51 insertions(+), 9 deletions(-) diff --git a/tests/networkxml2argvdata/isolated-network.argv b/tests/networkxml2argvdata/isolated-network.argv index 7ea2e94..a9beb05 100644 --- a/tests/networkxml2argvdata/isolated-network.argv +++ b/tests/networkxml2argvdata/isolated-network.argv @@ -1,4 +1,4 @@ -/usr/sbin/dnsmasq --strict-order --bind-interfaces --conf-file= \ +@DNSMASQ@ --strict-order --bind-interfaces --conf-file= \ --except-interface lo --dhcp-option=3 --no-resolv \ --listen-address 192.168.152.1 \ --dhcp-range 192.168.152.2,192.168.152.254 \ diff --git a/tests/networkxml2argvdata/nat-network-dns-hosts.argv b/tests/networkxml2argvdata/nat-network-dns-hosts.argv index 2158df8..c7e4967 100644 --- a/tests/networkxml2argvdata/nat-network-dns-hosts.argv +++ b/tests/networkxml2argvdata/nat-network-dns-hosts.argv @@ -1,3 +1,3 @@ -/usr/sbin/dnsmasq --strict-order --bind-interfaces --domain example.com \ +@DNSMASQ@ --strict-order --bind-interfaces --domain example.com \ --conf-file= --except-interface lo --listen-address 192.168.122.1 \ --expand-hosts --addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ diff --git a/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv b/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv index 021e8f0..ea1da6d 100644 --- a/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv +++ b/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv @@ -1,4 +1,4 @@ -/usr/sbin/dnsmasq \ +@DNSMASQ@ \ --strict-order \ --bind-interfaces \ --conf-file= \ diff --git a/tests/networkxml2argvdata/nat-network-dns-srv-record.argv b/tests/networkxml2argvdata/nat-network-dns-srv-record.argv index 85afbba..84c2d2f 100644 --- a/tests/networkxml2argvdata/nat-network-dns-srv-record.argv +++ b/tests/networkxml2argvdata/nat-network-dns-srv-record.argv @@ -1,4 +1,4 @@ -/usr/sbin/dnsmasq \ +@DNSMASQ@ \ --strict-order \ --bind-interfaces \ --conf-file= \ diff --git a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv index be6ba4b..bed309f 100644 --- a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv +++ b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv @@ -1,4 +1,4 @@ -/usr/sbin/dnsmasq --strict-order --bind-interfaces --conf-file= \ +@DNSMASQ@ --strict-order --bind-interfaces --conf-file= \ --except-interface lo --txt-record=example,example value \ --listen-address 192.168.122.1 --listen-address 192.168.123.1 \ --listen-address 2001:db8:ac10:fe01::1 \ diff --git a/tests/networkxml2argvdata/nat-network.argv b/tests/networkxml2argvdata/nat-network.argv index d7faee2..80878a8 100644 --- a/tests/networkxml2argvdata/nat-network.argv +++ b/tests/networkxml2argvdata/nat-network.argv @@ -1,4 +1,4 @@ -/usr/sbin/dnsmasq --strict-order --bind-interfaces --conf-file= \ +@DNSMASQ@ --strict-order --bind-interfaces --conf-file= \ --except-interface lo --listen-address 192.168.122.1 \ --listen-address 192.168.123.1 --listen-address 2001:db8:ac10:fe01::1 \ --listen-address 2001:db8:ac10:fd01::1 --listen-address 10.24.10.1 \ diff --git a/tests/networkxml2argvdata/netboot-network.argv b/tests/networkxml2argvdata/netboot-network.argv index 78e873c..7ae5b75 100644 --- a/tests/networkxml2argvdata/netboot-network.argv +++ b/tests/networkxml2argvdata/netboot-network.argv @@ -1,4 +1,4 @@ -/usr/sbin/dnsmasq --strict-order --bind-interfaces --domain example.com \ +@DNSMASQ@ --strict-order --bind-interfaces --domain example.com \ --conf-file= --except-interface lo --listen-address 192.168.122.1 \ --dhcp-range 192.168.122.2,192.168.122.254 \ --dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases \ diff --git a/tests/networkxml2argvdata/netboot-proxy-network.argv b/tests/networkxml2argvdata/netboot-proxy-network.argv index 5fe1b8e..2c5a0d5 100644 --- a/tests/networkxml2argvdata/netboot-proxy-network.argv +++ b/tests/networkxml2argvdata/netboot-proxy-network.argv @@ -1,4 +1,4 @@ -/usr/sbin/dnsmasq --strict-order --bind-interfaces --domain example.com \ +@DNSMASQ@ --strict-order --bind-interfaces --domain example.com \ --conf-file= --except-interface lo --listen-address 192.168.122.1 \ --dhcp-range 192.168.122.2,192.168.122.254 \ --dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases \ diff --git a/tests/networkxml2argvdata/routed-network.argv b/tests/networkxml2argvdata/routed-network.argv index d059630..75303e6 100644 --- a/tests/networkxml2argvdata/routed-network.argv +++ b/tests/networkxml2argvdata/routed-network.argv @@ -1,2 +1,2 @@ -/usr/sbin/dnsmasq --strict-order --bind-interfaces --conf-file= \ +@DNSMASQ@ --strict-order --bind-interfaces --conf-file= \ --except-interface lo --listen-address 192.168.122.1\ diff --git a/tests/networkxml2argvtest.c b/tests/networkxml2argvtest.c index 2dd9b7f..ec2ef39 100644 --- a/tests/networkxml2argvtest.c +++ b/tests/networkxml2argvtest.c @@ -15,6 +15,45 @@ #include "memory.h" #include "network/bridge_driver.h" +/* Replace all occurrences of @token in @buf by @replacement and adjust size of + * @buf accordingly. Returns 0 on success and -1 on out-of-memory errors. */ +static int replaceTokens(char **buf, const char *token, const char *replacement) { + char *token_start, *token_end; + size_t old_len, new_len, rest_len; + void *tmp; + const size_t token_len = strlen(token); + const size_t replacement_len = strlen(replacement); + const int diff = replacement_len - token_len; + + old_len = rest_len = strlen(*buf) + 1; + token_end = *buf; + for (;;) { + token_start = strstr(token_end, token); + if (token_start == NULL) + break; + rest_len -= token_start + token_len - token_end; + token_end = token_start + token_len; + new_len = old_len + diff; + if (diff > 0) { + tmp = realloc((void *)*buf, new_len); + if (tmp == NULL) + return -1; + *buf = tmp; + } + if (diff != 0) + memmove(token_end + diff, token_end, rest_len); + memmove(token_start, replacement, replacement_len); + if (diff < 0) { + tmp = realloc((void *)*buf, new_len); + if (tmp == NULL) + return -1; + *buf = tmp; + } + old_len = new_len; + } + return 0; +} + static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) { char *inXmlData = NULL; char *outArgvData = NULL; @@ -32,6 +71,9 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) { if (virtTestLoadFile(outargv, &outArgvData) < 0) goto fail; + if (replaceTokens(&outArgvData, "@DNSMASQ@", DNSMASQ)) + goto fail; + if (!(dev = virNetworkDefParseString(inXmlData))) goto fail; -- 1.7.1
On 01/30/2012 10:13 AM, Philipp Hahn wrote:
The path to the dnsmasq binary can be configured while in the test data the path is hard-coded to /usr/bin/. This break the test suite if a the binary is located in a different location, like /usr/local/sbin/.
Replace the hard coded path in the test data by a token, which is dynamically replaced in networkxml2argvtest with the configured path after the test data has been loaded.
Cool hack!
(Another option would have been to modify configure.ac to generate the test data during configure, but I do not know of an easy way do trick configure into mass-generate those test files without listing every single one, which I consider less flexible.)
Agreed with your reasoning and choice of implementation to do the replacement at test run rather than configure time. However,
+++ b/tests/networkxml2argvtest.c @@ -15,6 +15,45 @@ #include "memory.h" #include "network/bridge_driver.h"
+/* Replace all occurrences of @token in @buf by @replacement and adjust size of + * @buf accordingly. Returns 0 on success and -1 on out-of-memory errors. */ +static int replaceTokens(char **buf, const char *token, const char *replacement) { + char *token_start, *token_end; + size_t old_len, new_len, rest_len; + void *tmp; + const size_t token_len = strlen(token); + const size_t replacement_len = strlen(replacement); + const int diff = replacement_len - token_len; + + old_len = rest_len = strlen(*buf) + 1; + token_end = *buf; + for (;;) { + token_start = strstr(token_end, token); + if (token_start == NULL) + break; + rest_len -= token_start + token_len - token_end; + token_end = token_start + token_len; + new_len = old_len + diff; + if (diff > 0) { + tmp = realloc((void *)*buf, new_len);
We should not be using realloc in this file, but should be using VIR_RESIZE_N or similar.
+ if (tmp == NULL) + return -1; + *buf = tmp; + } + if (diff != 0) + memmove(token_end + diff, token_end, rest_len);
This must be memmove,
+ memmove(token_start, replacement, replacement_len);
But this would be more efficient as memcpy, since replacement (better not) overlap with token_start.
+ if (diff < 0) { + tmp = realloc((void *)*buf, new_len);
No need to downsize the allocation - it's okay to leave junk in the tail end of the buffer, as long as we have a trailing NUL to stop us in time. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Hello Eric, Am Montag 30 Januar 2012 20:18:15 schrieb Eric Blake:
+ if (diff > 0) { + tmp = realloc((void *)*buf, new_len);
We should not be using realloc in this file, but should be using VIR_RESIZE_N or similar.
Okay, makes the code even more readable.
+ memmove(token_start, replacement, replacement_len);
But this would be more efficient as memcpy, since replacement (better not) overlap with token_start.
I know of some projects which forbid memcpy() because of the missing overlap handling. But if this is okay with libvirt, I'll use it. I hope performance will never be a problem with this simple test scenario, because then doing one realloc() instead of one for each found would be better.
+ if (diff < 0) { + tmp = realloc((void *)*buf, new_len);
No need to downsize the allocation - it's okay to leave junk in the tail end of the buffer, as long as we have a trailing NUL to stop us in time.
Fine with me for this simple test case, but if that would be generalized and be used in other places, this might become a problem. I'll add a comment. Sincerely Philipp -- Philipp Hahn Open Source Software Engineer hahn@univention.de Univention GmbH Linux for Your Business fon: +49 421 22 232- 0 Mary-Somerville-Str.1 D-28359 Bremen fax: +49 421 22 232-99 http://www.univention.de/
On 01/31/2012 07:58 AM, Philipp Hahn wrote:
Hello Eric,
Am Montag 30 Januar 2012 20:18:15 schrieb Eric Blake:
+ if (diff > 0) { + tmp = realloc((void *)*buf, new_len);
We should not be using realloc in this file, but should be using VIR_RESIZE_N or similar.
Okay, makes the code even more readable.
+ memmove(token_start, replacement, replacement_len);
But this would be more efficient as memcpy, since replacement (better not) overlap with token_start.
I know of some projects which forbid memcpy() because of the missing overlap handling. But if this is okay with libvirt, I'll use it.
memcpy() is safe when the code is provably visiting non-overlapping strings, as is the case with your second use of memmove(). memcpy() is a mistake where the code is provably visiting overlapping strings, as is the case with your first memmove(). On some libc, memmove and memcpy are the same function; but since the standards allow memcpy to be more efficient than memmove by exploiting the non-overlap guarantee, you might as well use the more efficient method in the cases where things really are non-overlapping. Libvirt isn't so strict as to pessimize correct uses of memcpy() just because it can sometime be misused. It also helps that static analyzers like Coverity are getting pretty good at pointing out mis-uses of memcpy().
I hope performance will never be a problem with this simple test scenario, because then doing one realloc() instead of one for each found would be better.
You do have a valid counterpoint here - this is a test, and not a hot path, so the savings of memcpy() vs. memmove() are in the noise compared to any extra realloc(); thankfully, neither overhead is worth worrying about too much. At any rate, I'll review your v2 now. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Philipp Hahn