*** Bug 559474 has been marked as a duplicate of this bug. ***

*** Bug 561798 has been marked as a duplicate of this bug. ***

Yes, PPPoE should be more like a VPN connection and be layered on top the existing wired or wifi connection.  This should be possible in a future update of NM.

*** Bug 579432 has been marked as a duplicate of this bug. ***

*** Bug 592841 has been marked as a duplicate of this bug. ***

@Dan Williams:
How much work is needed to fix this? I currently use pppoeconf, pon, poff & plog when in Ubuntu for my internet. Would it be possible to just enable PPPoE for Wireless as a first step? How would one go about doing this?

I would be interested in trying to help resolve this, 
please can you provide a brief outline of the work needed to resolve this
issue?
thanks,
mike

See https://bugzilla.redhat.com/show_bug.cgi?id=446338 for another version of this bug, and @Dan Willian's March 2010 comment there that "It's not a small change actually, but it'll happen."

*** Bug 630332 has been marked as a duplicate of this bug. ***

>*** Bug 630332 has been marked as a duplicate of this bug. ***
Sorry for this another dup :)

If I can help somehow (testing, collecting debugging output) please let me know.

I need this feature too. This is quite annoying to switch between LAN and PPPoE connections each time i need to take smth from my LAN.

*** Bug 639578 has been marked as a duplicate of this bug. ***

*** Bug 651410 has been marked as a duplicate of this bug. ***

NM bugzilla reorganization... sorry for the bug spam.

Suffering from this bug, too. Looks like after almost 6 years and several releases of NM, nothing has changed. Really sad.

