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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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

Attachment: signature.asc
Description: PGP signature

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux