+		case 256:
+			/* Some s390 CTC-type devices report 256 for the encapsulation type
+			 * for some reason, but we need to call them Ethernet too. FIXME: use
+			 * something other than interface name to detect CTC here.
+			 */
+			if (g_str_has_prefix (rtnl_link_get_name (rtnllink), "ctc"))
+				return_type (NM_LINK_TYPE_ETHERNET, "ethernet");
+			/* else fall through */

The else fall through looks confusing, I would slightly prefer to handle that code path explicitly.

> use nm_rtnl_link_parse_info_data() to get their data.

Is this going to be addressed in NMPlatform? NMFakePlatformLink should now be ready to carry any arguments that don't requre separate allocation. I'm going to amend it for the separate allocation, too.

(In reply to comment #1)
> > use nm_rtnl_link_parse_info_data() to get their data.
> 
> Is this going to be addressed in NMPlatform?

That is basically what I'm asking right now; how do we want to handle device-specific data?

Having NMPlatformLink "subclasses" might make more sense than my current system of having per-device-type "properties" structs. Maybe we could even use a single generic method in this case:

  gboolean nm_platform_link_get_info (int ifindex, NMPlatformLink *info, gsize size);

and you'd do, eg:

  NMPlatformLinkVxlan vxlan;

  if (nm_platform_link_get_info (ifindex, &vxlan, sizeof (vxlan)))
  ...

and NMPlatform would validate that @size was correct for the type of @ifindex.

> NMFakePlatformLink should now be ready to carry any arguments that
> don't require separate allocation. I'm going to amend it for the
> separate allocation, too.

For the moment, I've managed to avoid needing allocated arguments. (eg, the macvlan mode is passed back as a const string.)

(In reply to comment #2)
> (In reply to comment #1)
> > > use nm_rtnl_link_parse_info_data() to get their data.
> > 
> > Is this going to be addressed in NMPlatform?
> 
> That is basically what I'm asking right now; how do we want to handle
> device-specific data?

The whole point of NMPlatform is to separate NM core and the kernel. Whenever you contact the kernel directly, you are breaking the 'make check' testing infrastructure.

> Having NMPlatformLink "subclasses" might make more sense than my current system
> of having per-device-type "properties" structs. Maybe we could even use a
> single generic method in this case:

I think NMPlatformLink is a perfect fit for the common attributes. Per-device-type works IMO better than subclassing. Whether it's specific as in vlan_get_info() or generic as in link_get_info() is another topic.

It's not set in stone but I'm trying to keep the interface as simple as possible, just in case we ever need to maintain the code for multiple platforms.

>   gboolean nm_platform_link_get_info (int ifindex, NMPlatformLink *info, gsize
> size);
> 
> and you'd do, eg:
> 
>   NMPlatformLinkVxlanto avoid needing allocated arguments. (eg, the
> macvlan mode is passed back as a const string.)
 vxlan;
> 
>   if (nm_platform_link_get_info (ifindex, &vxlan, sizeof (vxlan)))
>   ...
> 
> and NMPlatform would validate that @size was correct for the type of @ifindex.

I discussed stuff like that with dcbw and he preferred more explicit API. That's why there are for example separate functions for ip4/ip6 addresses and ip4/ip6 routes, even though it would be perfectly possible to use a gpointer and address family as arguments.

My suggestion is to take the easy route now and do any big restructuring and consolidation later if we then think that it would be good. The easy route is IMO to have one or more get/set functions per device type in the form of nm_platform_<devtype>_get/set(). For vlan, I'm going to use nm_platform_vlan_get_info (ifindex, &parent, &vlanid).

Let me know if you have any objections.

> > NMFakePlatformLink should now be ready to carry any arguments that
> > don't require separate allocation. I'm going to amend it for the
> > separate allocation, too.
> 
> For the moment, I've managed to avoid needing allocated arguments. (eg, the
> macvlan mode is passed back as a const string.)

Nice. Then the existing code should work for you.

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > > use nm_rtnl_link_parse_info_data() to get their data.
> > > 
> > > Is this going to be addressed in NMPlatform?
> > 
> > That is basically what I'm asking right now; how do we want to handle
> > device-specific data?
> 
> The whole point of NMPlatform is to separate NM core and the kernel. Whenever
> you contact the kernel directly, you are breaking the 'make check' testing
> infrastructure.

Oh, I misunderstood your question, because what you were actually asking didn't make sense. It's *already* handled via NMPlatform; nm_rtnl_link_parse_info_data() is internal to nm-linux-platform.c.

> The easy route is
> IMO to have one or more get/set functions per device type in the form of
> nm_platform_<devtype>_get/set(). For vlan, I'm going to use
> nm_platform_vlan_get_info (ifindex, &parent, &vlanid).

That works for most device types, but vxlan has 15 properties, so you really want some sort of struct. Having 15 different getters is possible, I guess, but it seems ridiculously inefficient, given that we have to fetch the object from the kernel and re-parse it every time...

(Though maybe we should make rtnl_link store the unparsed IFLA_INFO_DATA for types it doesn't have explicit support for, so we only have to re-parse that...)

(In reply to comment #4)
> Oh, I misunderstood your question, because what you were actually asking didn't
> make sense. It's *already* handled via NMPlatform;
> nm_rtnl_link_parse_info_data() is internal to nm-linux-platform.c.

Didn't realize that, sorry.

> > The easy route is
> > IMO to have one or more get/set functions per device type in the form of
> > nm_platform_<devtype>_get/set(). For vlan, I'm going to use
> > nm_platform_vlan_get_info (ifindex, &parent, &vlanid).
> 
> That works for most device types, but vxlan has 15 properties, so you really
> want some sort of struct.

Sure. A struct would be convenient for that one.

> Having 15 different getters is possible, I guess, but
> it seems ridiculously inefficient, given that we have to fetch the object from
> the kernel and re-parse it every time...

If you're concerned about the caching, I'd say do it simple now, optimize later.

> (Though maybe we should make rtnl_link store the unparsed IFLA_INFO_DATA for
> types it doesn't have explicit support for, so we only have to re-parse
> that...)

It would be nice if one could add a pointer and a free function to any nl_object in the cache. That way you could store just anything with the cached objects including stuff like carrier detect and vlan capabilities and whatever else is typically not stored in rtnl_link/addr/route.

Maybe I'll find time to contribute this to libnl. Even a simple callback with user data could be used (in conjunction with a hash table based on memory address) if it's not already there. An alternative would be to replace all calls to nl_cache_add/remove with NMLinuxPlatform methods that would handle the hashtable(s) with additional information. But that's not as elegant as a libnl way would be (while falling back to uncached retrieval).

(In reply to comment #1)
> The else fall through looks confusing, I would slightly prefer to handle that
> code path explicitly.

OK, the issue was that I didn't want to just return "generic" there, because we might add additional tests at some point in the future, so I wanted to fall through to the place where those tests would be.

But I rewrote it by moving the fallback case out of the switch() statement. So now instead of "/* else fallthough */", it's "else break".

this is now basically done, except that I still have to figure out what some of the properties actually *do*, so I can document them.

Basically all of the new types can be created with "ip link add", although you need iproute from rawhide for a few of them.

> platform,devices: add support for tun and tap devices

"The tunnel's "TUN_MULTI_QUEUE" flag; true if FIXME." <----

In link_changed(), shouldn't nm_platform_tun_get_properties() be writing the properties to 'props' rather than 'priv->props'?  Seems like we have a used-but-not-initialized issue here when the subsequent comparisons try to use 'props' but it's never been set to anything.

Any particular reason the owner/group values are 64-bit?  Is that because the kernel values are 64-bit?  Or is this simply to allow -1 for "unknown" and still be able to represent G_MAXUINT32?  Also, the D-Bus API defines them as 'u' which is 32-bit unsigned (of course) and thus we can't represent -1 over dbus anyway.  Also, I'm not 100% sure the marshalling will correctly handle 'u'<->gint64 in dbus-glib or anywhere else...  Maybe we should simply go with uint32 here and use G_MAXUINT32 as "unknown" owner/group?

> platform, devices: add support for vxlan devices

A bunch of FIXMEs in the D-Bus API docs.

Group and Local are defined by NMPlatform as in_addr_t; if they actually are IP addresses, I think we should expose the properties via D-Bus as strings, not IP addresses, which is something I regret not doing in the settings.  If we do expose them as 'u', we need to ensure the D-Bus API docs mention that the value is in network byte order.  Also these would of course be IPv4 specific, how about IPv6?  I've just seen various vxlan IPv6 patches float by on netdev, so it's coming Real Soon Now.

Random question; are we using "Link" for the 'parent' device these days, or is that just a C&P error?  Is it really a parent?  If so, should we use "Parent" instead of "Link"?  Or is there some reason to go with Link for all these virtual device types?

> platform, devices: add support for gre and gretap devices

Bunch of FIXMEs in the D-Bus API docs.

Looks like input/output "flags" here are a single value, while for eg TUN/TAP they get split out?  Any particular reason for that?  Kernel flags here appear to be defined as Big Endian too (see include/uapi/linux/if_tunnel.h) which is somewhat odd; we need to ensure that the API docs mention this, or we convert it to machine-endian, or just break out the flags.  Not sure which we want to do.  Nobody said the kernel API was well thought out either.

Also, I wonder if we should bother exposing PMTU discovery specifically for GRE, since it's a global sysctl and also a socket option?

Same thing here for local/remote: if they are IPv4 addresses, then we should either represent the address as a string, or if we do represent by a uint32, we should make sure the docs indicate that it's network byte order.

(In reply to comment #8)
> In link_changed(), shouldn't nm_platform_tun_get_properties() be writing the
> properties to 'props' rather than 'priv->props'?

Oops. Yes.

> Any particular reason the owner/group values are 64-bit?  Is that because the
> kernel values are 64-bit?  Or is this simply to allow -1 for "unknown" and
> still be able to represent G_MAXUINT32?

The latter.

> Also, the D-Bus API defines them as
> 'u' which is 32-bit unsigned

Oops, leftover from an earlier version. It should have been 'x' (int64) now.


(In reply to comment #9)
> Group and Local are defined by NMPlatform as in_addr_t; if they actually
> are IP addresses, I think we should expose the properties via D-Bus as
> strings, not IP addresses, which is something I regret not doing in the
> settings.

OK

> Also these would of course be IPv4 specific, how
> about IPv6?  I've just seen various vxlan IPv6 patches float by on netdev, so
> it's coming Real Soon Now.

Yeah, I saw some of that... maybe I should remove vxlan from the patchset for now, and wait for it to stablize a bit more.


(In reply to comment #10)
> Random question; are we using "Link" for the 'parent' device these days, or is
> that just a C&P error?  Is it really a parent?  If so, should we use "Parent"
> instead of "Link"?  Or is there some reason to go with Link for all these
> virtual device types?

I went with "link" because that's what the kernel calls it, but I guess it calls it that for vlan too, so yeah, "parent" is probably better.


(In reply to comment #11)
> Looks like input/output "flags" here are a single value, while for eg TUN/TAP
> they get split out?  Any particular reason for that?

GObject property-writing fatigue :-}

> Also, I wonder if we should bother exposing PMTU discovery specifically for
> GRE, since it's a global sysctl and also a socket option?

The netlink option gets/sets something specifically in the GRE data structures, so it might be something separate from whatever you're thinking of? I dunno; as per the FIXMEs, I haven't actually figured out what a lot of these things do.

pushed a bunch of fixes. still need to document GRE.

(In reply to comment #12)
> > Looks like input/output "flags" here are a single value, while for eg TUN/TAP
> > they get split out?  Any particular reason for that?
> 
> GObject property-writing fatigue :-}

would it make sense maybe to have a GHashTable-valued "options" / "properties" / "?" property on some of these devices, like with bond/bridge settings, rather than exposing every single flag individually? (And we could do the same for NMDeviceBond and NMDeviceBridge...)

(In reply to comment #13)
> pushed a bunch of fixes. still need to document GRE.
> 
> (In reply to comment #12)
> > > Looks like input/output "flags" here are a single value, while for eg TUN/TAP
> > > they get split out?  Any particular reason for that?
> > 
> > GObject property-writing fatigue :-}
> 
> would it make sense maybe to have a GHashTable-valued "options" / "properties"
> / "?" property on some of these devices, like with bond/bridge settings, rather
> than exposing every single flag individually? (And we could do the same for
> NMDeviceBond and NMDeviceBridge...)

I tried to move in the direction of making them explicit properties becuase then we can use the various GObject enumeration, validation, etc stuff on them, and also we get to document them.  HOwever, that's a lot of code.  At the moment, Bridge uses explicit properties, while Bond uses an 'options' hash.  I was thinking of moving Bond over to properties.

In the end, I don't really know what's best.  It's easier to add new properties using an options hash, but then it's harder for clients like nmcli or the control center to get the property type (string, uint, bool, etc) and to read the valid value ranges.  We also have to ahve functions like nm_device_bond_get_valid_options() or whatever.

On the other hand, the kernel people keep adding options too.

(In reply to comment #11)
> Looks like input/output "flags" here are a single value, while for eg TUN/TAP
> they get split out?  Any particular reason for that?  Kernel flags here appear
> to be defined as Big Endian too (see include/uapi/linux/if_tunnel.h) which is
> somewhat odd; we need to ensure that the API docs mention this, or we convert
> it to machine-endian, or just break out the flags.  Not sure which we want to
> do.  Nobody said the kernel API was well thought out either.

OK, so it turns out that even though it was laziness that made me not split them out before, this may have been the right decision; I felt that I had to split out the TUN/TAP flags, because they are specific to the Linux driver implementation; another OS that supported TUN/TAP devices might have different flags, or different values for the same flags.

But the GRE flags fields actually use the values directly from the GRE protocol (which is why they're in network byte order). So any OS that supported GRE would have the same set of flags. So in this case, I think it might be reasonable to leave them squished together. ?

(pushed with slightly improved docs and a bugfix noticed in passing. If you're OK with keeping the GRE flags squished then I think this is all ready to go.)

The only comment I have left is that, although the GRE flags are 16-bit, netlink defines them as 32-bit, so should we just do 'u' for the in/out flags intead of 'q' and keep it at 32 bit?

Other than that, looks good.

(In reply to comment #16)
> The only comment I have left is that, although the GRE flags are 16-bit,
> netlink defines them as 32-bit

nope:

	[IFLA_GRE_IFLAGS]	= { .type = NLA_U16 },
	[IFLA_GRE_OFLAGS]	= { .type = NLA_U16 },

(IKEY and OKEY are 32-bit. You might have misread that.)

> Other than that, looks good.

pushed