Pushed branch bg/device-creation-ip-tunnel-bgo758047, please comment/ack/nack the approach of defining a single device/setting type for all tunnel types. In my opinion this makes sense since it avoids the proliferation of objects which would be very similar to each others.

Sounds right to have one setting/device-type only.




+typedef enum {
+    NM_IP_TUNNEL_MODE_IPIP        = 0,
+    NM_IP_TUNNEL_MODE_GRE         = 1,
+    NM_IP_TUNNEL_MODE_SIT         = 2,
+    NM_IP_TUNNEL_MODE_ISATAP      = 3,
+    NM_IP_TUNNEL_MODE_VTI         = 4,
+    NM_IP_TUNNEL_MODE_IP6IP6      = 5,
+    NM_IP_TUNNEL_MODE_IPIP6       = 6,
+    NM_IP_TUNNEL_MODE_IP6GRE      = 7,
+    NM_IP_TUNNEL_MODE_VTI6        = 8,
+} NMIPTunnelMode;



0 should be a reserved value (UNKNOWN/NONE).




settings.c:1674:23: error: unused variable 'values' [-Werror,-Wunused-variable]
        gs_free const char **values = NULL;
                             ^
settings.c:1675:16: error: unused variable 'options' [-Werror,-Wunused-variable]
        gs_free char *options = NULL;



nm_platform_gre_add(): can we name link-related functions nm_platform_link_*()?
Yes, we didn't do that (and I consider that wrong). I did change the naming scheme for nm_platform_link_vlan_change() though, and I think new function should get the ~right~ name.
One day, we might even do a renaming of the old function, but let's not add new functions with this IMO wrong naming scheme.




+    g_return_val_if_fail (klass->gre_add, NM_PLATFORM_ERROR_BUG);

These runtime checks against "klass" make little sense. Yes, we have them all over the place, but there are only two implementations of NMPlatform (linux and fake). NMLinuxPlatform by definition implements all virtual functions and this runtime check must never fail (if it does, just let it crash later on).

If you really want, nm_assert (klass->gre_add); is better IMO.



 nla_put_failure:
+g_return_val_if_reached (FALSE);
+}

whitespace.




Could we have a test for nm_platform_gre_add()?
See how test_software_detect() already creates a gre-tunnel via ip-link.
Let's add a nmtstp_link_gre_add() function, which is analog to nmtstp_ip4_address_add().


>> device: add NMDeviceIPTunnel



+         g_clear_pointer (&priv->output_key, g_free);
+         if (NM_FLAGS_HAS (lnk->output_flags, GRE_KEY))
+              priv->output_key = g_strdup_printf ("%u", ntohl (lnk->output_key));

I think that NMPlatformLnkGre's key-field should do th ntohl() as appropriate (during parsing and serialization of netlink messages).






>> platform: add basic SIT support

you sorted all new code related to lnk_sit after lnk_gre. That makes some sense (as they both are tunnels). Previously, they were all sorted by name though. I would move the new code between MACVLAN and VLAN.

Let's add a SIT test to test_software_detect()?

ps: could you rebase on master?

Rebased on master and fixed all the points above.

Added fixups.

Looks good to me now

Squashed fixups, added support for IPv6 tunnels, added missing test cases for new link types and repushed branch bg/device-creation-ip-tunnel-bgo758047.

>> libnm-glib: add support for IP tunnel devices as generic ones


As a follow-up commit to:

     case NM_DEVICE_TYPE_GENERIC:
     case NM_DEVICE_TYPE_TUN:
+    case NM_DEVICE_TYPE_IP_TUNNEL:
          return NM_TYPE_DEVICE_GENERIC;
     default:
          g_warning ("Unknown device type %d", dtype);
          return G_TYPE_INVALID;
     }


maybe now we should always create a generic device for an unknown device-type. That allows us not to backport all future enums and it will just work.



>> device/ip-tunnel: add support for IP6TNL tunnels


+         memcpy (&local6, &lnk->local, sizeof (local6));
+         memcpy (&remote6, &lnk->remote, sizeof (remote6));

nitpick:

remote6 = lnk->local

is typesafe.





rest LGTM

(In reply to Thomas Haller from comment #7)
> >> libnm-glib: add support for IP tunnel devices as generic ones
> 
> maybe now we should always create a generic device for an unknown
> device-type. That allows us not to backport all future enums and it will
> just work.

I'll include a a patch in the next upcoming branch for vxlans.

> >> device/ip-tunnel: add support for IP6TNL tunnels
>
> remote6 = lnk->local
> 
> is typesafe.

Fixed and merged to master:

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=193bf9291831729f0f75e018104423ffa4e5f8aa