> libnm-util: add private header file nm-test-utils.h

> This is intended to contain utility functions for tests. It will
> be header only (containing inline functions).

Why? That just causes you pain. If you had nm-test-utils.c, then you could get rid of NMTST_DEFINE and __nmtst_internal, and just have nmtst_get_rand0() return a static GRand.

Although, nmtst_get_rand0() doesn't seem like a great idea anyway. It's better to let a different random seed get used each time, so that if there are subtle edge case bugs in your code, *eventually* someone will hit it in testing (and the test harness prints out the random seed used, so you can re-run it with the same seed again later).

>+/*
>+ * Dan Williams <dcbw@redhat.com>
>+ *

I assume that's a cut+paste-o (as opposed to this being code dcbw wrote originally)?


> core: sort IPv6 addresses (add nm_ip6_config_addresses_sort())

V4COMPAT is deprecated, and I don't think it's possible to assign a V4MAPPED address to an interface. So I'd rank those lower.

And I'd rank LOOPBACK higher; if for some reason you have some other address on lo as well, you still want to see ::1 first.


It would simplify things to move the data_pre/memcmp stuff into nm_ip6_config_addresses_sort() rather than _addresses_sort().

(In reply to comment #1)
> > libnm-util: add private header file nm-test-utils.h
> 
> > This is intended to contain utility functions for tests. It will
> > be header only (containing inline functions).
> 
> Why? That just causes you pain. If you had nm-test-utils.c, then you could get
> rid of NMTST_DEFINE and __nmtst_internal, and just have nmtst_get_rand0()
> return a static GRand.

You mean, why header-only? That way, all you need is three lines:

+#include "nm-test-utils.h"

+NMTST_DEFINE();

-    g_test_init (&argc, &argv, NULL);
-
-    g_type_init ();
+    nmtst_init (&argc, &argv);


By having it not header-only, you save only the NMTST_DEFINE() line -- nmtst_init() is still useful, because we can use it to setup our tests

e.g. we should do (configurable):
+    nm_logging_syslog_openlog (TRUE);
+
+    nm_logging_setup ("DEBUG", "ALL", NULL, NULL);


With having this functionality in individual object files, you have to specify them in Makefile.am. And more importantly, you need different version for libnm-util only, libnm-util+NM, and libnm-util+libnm-glib. Certainly that is much more cumbersome to use in every test.



> Although, nmtst_get_rand0() doesn't seem like a great idea anyway. It's better
> to let a different random seed get used each time, so that if there are subtle
> edge case bugs in your code, *eventually* someone will hit it in testing (and
> the test harness prints out the random seed used, so you can re-run it with the
> same seed again later).

I understand the consequences of choosing the seed fixed :), that's why I choose it on purpose to have *reproducible* tests. Hence the name get_rand0().

But I tend to agree with your point, so I added nmtst_get_rand(), which by default seeds randomly -- unless you set the environment variable NMTST_SEED_RAND=4711

Yes, I know that there is --seed in glib tests, but:
 - we call our tests from the makefile, so it's easier to specify the seed in the environment then as command line argument.
 - nmtst_init() prints the used seed, so if you have a test that fails, you can find out more easily the seed that was used.



> >+/*
> >+ * Dan Williams <dcbw@redhat.com>
> >+ *
> 
> I assume that's a cut+paste-o (as opposed to this being code dcbw wrote
> originally)?

I copy+pasted it intentionally, but if that is wrong... I removed it.


> > core: sort IPv6 addresses (add nm_ip6_config_addresses_sort())
> 
> V4COMPAT is deprecated, and I don't think it's possible to assign a V4MAPPED
> address to an interface. So I'd rank those lower.

Done.


> And I'd rank LOOPBACK higher; if for some reason you have some other address on
> lo as well, you still want to see ::1 first.

Higher effectively means: "highest"? Are you sure about that? Done it for now!



> It would simplify things to move the data_pre/memcmp stuff into
> nm_ip6_config_addresses_sort() rather than _addresses_sort().

Indeed!!

> libnm-util: add private header file nm-test-utils.h

+inline GRand *nmtst_get_rand0 (void);
+
+inline GRand *
+nmtst_get_rand0 ()

What's the reason to ahve the prototypes separate from the function?


> core: sort IPv6 addresses (add nm_ip6_config_addresses_sort())

+	ipv6_privacy1 = !!(a1->flags & (IFA_F_MANAGETEMPADDR | IFA_F_TEMPORARY));
+	ipv6_privacy2 = !!(a2->flags & (IFA_F_MANAGETEMPADDR | IFA_F_TEMPORARY));
+
+	if (ipv6_privacy1 || ipv6_privacy2) {
+		gboolean public1, public2;
+

Maybe lose the blank line here?

Also, for the use_temporary, instead of passing what should be 'const' by reference, couldn't we use GUINT_TO_POINTER() or GINT_TO_POINTER() depending on the enum's type?  (I forget whether it's int or uint)

(In reply to comment #3)
> > libnm-util: add private header file nm-test-utils.h
> 
> +inline GRand *nmtst_get_rand0 (void);
> +
> +inline GRand *
> +nmtst_get_rand0 ()
> 
> What's the reason to ahve the prototypes separate from the function?

If I define the function without separate function declaration, a compiler warning hits.

In file included from test-ip6-config.c:27:0:
../../libnm-util/nm-test-utils.h:65:1: error: no previous prototype for ‘nmtst_get_rand0’ [-Werror=missing-prototypes]
 nmtst_get_rand0 ()



>
> > core: sort IPv6 addresses (add nm_ip6_config_addresses_sort())
> 
> +    ipv6_privacy1 = !!(a1->flags & (IFA_F_MANAGETEMPADDR | IFA_F_TEMPORARY));
> +    ipv6_privacy2 = !!(a2->flags & (IFA_F_MANAGETEMPADDR | IFA_F_TEMPORARY));
> +
> +    if (ipv6_privacy1 || ipv6_privacy2) {
> +        gboolean public1, public2;
> +
> 
> Maybe lose the blank line here?

done.


> Also, for the use_temporary, instead of passing what should be 'const' by
> reference, couldn't we use GUINT_TO_POINTER() or GINT_TO_POINTER() depending on
> the enum's type?  (I forget whether it's int or uint)

well, for an enum that depends on whether there are negative values. And also the storage size (int vs. short)... I think this is a bug in the C language (at least you should be able to specify the underlying type explicitly). I changed it to GINT_TO_POINTER (it's NM internal anyway).


pushed to fixup! commits

Merged to master: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=e3eb7605bee05b9158da3456bc2616e9dfef6387