Re: [PATCH 3/3] networkxml2conftest: Fix build on BSD

Martin Kletzander <mkletzan@xxxxxxxxxx> · Sat, 31 Dec 2016 15:44:07 +0100

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@xxxxxxxxxx>
---

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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc

Description: Digital signature
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