(In reply to comment #15)
> Suffering from this bug, too. Looks like after almost 6 years and several
> releases of NM, nothing has changed. Really sad.

Patches welcome.

(In reply to comment #16)
> (In reply to comment #15)
> > Suffering from this bug, too. Looks like after almost 6 years and several
> > releases of NM, nothing has changed. Really sad.
> 
> Patches welcome.

Funny to see such response from someone with email @redhat.com.

3 more years and still no progress? PPPoE shouldn't use the ethernet device exclusively.

Pushed branch bg/ppp-bgo559134, please review.

The branch introduces a new 'parent' property in the pppoe
setting. When this property is set, the PPPoE connection is activated
on that interface and 'connection.interface-name' is used as the PPP
interface name:

 $ nmcli connection add type pppoe con-name pppoe-wifi \
                        ifname ppp-wifi parent wlp4s0 \
                        username user password password

When the 'parent' property is empty, the old behavior is maintained
and the PPPoE connection is activated on 'connection.interface-name',
using a unpredictable PPP interface name.

> libnm,clients: add 'parent' property to PPPoE setting

I think NMPppoeSetting:verify should check the new "parent" property. At least, that it's a valid interface name (IFNAMSIZ length).

> device: add NMDevicePPP
    
+/**
+ * NMDevicePpp:
+ */
+struct _NMDevicePpp {
+    NMDevice parent;
+};
+
+typedef struct {
+    NMDeviceClass parent;
+
+    /*< private >*/
+    gpointer padding[4];
+} NMDevicePppClass;
+

Could we not expose the type structures as public API? It makes no sense that a user creates such types, they are created by NMClient. It makes even less sense, that a user derives from these types.

It's a bug that we have any of these structures public, and if it wouldn't be so much work, I would even remove them from public API. Nobody is using them, and if they do: they shouldn't.

Even for NMSetting, a user cannot derive from it NMSettingMy, because _nm_register_setting() is not public API.


and then, we should implement private data, very much like we do in src ( https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=4d37f7a1e94f469fb1e3eacde4d2424ebf6ccf0b ). I only didn't do it for libnm/libnm-core, because it was too much work. For new code, we should do it (in my opinion).



same in src/devices/nm-device-ppp.h. See https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=4d37f7a1e94f469fb1e3eacde4d2424ebf6ccf0b
Also:
+    struct _NMDevicePppPrivate *_priv;
                                ^^ no pointer.

(we can of course discuss whether what I consider best practice is indeed best, and why. But at least, your patch does it differently from what I currently think is best.)




+    NM_DEVICE_FACTORY_DECLARE_LINK_TYPES    (NM_LINK_TYPE_PPP),
                                         ^^^^ why the spaces? Don't align :)




+static NMDeviceCapabilities
+get_generic_capabilities (NMDevice *device)
+{
+    return NM_DEVICE_CAP_IS_SOFTWARE;
+}
+
+static void
+dispose (GObject *object)


can we order code for GObject implementations, so that they end with

  everything-else
  /****
  get_property()
  set_property()
  /****
  nm_type_init()
  constructor()
  constructed()
  dispose()
  finalize()
  nm_type_class_init()
  /**/
  NMDeviceFactory stuff.


get_generic_capabilities() should be earlier.


+ g_clear_pointer (&priv->pending_ip4_config, g_object_unref);
g_clear_object()?
+ g_clear_pointer (&priv->pending_ifname, g_free);
nm_clear_g_free()?




nm_device_take_over_link() must not access @plink after invoking any platform operation. The pointer could be invalidated. With current master, you may safely do:

  nm_auto_nmpobj NMPObject *plink_keep_alive = NULL;
  ...
  plink_keep_alive = nmp_object_ref (NMP_OBJECT_UP_CAST (plink));

or you just cache plink->ifindex.



-   NM_DEVICE_DEVICE_TYPE, NM_DEVICE_TYPE_PPP,
+   NM_DEVICE_DEVICE_TYPE, NM_DEVICE_TYPE_ETHERNET,

why this? Can you add a comment?




+create_and_realize()
+
+    /* The interface is created later */
+
+    return TRUE;

doesn't this kinda defeat the point of what create_and_realize() means? Maybe we should add another stage, to allow for create_and_realize() to work asynchronously? But probably not...





> core: ppp: use a different unit for each activation

doesn't this still race with other ppp instances? What happens if such a number exists? Maybe have a retry loop and skip over interfaces that exist?

(In reply to Thomas Haller from comment #21)
> > libnm,clients: add 'parent' property to PPPoE setting
> > ...
> > why this? Can you add a comment?

Fixed all the points above and repushed.


> +create_and_realize()
> +
> +    /* The interface is created later */
> +
> +    return TRUE;
> 
> doesn't this kinda defeat the point of what create_and_realize() means?
> Maybe we should add another stage, to allow for create_and_realize() to work
> asynchronously? But probably not...

The point is that NM knows the ifindex (and thus can actually realize
the device) only after pppd connects, i.e. after the connection is
almost completed; so I don't know how a new stage would help.


> > core: ppp: use a different unit for each activation
> 
> doesn't this still race with other ppp instances? What happens if such a
> number exists? Maybe have a retry loop and skip over interfaces that exist?

We can't tell pppd to create an interface with a given name, so we use
the name generated by kernel and rename the interface afterwards. The
race condition can happen during the rename: NM receives the interface
name from pppd, but in the meantime the interface could be deleted and
another one with that name could appear. In this case we would rename
the wrong interface.

Using a changing unit index, we ensure that interfaces created by NM
don't race with each others. There is still the chance to race with
externally-created interfaces, but I guess this is not easily solvable
since the pppd plugin does not expose the ifindex.

When the specified unit is already in use, the kernel selects another
one, so a loop is not necessary.

>> core: device-factory: implement match_connection()
    
what prevents the bluetooth factory to claim the bluetooth NAP connection? Probably only the order in which the factories are registered.
If you register more then one factories, all of them must implement match_connection(), otherwise it depends on the order -- which is hard to understand.

the handling of priv->ip_iface in NMPppManager seems wrong.

impl_ppp_manager_set_ip4_config() first uses it:

»···config = nm_ip4_config_new (nm_platform_get_multi_idx (NM_PLATFORM_GET),
»···                            nm_platform_link_get_ifindex (NM_PLATFORM_GET, priv->ip_iface));

and only later initializes it:

»···if (!set_ip_config_common (manager, config_dict, NM_PPP_IP4_CONFIG_INTERFACE, &u32))
»···»···goto out;



I think set_ip_config_common() should be done first.




> core: ppp: use a different unit for each activation

maybe src/ppp/nm-pppd-plugin.c should resolve the name right away (if_nametoindex) and send it to NM too. Instead of sending the name, and having NM later look it up.

(In reply to Thomas Haller from comment #23)
> >> core: device-factory: implement match_connection()
>     
> what prevents the bluetooth factory to claim the bluetooth NAP connection?
> Probably only the order in which the factories are registered.
> If you register more then one factories, all of them must implement
> match_connection(), otherwise it depends on the order -- which is hard to
> understand.

Yeah, it depends on the order - internal factories as bridge are processed before external ones as bluetooth. For clarity, I added a check in the bluetooth factory as well.


(In reply to Thomas Haller from comment #24)
> the handling of priv->ip_iface in NMPppManager seems wrong.
> I think set_ip_config_common() should be done first.

This is already fixed on master with commit:

https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=565adb4df25639c75af858c26515c69e85348be3

> > core: ppp: use a different unit for each activation
> 
> maybe src/ppp/nm-pppd-plugin.c should resolve the name right away
> (if_nametoindex) and send it to NM too. Instead of sending the name, and
> having NM later look it up.

Maybe, but I don't see a clear advantage in doing so...

lgtm now

Merged:

https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=220a329feaa79d535da0abcc10f94ebbe7f7be78

nm-connection-editor part still to do...

Created attachment 357356 [details] [review]
[nm-c-e PATCH] editor: pppoe: add support for new-style pppoe connections

(In reply to Beniamino Galvani from comment #28)
> Created attachment 357356 [details] [review] [review]
> [nm-c-e PATCH] editor: pppoe: add support for new-style pppoe connections

nm_setting_pppoe_get_parent() is NM_AVAILABLE_IN_1_10, so you cannot use it on current master. You either 
 1) bump the required libnm version to 1.9,
 2) or you add some #if for conditional compliation,
 3) or you do some runtime dependent hack.

1) is not great, because it forces us to maintain a separate nma-1-8 branch. 2) is easy, but conditional compilation is not great.
3) should be easy enough. Use g_object_class_find_property() and g_object_get() at runtime, instead of using 1.10 ABI.

Created attachment 359362 [details] [review]
[nm-c-e PATCH v2] editor: pppoe: add support for new-style pppoe connections

How about v2?

(In reply to Beniamino Galvani from comment #30)
> Created attachment 359362 [details] [review] [review]
> [nm-c-e PATCH v2] editor: pppoe: add support for new-style pppoe connections
> 
> How about v2?

     <property name="can_focus">False</property>
+    <property name="tooltip_text" translatable="yes">Username used to authenticate with the PPPoE service.</property>
     <property name="hexpand">True</property>

this tooltip seems wrong.




I like the approach to detect libnm capabilities based on gobject metadata about classes. I think we will end up using this runtime detection more in the future, because I see value in not branching off stable branches.
But for that, I would suggest to use nm_g_object_class_find_property_from_gtype(), which I just added.



Otherwise, patch lgtm.

Created attachment 359386 [details] [review]
[nm-c-e PATCH v3 1/2] editor: pppoe: add support for new-style pppoe connections

Created attachment 359387 [details] [review]
[nm-c-e PATCH v3 2/2] editor: pppoe: add tooltips

(In reply to Thomas Haller from comment #31)
> (In reply to Beniamino Galvani from comment #30)
> > Created attachment 359362 [details] [review] [review] [review]
> > [nm-c-e PATCH v2] editor: pppoe: add support for new-style pppoe connections
> > 
> > How about v2?
> 
>      <property name="can_focus">False</property>
> +    <property name="tooltip_text" translatable="yes">Username used to
> authenticate with the PPPoE service.</property>
>      <property name="hexpand">True</property>
> 
> this tooltip seems wrong.

This is actually for the Username field. Now I've split tooltips addition to a different patch for clarity.

> I would suggest to use nm_g_object_class_find_property_from_gtype(),
> which I just added.

Changed.

(In reply to Beniamino Galvani from comment #34)
> (In reply to Thomas Haller from comment #31)

> This is actually for the Username field. Now I've split tooltips addition to
> a different patch for clarity.

it still seems to be at the wrong place. The entire DSL page gets the tooltip "Username used to authenticate with the PPPoE service.", not only the username entry (and it's label).

(In reply to Thomas Haller from comment #35)
> it still seems to be at the wrong place. The entire DSL page gets the
> tooltip "Username used to authenticate with the PPPoE service.", not only
> the username entry (and it's label).

Now fixed, for real :)

Applied to master:

https://git.gnome.org/browse/network-manager-applet/commit/?id=3c83f844ced4f620993054cddb19b22fe459fa0b

https://git.gnome.org/browse/network-manager-applet/commit/?id=956d8e0a1e9002069938bdc7a39857400992a458

*** Bug 724501 has been marked as a duplicate of this bug. ***