Pushed branch bg/device-creation-vxlan-bgo756963, please review.

Looks all good to me.

However, iproute2 seems to support more properties; I'm wondering why you don't:
remote, dstport, srcport, maxaddress and gbp.

(In reply to Lubomir Rintel from comment #2)
> Looks all good to me.
>
> However, iproute2 seems to support more properties; I'm wondering why you
> don't:
> remote,

iproute2 offers two distinct options to set 'group' and 'remote' but
from a netlink/kernel point of view there is a single attribute
'group' that contains one or the other. So I think it makes sense to
have only one property ('vxlan.group') which specifies the multicast
group or the unicast remote address.

> dstport, srcport, maxaddress and gbp.

The iproute2 version I was using didn't have dstport, maxaddresses
and gbp. They seem useful, I will add them.

>> libnm: add NMSettingVxlan
    

I thought that the name "remote" makes more sense the "group" (given that we combine them).


+         g_set_error_literal (error,
+                      NM_CONNECTION_ERROR,
+                      NM_C

whitespace




Is the parent property redundant to master? Maybe then the vxlan setting should not have it's own parent property, but reuse master.

This question will also pop up for gre again, maybe others. I think the parent should be consolidated with the master property there.

Do you intend the vxlan setting to be a slave-type? Then you'd also have to extend _nm_setting_slave_type_is_valid() and _nm_connection_detect_slave_type().


>> ifcfg-rh: add VXLAN support

Regarding ifcfg-rh support... I commented on the tun branch that it was missing there. I merely asked whether we really should add it. I think adding ifcfg-rh support *only* makes sense if initscripts also support such a connection type. Do initscripts support vxlan tunnels (Beyond placing a plain `ip link add` command there).
Otherwise, it seems that keyfile is preferable in every way.



>> platform: add vxlan support

+NMPlatformError nm_platform_vxlan_add (NMPlatform *self, const char *name, NMPlatformLnkVxlan *props, NMPlatformLink *out_link);
+

all our other platform-add commands accept the arguments individually instead of accepting a NMPlatform* struct. Eg. nm_platform_ip4_route_add().

Hm, ok.



>> device/vxlan: support device creation


inet_ntop (AF_INET6, &priv->props.group6, str, sizeof (str)

nm_utils_inet6_ntop()?



When I compare vxlan's update_connection() to the vlan implementation, they behave different. The vlan implementation looks up the platform settings and updates priv->props. The vxlan implementation misses that step. Seems one of them needs fixing.




Regarding the NMDeviceVxlan implementation for libnm-glib, I think again that it would be better just to create an shallow generic device for it to preserve backward behavior.

(In reply to Thomas Haller from comment #4)
> >> libnm: add NMSettingVxlan
> I thought that the name "remote" makes more sense the "group" (given that we
> combine them).

Fixed. But note that NMDeviceVxlan is still exporting via D-Bus
".Group", we can't change the D-Bus API. Other properties that differ
are:

NMSettingVxlan:                         NMDeviceVxlan:
vxlan.remote                           .Group
vxlan.source-port-min                  .SrcPortMin
vxlan.source-port-max                  .SrcPortMax
vxlan.destination-port                 .DstPort

> Is the parent property redundant to master? Maybe then the vxlan setting
> should not have it's own parent property, but reuse master.

> Do you intend the vxlan setting to be a slave-type? Then you'd also have to
> extend _nm_setting_slave_type_is_valid() and
> _nm_connection_detect_slave_type().

This is only for specifying a link parent device. There is no explicit
support for master-slave relationships at the moment (is it needed)?

> >> ifcfg-rh: add VXLAN support
>
> Regarding ifcfg-rh support... I commented on the tun branch that it was
> missing there. I merely asked whether we really should add it. I think
> adding ifcfg-rh support *only* makes sense if initscripts also support such
> a connection type. Do initscripts support vxlan tunnels (Beyond placing a
> plain `ip link add` command there).
> Otherwise, it seems that keyfile is preferable in every way.

Ok, dropped ifcfg-rh support.

> all our other platform-add commands accept the arguments individually
> instead of accepting a NMPlatform* struct. Eg. nm_platform_ip4_route_add().
>
> Hm, ok.

Vxlans have a lot of parameters, IMO it makes sense to use a struct to
pass them.

> When I compare vxlan's update_connection() to the vlan implementation, they
> behave different. The vlan implementation looks up the platform settings and
> updates priv->props. The vxlan implementation misses that step. Seems one of
> them needs fixing.

Now vxlan implementation updates platform settings too.

> Regarding the NMDeviceVxlan implementation for libnm-glib, I think again
> that it would be better just to create an shallow generic device for it to
> preserve backward behavior.

Makes sense. Repushed branch bg/device-creation-vxlan-bgo756963.

Rebased on master.

nmtstp_link_vxlan_add() must wait until platform sees the changes.

see nmtstp_link_gre_add():
  nmtstp_assert_wait_for_link (name, NM_LINK_TYPE_GRE, 100);


- inet_pton (AF_INET, "23.1.2.164", &lnk_vxlan.local);
+ lnk_vxlan.local = nmtst_inet4_from_string ("23.1.2.164");



>> libnm-glib: consider unknown devices as generic ones
    
          g_warning ("Unknown device type %d", dtype);
-         return G_TYPE_INVALID;
+         return NM_TYPE_DEVICE_GENERIC;


maybe even drop the warning? (or make it g_debug() only).

It's well understood that there are device-types that libnm-glib doesn't understand. Printing a message to stdout doesn't help much.




Rest LGTM

(In reply to Thomas Haller from comment #7)

Fixed, thanks.

Merged to master:

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