2:35 a.m.
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(a)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