After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 559134 - ppp: better PPPoE support (local net access, wifi)
ppp: better PPPoE support (local net access, wifi)
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: PPP
0.7.x
Other All
: Normal enhancement
: ---
Assigned To: Dan Williams
NetworkManager maintainer(s)
: 559474 561798 579432 592841 630332 639578 651410 724501 (view as bug list)
Depends on: 773482
Blocks: 724501 nm-review nm-next
 
 
Reported: 2008-11-03 16:12 UTC by Klaus Doblmann
Modified: 2017-11-13 11:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[nm-c-e PATCH] editor: pppoe: add support for new-style pppoe connections (10.94 KB, patch)
2017-08-10 20:12 UTC, Beniamino Galvani
none Details | Review
[nm-c-e PATCH v2] editor: pppoe: add support for new-style pppoe connections (13.96 KB, patch)
2017-09-07 15:32 UTC, Beniamino Galvani
none Details | Review
[nm-c-e PATCH v3 1/2] editor: pppoe: add support for new-style pppoe connections (12.42 KB, patch)
2017-09-08 08:01 UTC, Beniamino Galvani
none Details | Review
[nm-c-e PATCH v3 2/2] editor: pppoe: add tooltips (2.45 KB, patch)
2017-09-08 08:01 UTC, Beniamino Galvani
none Details | Review

Description Klaus Doblmann 2008-11-03 16:12:58 UTC
(note: see https://bugs.launchpad.net/bugs/290639 for my original bug report and additional info)

In general, the title says it all:

I'm required to use LAN and internet at the same time and I have only one ethernet-port on my iMac.
I connect to the internet using a pppoe (ppp0) connection via my one and only ethernet-port (eth0). Previously (pre NM 0.7) I was able to get an IP on ppp0 and eth0 (manually assigned or via DHCP) at the same time by using the interfaces config-file.

Now with NM 0.7 ignoring that config-file, the following problem exists: When I select "ppp0" in the applet, I get connected to the internet but don't get an IP on the eth0 port. When I connect to eth0 I get an IP for the LAN at home but, obviously, internet doesn't work.

LOGS:

1) Here's the ifconfig-log when eth0 is enabled:
#ifconfig for eth0 below
ifconfig
eth0      Link encap:Ethernet  Hardware Adresse 00:22:41:37:af:61  
          inet Adresse:192.168.0.3  Bcast:192.168.0.255  Maske:255.255.255.0
          inet6-Adresse: fe80::222:41ff:fe37:af61/64 Gültigkeitsbereich:Verbindung
          UP BROADCAST RUNNING MULTICAST  MTU:1500  Metrik:1
          RX packets:3053 errors:0 dropped:0 overruns:0 frame:0
          TX packets:3037 errors:0 dropped:0 overruns:0 carrier:0
          Kollisionen:0 Sendewarteschlangenlänge:1000 
          RX bytes:2123258 (2.1 MB)  TX bytes:548526 (548.5 KB)
          Interrupt:17 

lo        Link encap:Lokale Schleife  
          inet Adresse:127.0.0.1  Maske:255.0.0.0
          inet6-Adresse: ::1/128 Gültigkeitsbereich:Maschine
          UP LOOPBACK RUNNING  MTU:16436  Metrik:1
          RX packets:247 errors:0 dropped:0 overruns:0 frame:0
          TX packets:247 errors:0 dropped:0 overruns:0 carrier:0
          Kollisionen:0 Sendewarteschlangenlänge:0 
          RX bytes:15676 (15.6 KB)  TX bytes:15676 (15.6 KB)

2) here's the log for ppp0 being enabled:
#ifconfig for ppp0 below
 ifconfig
eth0      Link encap:Ethernet  Hardware Adresse 00:22:41:37:af:61  
          inet6-Adresse: fe80::222:41ff:fe37:af61/64 Gültigkeitsbereich:Verbindung
          UP BROADCAST RUNNING MULTICAST  MTU:1500  Metrik:1
          RX packets:3008 errors:0 dropped:0 overruns:0 frame:0
          TX packets:2993 errors:0 dropped:0 overruns:0 carrier:0
          Kollisionen:0 Sendewarteschlangenlänge:1000 
          RX bytes:2118099 (2.1 MB)  TX bytes:538937 (538.9 KB)
          Interrupt:17 

