[libvirt] [PATCH 0/3] Fix networkxml2conftest on FreeBSD
Yet another approach. If you hate it, we still have this: https://www.redhat.com/archives/libvir-list/2016-December/msg01123.html as an option. Martin Kletzander (3): networkxml2conftest: Rename outxml to outconf networkxml2conftest: Indent function parameters properly networkxml2conftest: Fix build on BSD tests/networkxml2confdata/dhcp6-nat-network.conf | 2 +- tests/networkxml2confdata/dhcp6-network.conf | 2 +- .../dhcp6host-routed-network.conf | 2 +- tests/networkxml2confdata/isolated-network.conf | 2 +- .../nat-network-dns-forward-plain.conf | 2 +- .../nat-network-dns-forwarders.conf | 2 +- .../networkxml2confdata/nat-network-dns-hosts.conf | 2 +- .../nat-network-dns-local-domain.conf | 2 +- .../nat-network-dns-srv-record-minimal.conf | 2 +- .../nat-network-dns-srv-record.conf | 2 +- .../nat-network-dns-txt-record.conf | 2 +- .../nat-network-name-with-quotes.conf | 2 +- tests/networkxml2confdata/nat-network.conf | 2 +- tests/networkxml2confdata/netboot-network.conf | 2 +- .../networkxml2confdata/netboot-proxy-network.conf | 2 +- tests/networkxml2confdata/open-network.conf | 2 +- tests/networkxml2confdata/ptr-domains-auto.conf | 2 +- .../networkxml2confdata/routed-network-no-dns.conf | 2 +- tests/networkxml2confdata/routed-network.conf | 2 +- tests/networkxml2conftest.c | 51 +++++++++++++++++++--- 20 files changed, 63 insertions(+), 26 deletions(-) -- 2.11.0
Just a name, I know, but it bothered me a lot since it does not refer to XML. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tests/networkxml2conftest.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index a80d3b2d45d3..9a3eab1f8230 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -70,20 +70,20 @@ testCompareXMLToConfHelper(const void *data) int result = -1; const testInfo *info = data; char *inxml = NULL; - char *outxml = NULL; + char *outconf = NULL; if (virAsprintf(&inxml, "%s/networkxml2confdata/%s.xml", abs_srcdir, info->name) < 0 || - virAsprintf(&outxml, "%s/networkxml2confdata/%s.conf", + virAsprintf(&outconf, "%s/networkxml2confdata/%s.conf", abs_srcdir, info->name) < 0) { goto cleanup; } - result = testCompareXMLToConfFiles(inxml, outxml, info->caps); + result = testCompareXMLToConfFiles(inxml, outconf, info->caps); cleanup: VIR_FREE(inxml); - VIR_FREE(outxml); + VIR_FREE(outconf); return result; } -- 2.11.0
On Wed, 2016-12-28 at 23:10 +0100, Martin Kletzander wrote:
Just a name, I know, but it bothered me a lot since it does not refer to XML.
Please push this regardless of which approach we end up choosing for the actual fix :) ACK -- Andrea Bolognani / Red Hat / Virtualization
Otherwise we might end up bikeshedding on the next patch. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tests/networkxml2conftest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 9a3eab1f8230..7ff243b98d20 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -42,7 +42,7 @@ testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr goto fail; if (networkDnsmasqConfContents(obj, pidfile, &actual, - dctx, caps) < 0) + dctx, caps) < 0) goto fail; if (virTestCompareToFile(actual, outconf) < 0) -- 2.11.0
Yet another way to fix the different loopback naming. This variant doesn't use multiple files, allows easy addition of more loopback names and factoring out the naming code to a function (which would programmatically get the loopback interface name) is very easy. OTOH we lose a way to regenerate the files (well, the *easy* way). Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- Notes: The build is *NOT* tested on any non-Linux patch after this change. I feel like it's easier for someone else to try it out than for me to spin up a new VM, install FreeBSD, figure out what's needed for building libvirt there and all that during the holidays. So please, if someone is bored out of their mind, feel free to test it out ;) tests/networkxml2confdata/dhcp6-nat-network.conf | 2 +- tests/networkxml2confdata/dhcp6-network.conf | 2 +- .../dhcp6host-routed-network.conf | 2 +- tests/networkxml2confdata/isolated-network.conf | 2 +- .../nat-network-dns-forward-plain.conf | 2 +- .../nat-network-dns-forwarders.conf | 2 +- .../networkxml2confdata/nat-network-dns-hosts.conf | 2 +- .../nat-network-dns-local-domain.conf | 2 +- .../nat-network-dns-srv-record-minimal.conf | 2 +- .../nat-network-dns-srv-record.conf | 2 +- .../nat-network-dns-txt-record.conf | 2 +- .../nat-network-name-with-quotes.conf | 2 +- tests/networkxml2confdata/nat-network.conf | 2 +- tests/networkxml2confdata/netboot-network.conf | 2 +- .../networkxml2confdata/netboot-proxy-network.conf | 2 +- tests/networkxml2confdata/open-network.conf | 2 +- tests/networkxml2confdata/ptr-domains-auto.conf | 2 +- .../networkxml2confdata/routed-network-no-dns.conf | 2 +- tests/networkxml2confdata/routed-network.conf | 2 +- tests/networkxml2conftest.c | 41 ++++++++++++++++++++-- 20 files changed, 58 insertions(+), 21 deletions(-) diff --git a/tests/networkxml2confdata/dhcp6-nat-network.conf b/tests/networkxml2confdata/dhcp6-nat-network.conf index d1058df3b65e..7185236aac0a 100644 --- a/tests/networkxml2confdata/dhcp6-nat-network.conf +++ b/tests/networkxml2confdata/dhcp6-nat-network.conf @@ -5,7 +5,7 @@ ## ## dnsmasq conf file created by libvirt strict-order -except-interface=lo +except-interface=@LOOPBACK_NAME@ bind-dynamic interface=virbr0 dhcp-range=192.168.122.2,192.168.122.254 diff --git a/tests/networkxml2confdata/dhcp6-network.conf b/tests/networkxml2confdata/dhcp6-network.conf index 82706903b31d..b99d5b1d4ab9 100644 --- a/tests/networkxml2confdata/dhcp6-network.conf +++ b/tests/networkxml2confdata/dhcp6-network.conf @@ -7,7 +7,7 @@ strict-order domain=mynet expand-hosts -except-interface=lo +except-interface=@LOOPBACK_NAME@ bind-dynamic interface=virbr0 dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64 diff --git a/tests/networkxml2confdata/dhcp6host-routed-network.conf b/tests/networkxml2confdata/dhcp6host-routed-network.conf index 87a149880992..b8840eee1e20 100644 --- a/tests/networkxml2confdata/dhcp6host-routed-network.conf +++ b/tests/networkxml2confdata/dhcp6host-routed-network.conf @@ -5,7 +5,7 @@ ## ## dnsmasq conf file created by libvirt strict-order -except-interface=lo +except-interface=@LOOPBACK_NAME@ bind-dynamic interface=virbr1 dhcp-range=192.168.122.1,static diff --git a/tests/networkxml2confdata/isolated-network.conf b/tests/networkxml2confdata/isolated-network.conf index ce4a59f6c1a7..ad2bdecefe70 100644 --- a/tests/networkxml2confdata/isolated-network.conf +++ b/tests/networkxml2confdata/isolated-network.conf @@ -5,7 +5,7 @@ ## ## dnsmasq conf file created by libvirt strict-order -except-interface=lo +except-interface=@LOOPBACK_NAME@ bind-interfaces listen-address=192.168.152.1 dhcp-option=3 diff --git a/tests/networkxml2confdata/nat-network-dns-forward-plain.conf b/tests/networkxml2confdata/nat-network-dns-forward-plain.conf index 9a000b887968..c384b8cfab39 100644 --- a/tests/networkxml2confdata/nat-network-dns-forward-plain.conf +++ b/tests/networkxml2confdata/nat-network-dns-forward-plain.conf @@ -5,7 +5,7 @@ ## ## dnsmasq conf file created by libvirt strict-order -except-interface=lo +except-interface=@LOOPBACK_NAME@ bind-dynamic interface=virbr0 addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2confdata/nat-network-dns-forwarders.conf b/tests/networkxml2confdata/nat-network-dns-forwarders.conf index 0bd76bf60c97..1560c7d0fe62 100644 --- a/tests/networkxml2confdata/nat-network-dns-forwarders.conf +++ b/tests/networkxml2confdata/nat-network-dns-forwarders.conf @@ -10,7 +10,7 @@ server=8.8.8.8 server=8.8.4.4 server=/example.com/192.168.1.1 server=/www.example.com/# -except-interface=lo +except-interface=@LOOPBACK_NAME@ bind-dynamic interface=virbr0 addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2confdata/nat-network-dns-hosts.conf b/tests/networkxml2confdata/nat-network-dns-hosts.conf index 021316f9c768..23bf2fe9052e 100644 --- a/tests/networkxml2confdata/nat-network-dns-hosts.conf +++ b/tests/networkxml2confdata/nat-network-dns-hosts.conf @@ -9,7 +9,7 @@ domain=example.com expand-hosts domain-needed local=// -except-interface=lo +except-interface=@LOOPBACK_NAME@ bind-dynamic interface=virbr0 addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2confdata/nat-network-dns-local-domain.conf b/tests/networkxml2confdata/nat-network-dns-local-domain.conf index 5f41b9186cbc..922bbcf77af5 100644 --- a/tests/networkxml2confdata/nat-network-dns-local-domain.conf +++ b/tests/networkxml2confdata/nat-network-dns-local-domain.conf @@ -8,7 +8,7 @@ strict-order local=/example.com/ domain=example.com expand-hosts -except-interface=lo +except-interface=@LOOPBACK_NAME@ bind-dynamic interface=virbr0 addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf index f35ea1d5d42d..ab5788f099f3 100644 --- a/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf +++ b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf @@ -5,7 +5,7 @@ ## ## dnsmasq conf file created by libvirt strict-order -except-interface=lo +except-interface=@LOOPBACK_NAME@ bind-interfaces listen-address=192.168.122.1 listen-address=192.168.123.1 diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record.conf b/tests/networkxml2confdata/nat-network-dns-srv-record.conf index af1ed7075851..78412f8d2446 100644 --- a/tests/networkxml2confdata/nat-network-dns-srv-record.conf +++ b/tests/networkxml2confdata/nat-network-dns-srv-record.conf @@ -5,7 +5,7 @@ ## ## dnsmasq conf file created by libvirt strict-order -except-interface=lo +except-interface=@LOOPBACK_NAME@ bind-dynamic interface=virbr0 srv-host=_name._tcp.test-domain-name.com,test.example.com,1111,11,111 diff --git a/tests/networkxml2confdata/nat-network-dns-txt-record.conf b/tests/networkxml2confdata/nat-network-dns-txt-record.conf index 7f560fbb5c76..ff553fff7bb6 100644 --- a/tests/networkxml2confdata/nat-network-dns-txt-record.conf +++ b/tests/networkxml2confdata/nat-network-dns-txt-record.conf @@ -5,7 +5,7 @@ ## ## dnsmasq conf file created by libvirt strict-order -except-interface=lo +except-interface=@LOOPBACK_NAME@ bind-dynamic interface=virbr0 txt-record=example,example value diff --git a/tests/networkxml2confdata/nat-network-name-with-quotes.conf b/tests/networkxml2confdata/nat-network-name-with-quotes.conf index 36e11d17b990..35289f0bdb75 100644 --- a/tests/networkxml2confdata/nat-network-name-with-quotes.conf +++ b/tests/networkxml2confdata/nat-network-name-with-quotes.conf @@ -5,7 +5,7 @@ ## ## dnsmasq conf file created by libvirt strict-order -except-interface=lo +except-interface=@LOOPBACK_NAME@ bind-interfaces listen-address=192.168.122.1 listen-address=192.168.123.1 diff --git a/tests/networkxml2confdata/nat-network.conf b/tests/networkxml2confdata/nat-network.conf index a3c8b102d302..d2b15471c6ab 100644 --- a/tests/networkxml2confdata/nat-network.conf +++ b/tests/networkxml2confdata/nat-network.conf @@ -5,7 +5,7 @@ ## ## dnsmasq conf file created by libvirt strict-order -except-interface=lo +except-interface=@LOOPBACK_NAME@ bind-dynamic interface=virbr0 dhcp-range=192.168.122.2,192.168.122.254 diff --git a/tests/networkxml2confdata/netboot-network.conf b/tests/networkxml2confdata/netboot-network.conf index b554a5456c6a..d94d4255c6ea 100644 --- a/tests/networkxml2confdata/netboot-network.conf +++ b/tests/networkxml2confdata/netboot-network.conf @@ -7,7 +7,7 @@ strict-order domain=example.com expand-hosts -except-interface=lo +except-interface=@LOOPBACK_NAME@ bind-interfaces listen-address=192.168.122.1 dhcp-range=192.168.122.2,192.168.122.254 diff --git a/tests/networkxml2confdata/netboot-proxy-network.conf b/tests/networkxml2confdata/netboot-proxy-network.conf index afb4033f7eda..67e86a5168e3 100644 --- a/tests/networkxml2confdata/netboot-proxy-network.conf +++ b/tests/networkxml2confdata/netboot-proxy-network.conf @@ -7,7 +7,7 @@ strict-order domain=example.com expand-hosts -except-interface=lo +except-interface=@LOOPBACK_NAME@ bind-interfaces listen-address=192.168.122.1 dhcp-range=192.168.122.2,192.168.122.254 diff --git a/tests/networkxml2confdata/open-network.conf b/tests/networkxml2confdata/open-network.conf index ff099847d43f..2c20773efc71 100644 --- a/tests/networkxml2confdata/open-network.conf +++ b/tests/networkxml2confdata/open-network.conf @@ -5,7 +5,7 @@ ## ## dnsmasq conf file created by libvirt strict-order -except-interface=lo +except-interface=@LOOPBACK_NAME@ bind-dynamic interface=virbr1 addn-hosts=/var/lib/libvirt/dnsmasq/open.addnhosts diff --git a/tests/networkxml2confdata/ptr-domains-auto.conf b/tests/networkxml2confdata/ptr-domains-auto.conf index 7f1a393dd5a2..f49512fe428f 100644 --- a/tests/networkxml2confdata/ptr-domains-auto.conf +++ b/tests/networkxml2confdata/ptr-domains-auto.conf @@ -7,7 +7,7 @@ strict-order local=/122.168.192.in-addr.arpa/ local=/1.0.e.f.0.1.c.a.8.b.d.0.1.0.0.2.ip6.arpa/ -except-interface=lo +except-interface=@LOOPBACK_NAME@ bind-dynamic interface=virbr0 dhcp-range=192.168.122.2,192.168.122.254 diff --git a/tests/networkxml2confdata/routed-network-no-dns.conf b/tests/networkxml2confdata/routed-network-no-dns.conf index 83cc85ea6b47..fe5d55b19be4 100644 --- a/tests/networkxml2confdata/routed-network-no-dns.conf +++ b/tests/networkxml2confdata/routed-network-no-dns.conf @@ -6,6 +6,6 @@ ## dnsmasq conf file created by libvirt strict-order port=0 -except-interface=lo +except-interface=@LOOPBACK_NAME@ bind-dynamic interface=virbr1 diff --git a/tests/networkxml2confdata/routed-network.conf b/tests/networkxml2confdata/routed-network.conf index 970aa3cd3500..7f659b5a1c1e 100644 --- a/tests/networkxml2confdata/routed-network.conf +++ b/tests/networkxml2confdata/routed-network.conf @@ -5,7 +5,7 @@ ## ## dnsmasq conf file created by libvirt strict-order -except-interface=lo +except-interface=@LOOPBACK_NAME@ bind-dynamic interface=virbr1 addn-hosts=/var/lib/libvirt/dnsmasq/local.addnhosts diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 7ff243b98d20..8ff2c0d33d34 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -19,15 +19,25 @@ #define VIR_FROM_THIS VIR_FROM_NONE static int -testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr caps) +testCompareXMLToConfFiles(const char *inxml, + const char *outconf, + dnsmasqCapsPtr caps) { char *actual = NULL; + char *expected = NULL; int ret = -1; virNetworkDefPtr dev = NULL; virNetworkObjPtr obj = NULL; virCommandPtr cmd = NULL; char *pidfile = NULL; dnsmasqContext *dctx = NULL; + const char *loopback_name = "lo"; + const char *loopback_placeholder = "@LOOPBACK_NAME@"; + char *tmp = NULL; + +#ifndef __linux__ + loname = "lo0"; +#endif if (!(dev = virNetworkDefParseFile(inxml))) goto fail; @@ -45,13 +55,40 @@ testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr dctx, caps) < 0) goto fail; - if (virTestCompareToFile(actual, outconf) < 0) + /* Regeneration option is sacrificed so that we can have one file for both + * Linux and non-Linux outputs */ + if (virTestLoadFile(outconf, &expected) < 0) goto fail; + tmp = strstr(expected, loopback_placeholder); + if (tmp) { + size_t placeholder_len = strlen(loopback_placeholder); + size_t loname_len = strlen(loopback_name); + + if (loname_len > placeholder_len) { + fprintf(stderr, "%s", "Increase the loopback placeholder size"); + goto fail; + } + + if (!virStrncpy(tmp, loopback_name, strlen(loopback_name), strlen(tmp))) + goto fail; + + memmove(tmp + loname_len, tmp + placeholder_len, + strlen(tmp + placeholder_len) + 1); + } + + if (STRNEQ_NULLABLE(actual, expected)) { + virTestDifferenceFullNoRegenerate(stderr, + expected, outconf, + actual, NULL); + goto fail; + } + ret = 0; fail: VIR_FREE(actual); + VIR_FREE(expected); VIR_FREE(pidfile); virCommandFree(cmd); virObjectUnref(obj); -- 2.11.0
Martin Kletzander wrote:
Yet another way to fix the different loopback naming. This variant doesn't use multiple files, allows easy addition of more loopback names and factoring out the naming code to a function (which would programmatically get the loopback interface name) is very easy. OTOH we lose a way to regenerate the files (well, the *easy* way).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
Notes: The build is *NOT* tested on any non-Linux patch after this change. I feel like it's easier for someone else to try it out than for me to spin up a new VM, install FreeBSD, figure out what's needed for building libvirt there and all that during the holidays.
This sounds like a process with a very high entertainment value. :-) Unfortunately, I have a FreeBSD installation already, so... spoiler alert, test results are below.
So please, if someone is bored out of their mind, feel free to test it out ;)
tests/networkxml2confdata/dhcp6-nat-network.conf | 2 +- tests/networkxml2confdata/dhcp6-network.conf | 2 +- .../dhcp6host-routed-network.conf | 2 +- tests/networkxml2confdata/isolated-network.conf | 2 +- .../nat-network-dns-forward-plain.conf | 2 +- .../nat-network-dns-forwarders.conf | 2 +- .../networkxml2confdata/nat-network-dns-hosts.conf | 2 +- .../nat-network-dns-local-domain.conf | 2 +- .../nat-network-dns-srv-record-minimal.conf | 2 +- .../nat-network-dns-srv-record.conf | 2 +- .../nat-network-dns-txt-record.conf | 2 +- .../nat-network-name-with-quotes.conf | 2 +- tests/networkxml2confdata/nat-network.conf | 2 +- tests/networkxml2confdata/netboot-network.conf | 2 +- .../networkxml2confdata/netboot-proxy-network.conf | 2 +- tests/networkxml2confdata/open-network.conf | 2 +- tests/networkxml2confdata/ptr-domains-auto.conf | 2 +- .../networkxml2confdata/routed-network-no-dns.conf | 2 +- tests/networkxml2confdata/routed-network.conf | 2 +- tests/networkxml2conftest.c | 41 ++++++++++++++++++++-- 20 files changed, 58 insertions(+), 21 deletions(-)
diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 7ff243b98d20..8ff2c0d33d34 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -19,15 +19,25 @@ #define VIR_FROM_THIS VIR_FROM_NONE
static int -testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr caps) +testCompareXMLToConfFiles(const char *inxml, + const char *outconf, + dnsmasqCapsPtr caps) { char *actual = NULL; + char *expected = NULL; int ret = -1; virNetworkDefPtr dev = NULL; virNetworkObjPtr obj = NULL; virCommandPtr cmd = NULL; char *pidfile = NULL; dnsmasqContext *dctx = NULL; + const char *loopback_name = "lo"; + const char *loopback_placeholder = "@LOOPBACK_NAME@"; + char *tmp = NULL; + +#ifndef __linux__ + loname = "lo0";
^^^^^^ should be 'loopback_name'. After that I have networkxml2conftest runs smoothly for me.
+#endif
if (!(dev = virNetworkDefParseFile(inxml))) goto fail; @@ -45,13 +55,40 @@ testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr dctx, caps) < 0) goto fail;
- if (virTestCompareToFile(actual, outconf) < 0) + /* Regeneration option is sacrificed so that we can have one file for both + * Linux and non-Linux outputs */ + if (virTestLoadFile(outconf, &expected) < 0) goto fail;
+ tmp = strstr(expected, loopback_placeholder); + if (tmp) { + size_t placeholder_len = strlen(loopback_placeholder); + size_t loname_len = strlen(loopback_name); + + if (loname_len > placeholder_len) { + fprintf(stderr, "%s", "Increase the loopback placeholder size"); + goto fail; + } + + if (!virStrncpy(tmp, loopback_name, strlen(loopback_name), strlen(tmp))) + goto fail; + + memmove(tmp + loname_len, tmp + placeholder_len, + strlen(tmp + placeholder_len) + 1); + } + + if (STRNEQ_NULLABLE(actual, expected)) { + virTestDifferenceFullNoRegenerate(stderr, + expected, outconf, + actual, NULL); + goto fail; + } + ret = 0;
fail: VIR_FREE(actual); + VIR_FREE(expected); VIR_FREE(pidfile); virCommandFree(cmd); virObjectUnref(obj);
Out of curiosity, can we use virStringReplace() here? I still have this working with the following change (along with the loname fix): diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 8ff2c0d33..789b5100c 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -24,6 +24,7 @@ testCompareXMLToConfFiles(const char *inxml, dnsmasqCapsPtr caps) { char *actual = NULL; + char *template = NULL; char *expected = NULL; int ret = -1; virNetworkDefPtr dev = NULL; @@ -33,10 +34,9 @@ testCompareXMLToConfFiles(const char *inxml, dnsmasqContext *dctx = NULL; const char *loopback_name = "lo"; const char *loopback_placeholder = "@LOOPBACK_NAME@"; - char *tmp = NULL; #ifndef __linux__ - loname = "lo0"; + loopback_name = "lo0"; #endif if (!(dev = virNetworkDefParseFile(inxml))) @@ -57,25 +57,13 @@ testCompareXMLToConfFiles(const char *inxml, /* Regeneration option is sacrificed so that we can have one file for both * Linux and non-Linux outputs */ - if (virTestLoadFile(outconf, &expected) < 0) + if (virTestLoadFile(outconf, &template) < 0) goto fail; - tmp = strstr(expected, loopback_placeholder); - if (tmp) { - size_t placeholder_len = strlen(loopback_placeholder); - size_t loname_len = strlen(loopback_name); - - if (loname_len > placeholder_len) { - fprintf(stderr, "%s", "Increase the loopback placeholder size"); - goto fail; - } - - if (!virStrncpy(tmp, loopback_name, strlen(loopback_name), strlen(tmp))) - goto fail; - - memmove(tmp + loname_len, tmp + placeholder_len, - strlen(tmp + placeholder_len) + 1); - } + if (!(expected = virStringReplace(template, + loopback_placeholder, + loopback_name))) + goto fail; if (STRNEQ_NULLABLE(actual, expected)) { virTestDifferenceFullNoRegenerate(stderr, @@ -88,6 +76,7 @@ testCompareXMLToConfFiles(const char *inxml, fail: VIR_FREE(actual); + VIR_FREE(template); VIR_FREE(expected); VIR_FREE(pidfile); virCommandFree(cmd); Roman Bogorodskiy
On Fri, Dec 30, 2016 at 04:00:21PM +0400, Roman Bogorodskiy wrote:
Martin Kletzander wrote:
Yet another way to fix the different loopback naming. This variant doesn't use multiple files, allows easy addition of more loopback names and factoring out the naming code to a function (which would programmatically get the loopback interface name) is very easy. OTOH we lose a way to regenerate the files (well, the *easy* way).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
Notes: The build is *NOT* tested on any non-Linux patch after this change. I feel like it's easier for someone else to try it out than for me to spin up a new VM, install FreeBSD, figure out what's needed for building libvirt there and all that during the holidays.
This sounds like a process with a very high entertainment value. :-)
I know, it's not like I haven't done that few times =D
Unfortunately, I have a FreeBSD installation already, so... spoiler alert, test results are below.
So please, if someone is bored out of their mind, feel free to test it out ;)
tests/networkxml2confdata/dhcp6-nat-network.conf | 2 +- tests/networkxml2confdata/dhcp6-network.conf | 2 +- .../dhcp6host-routed-network.conf | 2 +- tests/networkxml2confdata/isolated-network.conf | 2 +- .../nat-network-dns-forward-plain.conf | 2 +- .../nat-network-dns-forwarders.conf | 2 +- .../networkxml2confdata/nat-network-dns-hosts.conf | 2 +- .../nat-network-dns-local-domain.conf | 2 +- .../nat-network-dns-srv-record-minimal.conf | 2 +- .../nat-network-dns-srv-record.conf | 2 +- .../nat-network-dns-txt-record.conf | 2 +- .../nat-network-name-with-quotes.conf | 2 +- tests/networkxml2confdata/nat-network.conf | 2 +- tests/networkxml2confdata/netboot-network.conf | 2 +- .../networkxml2confdata/netboot-proxy-network.conf | 2 +- tests/networkxml2confdata/open-network.conf | 2 +- tests/networkxml2confdata/ptr-domains-auto.conf | 2 +- .../networkxml2confdata/routed-network-no-dns.conf | 2 +- tests/networkxml2confdata/routed-network.conf | 2 +- tests/networkxml2conftest.c | 41 ++++++++++++++++++++-- 20 files changed, 58 insertions(+), 21 deletions(-)
diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 7ff243b98d20..8ff2c0d33d34 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -19,15 +19,25 @@ #define VIR_FROM_THIS VIR_FROM_NONE
static int -testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr caps) +testCompareXMLToConfFiles(const char *inxml, + const char *outconf, + dnsmasqCapsPtr caps) { char *actual = NULL; + char *expected = NULL; int ret = -1; virNetworkDefPtr dev = NULL; virNetworkObjPtr obj = NULL; virCommandPtr cmd = NULL; char *pidfile = NULL; dnsmasqContext *dctx = NULL; + const char *loopback_name = "lo"; + const char *loopback_placeholder = "@LOOPBACK_NAME@"; + char *tmp = NULL; + +#ifndef __linux__ + loname = "lo0";
^^^^^^ should be 'loopback_name'.
🤦 so few rounds of refactoring will not be hidden under the covers. OK, OK, I admit I didn't write it in one go =D
After that I have networkxml2conftest runs smoothly for me.
Great!
+#endif
if (!(dev = virNetworkDefParseFile(inxml))) goto fail; @@ -45,13 +55,40 @@ testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr dctx, caps) < 0) goto fail;
- if (virTestCompareToFile(actual, outconf) < 0) + /* Regeneration option is sacrificed so that we can have one file for both + * Linux and non-Linux outputs */ + if (virTestLoadFile(outconf, &expected) < 0) goto fail;
+ tmp = strstr(expected, loopback_placeholder); + if (tmp) { + size_t placeholder_len = strlen(loopback_placeholder); + size_t loname_len = strlen(loopback_name); + + if (loname_len > placeholder_len) { + fprintf(stderr, "%s", "Increase the loopback placeholder size"); + goto fail; + } + + if (!virStrncpy(tmp, loopback_name, strlen(loopback_name), strlen(tmp))) + goto fail; + + memmove(tmp + loname_len, tmp + placeholder_len, + strlen(tmp + placeholder_len) + 1); + } + + if (STRNEQ_NULLABLE(actual, expected)) { + virTestDifferenceFullNoRegenerate(stderr, + expected, outconf, + actual, NULL); + goto fail; + } + ret = 0;
fail: VIR_FREE(actual); + VIR_FREE(expected); VIR_FREE(pidfile); virCommandFree(cmd); virObjectUnref(obj);
Out of curiosity, can we use virStringReplace() here? I still have this
8-0 We have that? I feel like I should've known... Or at least should've tried looking for it.
working with the following change (along with the loname fix):
Perfect. ACK to your version if you're OK with it as well (the only ACKs I've found in this mail are in the 'LOOPBACK' placeholder ;)), then feel free to push it since you have the working commit handy. Martin
On Fri, Dec 30, 2016 at 05:06:40PM +0100, Martin Kletzander wrote:
On Fri, Dec 30, 2016 at 04:00:21PM +0400, Roman Bogorodskiy wrote:
Martin Kletzander wrote:
Yet another way to fix the different loopback naming. This variant doesn't use multiple files, allows easy addition of more loopback names and factoring out the naming code to a function (which would programmatically get the loopback interface name) is very easy. OTOH we lose a way to regenerate the files (well, the *easy* way).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
Notes: The build is *NOT* tested on any non-Linux patch after this change. I feel like it's easier for someone else to try it out than for me to spin up a new VM, install FreeBSD, figure out what's needed for building libvirt there and all that during the holidays.
This sounds like a process with a very high entertainment value. :-)
I know, it's not like I haven't done that few times =D
Unfortunately, I have a FreeBSD installation already, so... spoiler alert, test results are below.
So please, if someone is bored out of their mind, feel free to test it out ;)
tests/networkxml2confdata/dhcp6-nat-network.conf | 2 +- tests/networkxml2confdata/dhcp6-network.conf | 2 +- .../dhcp6host-routed-network.conf | 2 +- tests/networkxml2confdata/isolated-network.conf | 2 +- .../nat-network-dns-forward-plain.conf | 2 +- .../nat-network-dns-forwarders.conf | 2 +- .../networkxml2confdata/nat-network-dns-hosts.conf | 2 +- .../nat-network-dns-local-domain.conf | 2 +- .../nat-network-dns-srv-record-minimal.conf | 2 +- .../nat-network-dns-srv-record.conf | 2 +- .../nat-network-dns-txt-record.conf | 2 +- .../nat-network-name-with-quotes.conf | 2 +- tests/networkxml2confdata/nat-network.conf | 2 +- tests/networkxml2confdata/netboot-network.conf | 2 +- .../networkxml2confdata/netboot-proxy-network.conf | 2 +- tests/networkxml2confdata/open-network.conf | 2 +- tests/networkxml2confdata/ptr-domains-auto.conf | 2 +- .../networkxml2confdata/routed-network-no-dns.conf | 2 +- tests/networkxml2confdata/routed-network.conf | 2 +- tests/networkxml2conftest.c | 41 ++++++++++++++++++++-- 20 files changed, 58 insertions(+), 21 deletions(-)
diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 7ff243b98d20..8ff2c0d33d34 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -19,15 +19,25 @@ #define VIR_FROM_THIS VIR_FROM_NONE
static int -testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr caps) +testCompareXMLToConfFiles(const char *inxml, + const char *outconf, + dnsmasqCapsPtr caps) { char *actual = NULL; + char *expected = NULL; int ret = -1; virNetworkDefPtr dev = NULL; virNetworkObjPtr obj = NULL; virCommandPtr cmd = NULL; char *pidfile = NULL; dnsmasqContext *dctx = NULL; + const char *loopback_name = "lo"; + const char *loopback_placeholder = "@LOOPBACK_NAME@"; + char *tmp = NULL; + +#ifndef __linux__ + loname = "lo0";
^^^^^^ should be 'loopback_name'.
🤦 so few rounds of refactoring will not be hidden under the covers. OK, OK, I admit I didn't write it in one go =D
After that I have networkxml2conftest runs smoothly for me.
Great!
+#endif
if (!(dev = virNetworkDefParseFile(inxml))) goto fail; @@ -45,13 +55,40 @@ testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr dctx, caps) < 0) goto fail;
- if (virTestCompareToFile(actual, outconf) < 0) + /* Regeneration option is sacrificed so that we can have one file for both + * Linux and non-Linux outputs */ + if (virTestLoadFile(outconf, &expected) < 0) goto fail;
+ tmp = strstr(expected, loopback_placeholder); + if (tmp) { + size_t placeholder_len = strlen(loopback_placeholder); + size_t loname_len = strlen(loopback_name); + + if (loname_len > placeholder_len) { + fprintf(stderr, "%s", "Increase the loopback placeholder size"); + goto fail; + } + + if (!virStrncpy(tmp, loopback_name, strlen(loopback_name), strlen(tmp))) + goto fail; + + memmove(tmp + loname_len, tmp + placeholder_len, + strlen(tmp + placeholder_len) + 1); + } + + if (STRNEQ_NULLABLE(actual, expected)) { + virTestDifferenceFullNoRegenerate(stderr, + expected, outconf, + actual, NULL); + goto fail; + } + ret = 0;
fail: VIR_FREE(actual); + VIR_FREE(expected); VIR_FREE(pidfile); virCommandFree(cmd); virObjectUnref(obj);
Out of curiosity, can we use virStringReplace() here? I still have this
8-0 We have that? I feel like I should've known... Or at least should've tried looking for it.
working with the following change (along with the loname fix):
Perfect. ACK to your version if you're OK with it as well (the only ACKs I've found in this mail are in the 'LOOPBACK' placeholder ;)), then feel free to push it since you have the working commit handy.
No rush, I see Michal has yet another proposal for this that we haven't considered and even though there are somedrawbacks to that as well, it looks nicer than this. After all the ideas I'm starting to like the "gross" one the best. Oh my =)
Martin
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Martin Kletzander wrote:
On Fri, Dec 30, 2016 at 05:06:40PM +0100, Martin Kletzander wrote:
On Fri, Dec 30, 2016 at 04:00:21PM +0400, Roman Bogorodskiy wrote:
Martin Kletzander wrote:
Yet another way to fix the different loopback naming. This variant doesn't use multiple files, allows easy addition of more loopback names and factoring out the naming code to a function (which would programmatically get the loopback interface name) is very easy. OTOH we lose a way to regenerate the files (well, the *easy* way).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
Notes: The build is *NOT* tested on any non-Linux patch after this change. I feel like it's easier for someone else to try it out than for me to spin up a new VM, install FreeBSD, figure out what's needed for building libvirt there and all that during the holidays.
This sounds like a process with a very high entertainment value. :-)
I know, it's not like I haven't done that few times =D
Unfortunately, I have a FreeBSD installation already, so... spoiler alert, test results are below.
So please, if someone is bored out of their mind, feel free to test it out ;)
tests/networkxml2confdata/dhcp6-nat-network.conf | 2 +- tests/networkxml2confdata/dhcp6-network.conf | 2 +- .../dhcp6host-routed-network.conf | 2 +- tests/networkxml2confdata/isolated-network.conf | 2 +- .../nat-network-dns-forward-plain.conf | 2 +- .../nat-network-dns-forwarders.conf | 2 +- .../networkxml2confdata/nat-network-dns-hosts.conf | 2 +- .../nat-network-dns-local-domain.conf | 2 +- .../nat-network-dns-srv-record-minimal.conf | 2 +- .../nat-network-dns-srv-record.conf | 2 +- .../nat-network-dns-txt-record.conf | 2 +- .../nat-network-name-with-quotes.conf | 2 +- tests/networkxml2confdata/nat-network.conf | 2 +- tests/networkxml2confdata/netboot-network.conf | 2 +- .../networkxml2confdata/netboot-proxy-network.conf | 2 +- tests/networkxml2confdata/open-network.conf | 2 +- tests/networkxml2confdata/ptr-domains-auto.conf | 2 +- .../networkxml2confdata/routed-network-no-dns.conf | 2 +- tests/networkxml2confdata/routed-network.conf | 2 +- tests/networkxml2conftest.c | 41 ++++++++++++++++++++-- 20 files changed, 58 insertions(+), 21 deletions(-)
diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 7ff243b98d20..8ff2c0d33d34 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -19,15 +19,25 @@ #define VIR_FROM_THIS VIR_FROM_NONE
static int -testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr caps) +testCompareXMLToConfFiles(const char *inxml, + const char *outconf, + dnsmasqCapsPtr caps) { char *actual = NULL; + char *expected = NULL; int ret = -1; virNetworkDefPtr dev = NULL; virNetworkObjPtr obj = NULL; virCommandPtr cmd = NULL; char *pidfile = NULL; dnsmasqContext *dctx = NULL; + const char *loopback_name = "lo"; + const char *loopback_placeholder = "@LOOPBACK_NAME@"; + char *tmp = NULL; + +#ifndef __linux__ + loname = "lo0";
^^^^^^ should be 'loopback_name'.
🤦 so few rounds of refactoring will not be hidden under the covers. OK, OK, I admit I didn't write it in one go =D
After that I have networkxml2conftest runs smoothly for me.
Great!
+#endif
if (!(dev = virNetworkDefParseFile(inxml))) goto fail; @@ -45,13 +55,40 @@ testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr dctx, caps) < 0) goto fail;
- if (virTestCompareToFile(actual, outconf) < 0) + /* Regeneration option is sacrificed so that we can have one file for both + * Linux and non-Linux outputs */ + if (virTestLoadFile(outconf, &expected) < 0) goto fail;
+ tmp = strstr(expected, loopback_placeholder); + if (tmp) { + size_t placeholder_len = strlen(loopback_placeholder); + size_t loname_len = strlen(loopback_name); + + if (loname_len > placeholder_len) { + fprintf(stderr, "%s", "Increase the loopback placeholder size"); + goto fail; + } + + if (!virStrncpy(tmp, loopback_name, strlen(loopback_name), strlen(tmp))) + goto fail; + + memmove(tmp + loname_len, tmp + placeholder_len, + strlen(tmp + placeholder_len) + 1); + } + + if (STRNEQ_NULLABLE(actual, expected)) { + virTestDifferenceFullNoRegenerate(stderr, + expected, outconf, + actual, NULL); + goto fail; + } + ret = 0;
fail: VIR_FREE(actual); + VIR_FREE(expected); VIR_FREE(pidfile); virCommandFree(cmd); virObjectUnref(obj);
Out of curiosity, can we use virStringReplace() here? I still have this
8-0 We have that? I feel like I should've known... Or at least should've tried looking for it.
working with the following change (along with the loname fix):
Perfect. ACK to your version if you're OK with it as well (the only ACKs I've found in this mail are in the 'LOOPBACK' placeholder ;)), then feel free to push it since you have the working commit handy.
No rush, I see Michal has yet another proposal for this that we haven't considered and even though there are somedrawbacks to that as well, it looks nicer than this.
After all the ideas I'm starting to like the "gross" one the best. Oh my =)
Martin
My vote still goes to this solution, because having a placeholder seems more explicit and easier to follow than doing s/lo/lo0/ directly. Roman Bogorodskiy
On Sun, 2017-01-01 at 12:35 +0400, Roman Bogorodskiy wrote:
No rush, I see Michal has yet another proposal for this that we haven't considered and even though there are somedrawbacks to that as well, it looks nicer than this. After all the ideas I'm starting to like the "gross" one the best. Oh my =) My vote still goes to this solution, because having a placeholder seems more explicit and easier to follow than doing s/lo/lo0/ directly.
I vote for Michal's approach as it doesn't require us to disable VIR_TEST_REGENERATE_OUTPUT. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Jan 02, 2017 at 09:57:04AM +0100, Andrea Bolognani wrote:
On Sun, 2017-01-01 at 12:35 +0400, Roman Bogorodskiy wrote:
No rush, I see Michal has yet another proposal for this that we haven't considered and even though there are somedrawbacks to that as well, it looks nicer than this. After all the ideas I'm starting to like the "gross" one the best. Oh my =) My vote still goes to this solution, because having a placeholder seems more explicit and easier to follow than doing s/lo/lo0/ directly.
I vote for Michal's approach as it doesn't require us to disable VIR_TEST_REGENERATE_OUTPUT.
Well, it does. Kinda. You *must not* regenerate output on FreeBSD with his patch, so it should be explicitly disabled. I can't make up my mind, currently I'm inclining to your solution with multiple files, so I'll let you guys decide.
-- Andrea Bolognani / Red Hat / Virtualization
On 02.01.2017 12:04, Martin Kletzander wrote:
On Mon, Jan 02, 2017 at 09:57:04AM +0100, Andrea Bolognani wrote:
On Sun, 2017-01-01 at 12:35 +0400, Roman Bogorodskiy wrote:
No rush, I see Michal has yet another proposal for this that we haven't considered and even though there are somedrawbacks to that as well, it looks nicer than this.
After all the ideas I'm starting to like the "gross" one the best. Oh my =)
My vote still goes to this solution, because having a placeholder seems more explicit and easier to follow than doing s/lo/lo0/ directly.
I vote for Michal's approach as it doesn't require us to disable VIR_TEST_REGENERATE_OUTPUT.
Well, it does. Kinda. You *must not* regenerate output on FreeBSD with his patch, so it should be explicitly disabled.
Really? I think it works well even if you do regenerate output there. I mean, my patch fixes the output of the actual configuration, so that it will always contain 'lo' instead of 'lo0'. And test output regeneration is done after that. With 'lo'. Michal
On Mon, 2017-01-02 at 13:10 +0100, Michal Privoznik wrote:
I vote for Michal's approach as it doesn't require us to disable VIR_TEST_REGENERATE_OUTPUT.
Well, it does. Kinda. You *must not* regenerate output on FreeBSD with his patch, so it should be explicitly disabled.
Really? I think it works well even if you do regenerate output there. I mean, my patch fixes the output of the actual configuration, so that it will always contain 'lo' instead of 'lo0'. And test output regeneration is done after that. With 'lo'.
I just checked and indeed, with Michal's patches that have since been pushed, VIR_TEST_REGENERATE_OUTPUT works perfectly fine even when run on FreeBSD. -- Andrea Bolognani / Red Hat / Virtualization
participants (4)
-
Andrea Bolognani -
Martin Kletzander -
Michal Privoznik -
Roman Bogorodskiy