Maybe it's best to pick one software device-type and implement it though...


which includes adding types like NMSettingTYPE and NMDeviceTYPE. I would not reuse NMDeviceGeneric to treat device types that we can handle (even with possibly limited support for a start).

Branch bg/create-sw-devices-bgo749369 implements device creation for
GRE tunnels. If the approach is reasonable I'll extend it to other
device types.

 # nmcli c add type tunnel con-name Tunnel1 ifname gre1 remote 2.2.2.2 mode gre -- ipv4.method static ipv4.addresses 172.25.1.1/16
 # nmcli d
 DEVICE      TYPE      STATE        CONNECTION
 gre1        gre       connected    Tunnel1
 # ip a
 51: gre1@NONE: <POINTOPOINT,NOARP,UP,LOWER_UP> mtu 1476 qdisc noqueue state UNKNOWN group default
     link/gre 0.0.0.0 peer 2.2.2.2
     inet 172.25.1.1/16 brd 172.25.255.255 scope global gre1
        valid_lft forever preferred_lft forever
     inet6 fe80::5efe:0:0/64 scope link
        valid_lft forever preferred_lft forever

(In reply to Beniamino Galvani from comment #2)
> Branch bg/create-sw-devices-bgo749369 implements device creation for
> GRE tunnels. If the approach is reasonable I'll extend it to other
> device types.

I don't see anything obviously wrong. Will check more :)

Let's first finalize GRE implementation and merge it, before you implement more (not saying, that we should take too long with reviewing this one).



Will NMSettingTunnel be suitable to hold all future options that a "tunnel" might have? E.g if we have a GRE-specific (additional) option, will that fit well into NMSettingTunnel? Or would it be better to have a NMSettingGre?
Unfortunately, it's quite cumbersome to introduce a new NMSetting* type...

(In reply to Thomas Haller from comment #3)
> Let's first finalize GRE implementation and merge it, before you implement
> more (not saying, that we should take too long with reviewing this one).

Fine by me.

> Will NMSettingTunnel be suitable to hold all future options that a "tunnel"
> might have? E.g if we have a GRE-specific (additional) option, will that fit
> well into NMSettingTunnel? Or would it be better to have a NMSettingGre?
> Unfortunately, it's quite cumbersome to introduce a new NMSetting* type...

My idea was to use NMSettingTunnel for all devices supported by "ip tunnel":

       ip tunnel { add | change | del | show | prl } [ NAME ]
               [ mode MODE ] [ remote ADDR ] [ local ADDR ]
               [ [i|o]seq ] [ [i|o]key KEY ] [ [i|o]csum ] ]
               [ encaplimit ELIM ] [ ttl TTL ]
               [ tos TOS ] [ flowlabel FLOWLABEL ]
               [ prl-default ADDR ] [ prl-nodefault ADDR ] [ prl-delete ADDR ]
               [ [no]pmtudisc ] [ dev PHYS_DEV ]

       MODE :=  { ipip | gre | sit | isatap | ip6ip6 | ipip6 | ip6gre | any }

Most options are common to all device types but there are some which
are specific for GRE (key,csum,seq) or IPv6 tunnels (encaplim,
flowlabel). And also, remote and local properties support only IPv4 or
IPv6 addresses depending on the tunnel type.

IMO all these options can be added without problems to NMSettingTunnel
and we can use validate() to check if the options are consistent with
the chosen mode.

Regarding other device types, probably we'll need a NMSettingTunTap
for tun and tap devices, but what about vxlan/macvtap/macvlan/veth?
Should we create a new setting for each of them?

(In reply to Beniamino Galvani from comment #4)
> (In reply to Thomas Haller from comment #3)
> 
> > Will NMSettingTunnel be suitable to hold all future options that a "tunnel"
> > might have? E.g if we have a GRE-specific (additional) option, will that fit
> > well into NMSettingTunnel? Or would it be better to have a NMSettingGre?
> > Unfortunately, it's quite cumbersome to introduce a new NMSetting* type...
> 
> My idea was to use NMSettingTunnel for all devices supported by "ip tunnel":
> 
>        ip tunnel { add | change | del | show | prl } [ NAME ]
>                [ mode MODE ] [ remote ADDR ] [ local ADDR ]
>                [ [i|o]seq ] [ [i|o]key KEY ] [ [i|o]csum ] ]
>                [ encaplimit ELIM ] [ ttl TTL ]
>                [ tos TOS ] [ flowlabel FLOWLABEL ]
>                [ prl-default ADDR ] [ prl-nodefault ADDR ] [ prl-delete ADDR
> ]
>                [ [no]pmtudisc ] [ dev PHYS_DEV ]
> 
>        MODE :=  { ipip | gre | sit | isatap | ip6ip6 | ipip6 | ip6gre | any }
> 
> Most options are common to all device types but there are some which
> are specific for GRE (key,csum,seq) or IPv6 tunnels (encaplim,
> flowlabel). And also, remote and local properties support only IPv4 or
> IPv6 addresses depending on the tunnel type.
> 
> IMO all these options can be added without problems to NMSettingTunnel
> and we can use validate() to check if the options are consistent with
> the chosen mode.

sounds good to me!

What about NMDeviceGre? If they are similar enough, should we not also combine them all as a NMDeviceTunnel?

Or maybe have classes NMDevice <= NMDeviceTunnel <= NMDevice(Tunnel)Gre ?

Maybe not... what do you think?


> Regarding other device types, probably we'll need a NMSettingTunTap
> for tun and tap devices, but what about vxlan/macvtap/macvlan/veth?
> Should we create a new setting for each of them?

tentative yes

(In reply to Thomas Haller from comment #5)
> What about NMDeviceGre? If they are similar enough, should we not also
> combine them all as a NMDeviceTunnel?

Since most of the tunnel devices will have the same fields, this looks
reasonable.

> Or maybe have classes NMDevice <= NMDeviceTunnel <= NMDevice(Tunnel)Gre ?
> 
> Maybe not... what do you think?

I would prefer not having to create a class for each one of the tunnel
types if possible, and having a single NMDeviceTunnel with a 'type'
property.

> > Regarding other device types, probably we'll need a NMSettingTunTap
> > for tun and tap devices, but what about vxlan/macvtap/macvlan/veth?
> > Should we create a new setting for each of them?
> 
> tentative yes

Branch bg/create-sw-devices-bgo749369-tun supports the creation of TUN/TAP
interfaces which is a bit simpler to start with than tunnels, please review.

(In reply to Beniamino Galvani from comment #6)
> (In reply to Thomas Haller from comment #5)

> Branch bg/create-sw-devices-bgo749369-tun supports the creation of TUN/TAP
> interfaces which is a bit simpler to start with than tunnels, please review.



I know, currently related functions like vlan_add() are like you implemented tun_add()... but I consider that wrong, could we instead:

  - have as output argument a type "const NMPlatformLink **out_link"
  - name it "link_tun_add()" and nm_platform_link_tun_add() (or nm_p_link_add_tun())?




and shouldn't it return !!obj like the other functions?

»···if (out_link && obj)
»···»···*out_link = obj->link;

»···return !!obj;



-typedef NMDeviceGeneric NMDeviceTun;
-typedef NMDeviceGenericClass NMDeviceTunClass;
+typedef NMDevice NMDeviceTun;
+typedef NMDeviceClass NMDeviceTunClass;

We don't usually have this kind of typedef... maybe not?




the change to nm_device_get_priority() seems to belong to the previous commit.


Maybe we should name the stuff NMSettingTunTap/NMDeviceTunTap. Not sure about that. We have NMDeviceTun as internal API. But that alone isn't a strong reason.

(In reply to Thomas Haller from comment #7)
>
> I know, currently related functions like vlan_add() are like you implemented
> tun_add()... but I consider that wrong, could we instead:
>
>   - have as output argument a type "const NMPlatformLink **out_link"
>   - name it "link_tun_add()" and nm_platform_link_tun_add() (or
> nm_p_link_add_tun())?

Yes, probably it's a better interface, but I don't like much the idea
of introducing a function different from existing ones. Where the
function is used we already have a plink* supplied by caller (for
instance nm_device_create_and_realize()) and so changing interface
would imply an additional allocation/copy. And also,
_link_add_check_existing() could not be used inside tun_add() because
it requires a preallocated buffer, we would need to add a new similar
helper.

I'm for leaving it as is and, if we decide to change the internal API
of link creation functions I think we should change all of them at the
same time along with helper functions and callers.

> and shouldn't it return !!obj like the other functions?

Fixed.

> -typedef NMDeviceGeneric NMDeviceTun;
> -typedef NMDeviceGenericClass NMDeviceTunClass;
> +typedef NMDevice NMDeviceTun;
> +typedef NMDeviceClass NMDeviceTunClass;
>
> We don't usually have this kind of typedef... maybe not?

All other devices use the same typedef, I don't think this is wrong.

> the change to nm_device_get_priority() seems to belong to the previous
> commit.

Right.

> Maybe we should name the stuff NMSettingTunTap/NMDeviceTunTap. Not sure
> about that. We have NMDeviceTun as internal API. But that alone isn't a
> strong reason.

The TunTap suffix sounds better to me too, but since NMDeviceTun
properties were already exported over D-Bus in the
org.fd.NM.Device.Tun interface, I think it would be better not to
change the interface name and thus also the name of the device object.

Repushed branch bg/create-sw-devices-bgo749369-tun

Since this isn't a completely new device type, as a tun device used to be found with libnm-glib but no longer will be (since it can't handle the new type), it'll technically break existing behavior.  Do we care?  I can see arguments either way.  I guess we can always add support for it to libnm-glib later too.

Otherwise LGTM.

Seems like the device doesn't appear & connection is not brought up after the connection with autoconnect on is added:

# nmcli c add type tun ifname lala mode tun

This is different from other software devices (teams, bridges, ...).

(In reply to Lubomir Rintel from comment #10)
> Seems like the device doesn't appear & connection is not brought up after
> the connection with autoconnect on is added:
>
> # nmcli c add type tun ifname lala mode tun
>
> This is different from other software devices (teams, bridges, ...).

The reason is that connection gets created with ipv4.method=auto, it
is autoactivated, but DHCP fails immediately because the device is a
tunnel. This works instead:

# nmcli c add type tun ifname lala mode tun ip4 10.0.0.1/8

I wonder if we should forbid ipv4.method=auto for connections of type
tun, mode tun to avoid situations like this. Or maybe set the default
ipv4.method to disable and ipv6.method to ignore?

(In reply to Dan Williams from comment #9)
> Since this isn't a completely new device type, as a tun device used to be
> found with libnm-glib but no longer will be (since it can't handle the new
> type), it'll technically break existing behavior.  Do we care?  I can see
> arguments either way.  I guess we can always add support for it to
> libnm-glib later too.

I pushed a commit which adds a NMDeviceTun in libnm-glib; should also
NMSettingTun be added to libnm-util? NMDeviceTun has a
get_setting_type() method and right now I don't know what to return
(because libnm-util doesn't have NMSettingTun).

Is that method supposed to return something meaningful for new device
types in libnm-glib?

(In reply to Beniamino Galvani from comment #11)
> (In reply to Lubomir Rintel from comment #10)
> > Seems like the device doesn't appear & connection is not brought up after
> > the connection with autoconnect on is added:
> >
> > # nmcli c add type tun ifname lala mode tun
> >
> > This is different from other software devices (teams, bridges, ...).
> 
> The reason is that connection gets created with ipv4.method=auto, it
> is autoactivated, but DHCP fails immediately because the device is a
> tunnel. This works instead:
> 
> # nmcli c add type tun ifname lala mode tun ip4 10.0.0.1/8
> 
> I wonder if we should forbid ipv4.method=auto for connections of type
> tun, mode tun to avoid situations like this. Or maybe set the default
> ipv4.method to disable and ipv6.method to ignore?

I'm not sure. Note that the method is "auto" not "dhcp" -- maybe we should just succeed the activation for the devices where DHCP can't work?

Anyway; I though the autoactivation is not even attempted not realizing that an attempt to do DHCP was made instead. I'm fine with just leaving it the way it is too.

(In reply to Lubomir Rintel from comment #13)
> Anyway; I though the autoactivation is not even attempted not realizing that
> an attempt to do DHCP was made instead. I'm fine with just leaving it the
> way it is too.

Okay, let's keep it as it is now.

(In reply to Beniamino Galvani from comment #12)
> Is that method supposed to return something meaningful for new device
> types in libnm-glib?

libnm-glib-test still had a failed assertion as a 'tun' setting could not
be found. I've added an empty NMSettingTun to libnm-util to fix this.

Branch bg/create-sw-devices-bgo749369-tun rebased on master and repushed.

I didn't look deep into it, but what about ifcfg-rh support to store tun-files?

Do you only want to have keyfile plugin support the tun-settings? (fine with me). Did you test that ifcfg-rh does not try to handle such a setting?

+#ifdef IFF_MULTI_QUEUE
        if (multi_queue)
                ifr.ifr_flags |= IFF_MULTI_QUEUE;
+#endif

do older kernels have a problem if you set the flag when they don't understand it?

Otherwise, I think it's better to just:

#ifndef IFF_MULTI_QUEUE
#define IFF_MULTI_QUEUE 0x0100
#endif

at the beginning of nm-linux-platform.c, where all the other defines for old kernel headers are.

(In reply to Thomas Haller from comment #15)
> Do you only want to have keyfile plugin support the tun-settings? (fine with
> me). Did you test that ifcfg-rh does not try to handle such a setting?

Repushed branch with ifcfg-rh support.

(In reply to Thomas Haller from comment #16)
> do older kernels have a problem if you set the flag when they don't
> understand it?

It seems not. Fixed, thanks.

>> libnm-core: add NMSettingTun

the settings mode, owner, group are strings. Maybe the should be enum/integers.

for mode, we again have the same reasoning as for ipv6.addr-gen-mode. Well, I don't really mind to strongly the string, but I tend to prefer again int/enum.
We can discuss that and find a policy for new settings, ... if everybody gives their opinion?


  For APIs being used in constrained situations, enumerated values should be
  transmitted as unsigned integers. For APIs which need to be extended by third 
  parties or which are used in more loosely coupled systems, enumerated values
  should be strings in some defined format.

[ http://dbus.freedesktop.org/doc/dbus-api-design.html#type-system ]





what was the reason to have the owner-ui as string? Do you also want to support user-names? Do you imagine to later allow to extend the field to allow user-names like: "user:bengal"?





>> device/tun: support device creation


+    nm_utils_complete_generic (connection,
+                               NM_SETTING_TUN_SETTING_NAME,
+                               existing_connections,
+                               NULL,
+                               _("TUN connection"),
+                               NULL,
+                               TRUE);

it's strange that the core daemon translates a message. It most likely is anyway not started with the right local of the user. Does it even make sense to do this?




+    user = _nm_utils_ascii_str_to_int64 (nm_setting_tun_get_owner (s_tun), 10, 0, G_MAXINT32, -1);
+    group = _nm_utils_ascii_str_to_int64 (nm_setting_tun_get_group (s_tun), 10, 0, G_MAXINT32, -1);
+
+    if (user != priv->props.owner)
+         return FALSE;


I think an unset owner means "root".
Thus you need:

    tmp = nm_setting_tun_get_owner (s_tun);
    user = tmp
           ? _nm_utils_ascii_str_to_int64 (tmp, 10, 0, G_MAXINT32, -1)
           : 0;





ah, there is already... can you consolidate that?
#ifndef IFF_MULTI_QUEUE
»···»···»···const int IFF_MULTI_QUEUE = 0x0100;
#endif



>> libnm: add NMDeviceTun


+#include <nm-setting-connection.h>
+#include <nm-setting-tun.h>
+#include <nm-utils.h>

I don't think it actually matters, but IMO we we should include our own headers with "" (everywhere, except in public headers that we install).





>> libnm-util: add NMSettingTun
    
why do you need this? Can you not just avoid the assertion? :)
I think we don't want to backport all new setting-types, so libnm-glib anyway should handle unknown settings gracefully.


>> libnm-glib: add NMDeviceTun

I would not have brought this new feature to the legacy library. Was there a problem with leaving it out?

(In reply to Thomas Haller from comment #18)
> the settings mode, owner, group are strings. Maybe the should be
> enum/integers.
>
> for mode, we again have the same reasoning as for ipv6.addr-gen-mode. Well,
> I don't really mind to strongly the string, but I tend to prefer again
> int/enum.
> We can discuss that and find a policy for new settings, ... if everybody
> gives their opinion?

In general I prefer an int/enum too; I added this as string because
NMDeviceTun already had a "mode" string property. But probably that
shouldn't matter much. I'll convert it to a int.

> what was the reason to have the owner-ui as string? Do you also want to
> support user-names? Do you imagine to later allow to extend the field to
> allow user-names like: "user:bengal"?

Yes, this was the idea (iproute2 for example allows to specify user
and groups both in numeric and string form).

> +    nm_utils_complete_generic (connection,
> +                               NM_SETTING_TUN_SETTING_NAME,
> +                               existing_connections,
> +                               NULL,
> +                               _("TUN connection"),
> +                               NULL,
> +                               TRUE);
>
> it's strange that the core daemon translates a message. It most likely is
> anyway not started with the right local of the user. Does it even make sense
> to do this?

Well, according to po/POTFILES.in there are many others messages
translated in the core. With the code above the default connection
names will be translated to the system-configured language, I don't
think this is a problem; in general user visible messages,labels,etc. are
supposed to be translated, no?

> +    user = _nm_utils_ascii_str_to_int64 (nm_setting_tun_get_owner (s_tun),
> 10, 0, G_MAXINT32, -1);
> +    group = _nm_utils_ascii_str_to_int64 (nm_setting_tun_get_group (s_tun),
> 10, 0, G_MAXINT32, -1);
> +
> +    if (user != priv->props.owner)
> +         return FALSE;
>
>
> I think an unset owner means "root".

From drivers/net/tun.c:

  static inline bool tun_not_capable(struct tun_struct *tun)
  {
        const struct cred *cred = current_cred();
          struct net *net = dev_net(tun->dev);

          return ((uid_valid(tun->owner) && !uid_eq(cred->euid, tun->owner)) ||
                    (gid_valid(tun->group) && !in_egroup_p(tun->group))) &&
                !ns_capable(net->user_ns, CAP_NET_ADMIN);
  }

it seems that unset (-1) and 0 have different effects. Attaching to a
tun device with owner -1 is allowed for an unprivileged user having
permissions on /dev/net/tun (and my documentation of tun.owner and
tun.group properties is wrong, I'll update it).

> ah, there is already... can you consolidate that?
> #ifndef IFF_MULTI_QUEUE
> »···»···»···const int IFF_MULTI_QUEUE = 0x0100;
> #endif

I'll squash your fixup

> I don't think it actually matters, but IMO we we should include our own
> headers with "" (everywhere, except in public headers that we install).

Agree.

> >> libnm-util: add NMSettingTun
>
> why do you need this? Can you not just avoid the assertion? :)
> I think we don't want to backport all new setting-types, so libnm-glib
> anyway should handle unknown settings gracefully.

Yes, this makes sense.

> >> libnm-glib: add NMDeviceTun
>
> I would not have brought this new feature to the legacy library. Was there a
> problem with leaving it out?

See comment 9; before this patchset tun devices showed up as generic
devices in libnm-glib. Without this patch libnm-glib whould complain
with:

  "libnm-glib-WARNING **: Unknown device type 16"

(In reply to Beniamino Galvani from comment #19)
> (In reply to Thomas Haller from comment #18)

> > >> libnm-util: add NMSettingTun
> >
> > why do you need this? Can you not just avoid the assertion? :)
> > I think we don't want to backport all new setting-types, so libnm-glib
> > anyway should handle unknown settings gracefully.
> 
> Yes, this makes sense.
> 
> > >> libnm-glib: add NMDeviceTun
> >
> > I would not have brought this new feature to the legacy library. Was there a
> > problem with leaving it out?
> 
> See comment 9; before this patchset tun devices showed up as generic
> devices in libnm-glib. Without this patch libnm-glib whould complain
> with:
> 
>   "libnm-glib-WARNING **: Unknown device type 16"

now the patch is already complete it makes sense.

But in the future (eg. vxlan devices), couldn't we fallback to create a generic device for certain unknown device-types?

And maybe the warning is just overly alarming and should be silenced altogether.

Overall LGTM.

On the 'mode' discussion, I usually think enums are better (since they are easier/cheaper to validate and it's really hard to mis-spell a number).  But they come at the price of more code in the 'keyfile' and ifcfg-rh plugins, where we typically want this stuff to be friendly to the user (eg "tap" instead of "2").

In this case, there are only 2 values and there aren't likely to be any more values ever, so let's just do an int.  Easy to parse in the plugins.

---

For future libnm-glib, we should probably just create Generic devices, but we'd probably have to add some code to make the check_connection_compatible and other hooks work with generic devices.

(In reply to Thomas Haller from comment #18)

> +#include <nm-setting-connection.h>
> +#include <nm-setting-tun.h>
> +#include <nm-utils.h>
> 
> I don't think it actually matters, but IMO we we should include our own
> headers with "" (everywhere, except in public headers that we install).

Well, thinking about it, these are headers from a different library so maybe
angle brackets are appropriate here.

(In reply to Dan Williams from comment #21)

> In this case, there are only 2 values and there aren't likely to be any more
> values ever, so let's just do an int.  Easy to parse in the plugins.

Changed to a uint property.

> For future libnm-glib, we should probably just create Generic devices, but
> we'd probably have to add some code to make the check_connection_compatible
> and other hooks work with generic devices.

Ok, I kept the new files in libnm-glib and libnm-util, but for future devices
I'll try to reuse NMDeviceGeneric.

Also, I realized that initscripts already have a basic support for TUN/TAP
devices, but they have the big limitation that the mode is derived from the
interface name (tun* is a TUN device, everything else is a TAP). I don't think
we want this limitation and so my proposal would be to drop the ifcfg-rh
patch and only support the keyfile plugin.

bg/create-sw-devices-bgo749369-tun repushed.

Updated branch to consider TUN devices as generic devices in libnm-glib.

(In reply to Beniamino Galvani from comment #22)
> (In reply to Thomas Haller from comment #18)
> 
> > +#include <nm-setting-connection.h>
> > +#include <nm-setting-tun.h>
> > +#include <nm-utils.h>
> > 
> > I don't think it actually matters, but IMO we we should include our own
> > headers with "" (everywhere, except in public headers that we install).
>
> Well, thinking about it, these are headers from a different library so maybe
> angle brackets are appropriate here.

Whenever we include those libnm headers, we want to include our user-provided source-file from our source tree. Contrary to a system-provided header file, so IMO <> is more appropriate.

Well, we are not consistent about it anyway...

+typedef enum {
+    NM_SETTING_TUN_MODE_TUN = 0,
+    NM_SETTING_TUN_MODE_TAP = 1,
+} NMSettingTunMode;

Let's have 0 being reserved for "unknown", and shift the numerical values by one?




>> libnm-util: add NMSettingTun

I still don't understand the purpose of this. What assertion is that? Can that not be avoided in another way?


>> libnm-glib: add support for TUN devices
    
This looks like the right approach to me. But can you also somehow properly handle generic devices for entirely unknown types? In this commit you still teach NMDeviceGeneric how to handle tun-devices. Can you let NMDeviceGeneric handle truly unknown types (as well as possible).




Rest LGTM

(In reply to Thomas Haller from comment #25)
>
> Let's have 0 being reserved for "unknown", and shift the numerical values by
> one?

Okay, fixed.

> >> libnm-util: add NMSettingTun
>
> I still don't understand the purpose of this. What assertion is that? Can
> that not be avoided in another way?

When launching libnm-glib/libnm-glib-test, the TUN connection
doesn't verify because the connection type is 'tun' but there isn't
any setting with such name in the connection:

(process:9958): libnm-glib-WARNING **: replace_settings: error updating connection /org/freedesktop/NetworkManager/Settings/20 settings: (3) connection.type: requires presence of 'tun' setting in the connection

If the check gets removed obviously we'll consider valid some invalid
connections. It's not clear to me how to solve this in other ways...

> >> libnm-glib: add support for TUN devices
>
> This looks like the right approach to me. But can you also somehow properly
> handle generic devices for entirely unknown types? In this commit you still
> teach NMDeviceGeneric how to handle tun-devices. Can you let NMDeviceGeneric
> handle truly unknown types (as well as possible).

If we want the NMDeviceGeneric properties to be properly populated, we
have to initialize the proxy with the right D-Bus interface for the
device type. That's why I have added the _device_type_to_interface()
function which knows the supported device types (ATM only tun).

I guess it depends on what you mean by "as well as possible". If you
consider ok to have empty properties, then yes, we can support truly
unknown types. In that case we don't have to create the D-Bus proxy.

(In reply to Beniamino Galvani from comment #26)
> (In reply to Thomas Haller from comment #25)

> > >> libnm-util: add NMSettingTun
> >
> > I still don't understand the purpose of this. What assertion is that? Can
> > that not be avoided in another way?
> 
> When launching libnm-glib/libnm-glib-test, the TUN connection
> doesn't verify because the connection type is 'tun' but there isn't
> any setting with such name in the connection:
> 
> (process:9958): libnm-glib-WARNING **: replace_settings: error updating
> connection /org/freedesktop/NetworkManager/Settings/20 settings: (3)
> connection.type: requires presence of 'tun' setting in the connection
> 
> If the check gets removed obviously we'll consider valid some invalid
> connections. It's not clear to me how to solve this in other ways...

The connection doesn't verify (because of unknown "tun" type). Is that a problem?

We cannot expect that all future-added connections will verify according to an old library version. The library must handle such a situation gracefully.

Is a g_warning() in this case appropriate? Maybe it just is. Or maybe we should just silently such failures in replace_settings()?

We anyway want that old, non-updated libnm/libnm-glib versions work as well as possible... and if they do that, there is no reason for such a backport.





> > >> libnm-glib: add support for TUN devices
> >
> > This looks like the right approach to me. But can you also somehow properly
> > handle generic devices for entirely unknown types? In this commit you still
> > teach NMDeviceGeneric how to handle tun-devices. Can you let NMDeviceGeneric
> > handle truly unknown types (as well as possible).
> 
> If we want the NMDeviceGeneric properties to be properly populated, we
> have to initialize the proxy with the right D-Bus interface for the
> device type. That's why I have added the _device_type_to_interface()
> function which knows the supported device types (ATM only tun).
> 
> I guess it depends on what you mean by "as well as possible". If you
> consider ok to have empty properties, then yes, we can support truly
> unknown types. In that case we don't have to create the D-Bus proxy.

NMDeviceGeneric only has NM_DEVICE_GENERIC_HW_ADDRESS and NM_DEVICE_GENERIC_TYPE_DESCRIPTION. Seems fine with me if they are empty.

(In reply to Thomas Haller from comment #27)
> (In reply to Beniamino Galvani from comment #26)
> > (In reply to Thomas Haller from comment #25)
> 
> > > >> libnm-util: add NMSettingTun
> The connection doesn't verify (because of unknown "tun" type). Is that a
> problem?
> 
> We cannot expect that all future-added connections will verify according to
> an old library version. The library must handle such a situation gracefully.
> 
> Is a g_warning() in this case appropriate? Maybe it just is. Or maybe we
> should just silently such failures in replace_settings()?
> 
> We anyway want that old, non-updated libnm/libnm-glib versions work as well
> as possible... and if they do that, there is no reason for such a backport.

Yeah, in any case this is just a warning and we can't add support for new
setting types to old libnm-glibs. I dropped this commit.

> > > >> libnm-glib: add support for TUN devices
> > I guess it depends on what you mean by "as well as possible". If you
> > consider ok to have empty properties, then yes, we can support truly
> > unknown types. In that case we don't have to create the D-Bus proxy.
> 
> NMDeviceGeneric only has NM_DEVICE_GENERIC_HW_ADDRESS and
> NM_DEVICE_GENERIC_TYPE_DESCRIPTION. Seems fine with me if they are empty.

Added a fixup which creates generic devices with empty properties for unknown
(neither Generic nor Tun) devices.

After these changes the branch looks ready to be merged, any objections?

LGTM

> fixup! device/tun: support device creation

Why does this need to override setup() just to call the parent's setup?  That should be happening automatically with GObject since NMDevice sets setup().

Otherwise LGTM.

(In reply to Dan Williams from comment #30)
> Why does this need to override setup() just to call the parent's setup? 
> That should be happening automatically with GObject since NMDevice sets
> setup().
> 
> Otherwise LGTM.

Fixed and merged to master:

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

All dependent bugs/device-types are implemented.

Missing is for example veth type, but it's not yet clear how useful that is (because usually you'd use veths with namespaces and how can you express a namespace in a NetworkManager connection?).


Still close this bug as large parts of it are fixed. Let's open new ones for further device-types and for new requirements.