lo        Link encap:Lokale Schleife  
          inet Adresse:127.0.0.1  Maske:255.0.0.0
          inet6-Adresse: ::1/128 Gültigkeitsbereich:Maschine
          UP LOOPBACK RUNNING  MTU:16436  Metrik:1
          RX packets:227 errors:0 dropped:0 overruns:0 frame:0
          TX packets:227 errors:0 dropped:0 overruns:0 carrier:0
          Kollisionen:0 Sendewarteschlangenlänge:0 
          RX bytes:14378 (14.3 KB)  TX bytes:14378 (14.3 KB)

ppp0      Link encap:Punkt-zu-Punkt-Verbindung  
          inet Adresse:85.236.224.71  P-z-P:85.236.224.65  Maske:255.255.255.255
          UP PUNKTZUPUNKT RUNNING NOARP MULTICAST  MTU:1488  Metrik:1
          RX packets:1060 errors:0 dropped:0 overruns:0 frame:0
          TX packets:1024 errors:0 dropped:0 overruns:0 carrier:0
          Kollisionen:0 Sendewarteschlangenlänge:3 
          RX bytes:654908 (654.9 KB)  TX bytes:184681 (184.6 KB)

REQUEST:
Find a way to assign an IP-address to eth0 when ppp0 over eth0 is in use

WORKAROUND:
Currently I can't use NM at all due to this issue but everything works well with a manual interfaces config-file. This is not the best solution for everyone because when doing it this way you can't use NM to manage VPN, 3G, WLAN and so on. I have to leave Ubuntu 8.04 with the old NM on my laptop for exactly that reason, as I need the old NM to manage my wireless connections and have to use the interfaces-file too.

Here's my current /etc/network/interfaces file that allows me to use eth0 and ppp0 at the same time with assigned IPs on oth connections:

#BEGIN interfaces
auto lo
iface lo inet loopback

auto dsl-provider
iface dsl-provider inet ppp
pre-up /sbin/ifconfig eth0 up # line maintained by pppoeconf
provider dsl-provider

auto eth0
iface eth0 inet static
       address 192.168.0.3
       netmask 255.255.255.0
       gateway 192.168.0.1
#END interfaces

here's the corresponding ifconfig-output:

#begin ifconfig output
root@klaus-imac:/home/klaus# ifconfig
eth0      Link encap:Ethernet  Hardware Adresse 00:22:41:37:af:61  
          inet Adresse:192.168.0.3  Bcast:192.168.0.255  Maske:255.255.255.0
          inet6-Adresse: fe80::222:41ff:fe37:af61/64 Gültigkeitsbereich:Verbindung
          UP BROADCAST RUNNING MULTICAST  MTU:1500  Metrik:1
          RX packets:29 errors:0 dropped:0 overruns:0 frame:0
          TX packets:77 errors:0 dropped:0 overruns:0 carrier:0
          Kollisionen:0 Sendewarteschlangenlänge:1000 
          RX bytes:8804 (8.8 KB)  TX bytes:9078 (9.0 KB)
          Interrupt:17 

lo        Link encap:Lokale Schleife  
          inet Adresse:127.0.0.1  Maske:255.0.0.0
          inet6-Adresse: ::1/128 Gültigkeitsbereich:Maschine
          UP LOOPBACK RUNNING  MTU:16436  Metrik:1
          RX packets:304 errors:0 dropped:0 overruns:0 frame:0
          TX packets:304 errors:0 dropped:0 overruns:0 carrier:0
          Kollisionen:0 Sendewarteschlangenlänge:0 
          RX bytes:20786 (20.7 KB)  TX bytes:20786 (20.7 KB)

ppp0      Link encap:Punkt-zu-Punkt-Verbindung  
          inet Adresse:85.236.224.71  P-z-P:85.236.224.65  Maske:255.255.255.255
          UP PUNKTZUPUNKT RUNNING NOARP MULTICAST  MTU:1488  Metrik:1
          RX packets:17 errors:0 dropped:0 overruns:0 frame:0
          TX packets:18 errors:0 dropped:0 overruns:0 carrier:0
          Kollisionen:0 Sendewarteschlangenlänge:3 
          RX bytes:8002 (8.0 KB)  TX bytes:1061 (1.0 KB)
#end ifconfig-output
Comment 1 Dan Williams 2008-11-05 19:12:55 UTC
*** Bug 559474 has been marked as a duplicate of this bug. ***
Comment 2 Dan Williams 2009-04-08 21:52:02 UTC
*** Bug 561798 has been marked as a duplicate of this bug. ***
Comment 3 Dan Williams 2009-04-08 21:52:59 UTC
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.
Comment 4 Dan Williams 2009-04-20 20:42:48 UTC
*** Bug 579432 has been marked as a duplicate of this bug. ***
Comment 5 Dan Williams 2009-08-25 21:52:20 UTC
*** Bug 592841 has been marked as a duplicate of this bug. ***
Comment 6 Luke Symes 2009-10-01 23:23:36 UTC
@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?
Comment 7 James Mike DuPont 2010-05-28 17:27:14 UTC
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
Comment 8 David Fraser 2010-06-09 08:19:30 UTC
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."
Comment 9 Dan Williams 2010-09-22 21:51:47 UTC
*** Bug 630332 has been marked as a duplicate of this bug. ***
Comment 10 Vitaly 2010-09-23 06:30:44 UTC
>*** 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.
Comment 11 Maksim 2010-10-17 11:04:44 UTC
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.
Comment 12 Dan Williams 2011-02-24 01:28:39 UTC
*** Bug 639578 has been marked as a duplicate of this bug. ***
Comment 13 Jiri Klimes 2012-07-27 07:27:36 UTC
*** Bug 651410 has been marked as a duplicate of this bug. ***
Comment 14 Dan Winship 2013-05-02 16:03:46 UTC
NM bugzilla reorganization... sorry for the bug spam.
Comment 15 Pavel 2014-02-02 12:36:35 UTC
Suffering from this bug, too. Looks like after almost 6 years and several releases of NM, nothing has changed. Really sad.
Comment 16 Pavel Simerda 2014-02-02 12:51:14 UTC
(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.
Comment 17 Pavel 2014-02-02 14:10:32 UTC
(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.
Comment 18 neumond 2017-05-22 03:47:43 UTC
3 more years and still no progress? PPPoE shouldn't use the ethernet device exclusively.
Comment 19 Beniamino Galvani 2017-07-04 11:41:05 UTC
Pushed branch bg/ppp-bgo559134, please review.
Comment 20 Beniamino Galvani 2017-07-05 07:01:33 UTC
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.
Comment 21 Thomas Haller 2017-07-07 16:49:24 UTC
> 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?
Comment 22 Beniamino Galvani 2017-07-18 14:23:47 UTC
(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.
Comment 23 Thomas Haller 2017-08-02 09:02:16 UTC
>> 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.
Comment 24 Thomas Haller 2017-08-02 10:07:09 UTC
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.
Comment 25 Beniamino Galvani 2017-08-03 08:19:26 UTC
(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...
Comment 26 Thomas Haller 2017-08-05 03:42:08 UTC
lgtm now
Comment 27 Beniamino Galvani 2017-08-05 06:10:30 UTC
Merged:

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

nm-connection-editor part still to do...
Comment 28 Beniamino Galvani 2017-08-10 20:12:13 UTC
Created attachment 357356 [details] [review]
[nm-c-e PATCH] editor: pppoe: add support for new-style pppoe connections
Comment 29 Thomas Haller 2017-08-11 09:52:36 UTC
(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.
Comment 30 Beniamino Galvani 2017-09-07 15:32:10 UTC
Created attachment 359362 [details] [review]
[nm-c-e PATCH v2] editor: pppoe: add support for new-style pppoe connections

How about v2?
Comment 31 Thomas Haller 2017-09-07 16:24:40 UTC
(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.
Comment 32 Beniamino Galvani 2017-09-08 08:01:17 UTC
Created attachment 359386 [details] [review]
[nm-c-e PATCH v3 1/2] editor: pppoe: add support for new-style pppoe connections
Comment 33 Beniamino Galvani 2017-09-08 08:01:44 UTC
Created attachment 359387 [details] [review]
[nm-c-e PATCH v3 2/2] editor: pppoe: add tooltips
Comment 34 Beniamino Galvani 2017-09-08 08:03:26 UTC
(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.
Comment 35 Thomas Haller 2017-09-08 08:13:50 UTC
(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).
Comment 36 Beniamino Galvani 2017-09-08 08:41:41 UTC
(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
Comment 37 Thomas Haller 2017-11-13 11:40:18 UTC
*** Bug 724501 has been marked as a duplicate of this bug. ***