new ppp release ? #111
Comments
|
Yes, a new release is very much needed - ppp no longer compiles on Gentoo. This has been fixed in commit 3c7b862 - "pppd: Use openssl for the DES instead of the libcrypt / glibc". Please consider creating a new release ASAP. |
Part of the problems of the ppp as the project is completely crappy and custom made set of makefiles and custom configure script. |
|
+1 (since 2014) Note: There are patches here: https://www.nikhef.nl/~janjust/ppp/ Please look this ticket: #131 EDIT : Merged, good job @jjkeijser! |
|
I'm begging to make new ppp release :/ |
|
@paulusmack, @joakim-tjernlund, @hansfn, @kloczek: What do you think for an official perfect build? |
|
A release would be great but seems like github is not the place Paulus wants to discuss, see |
|
Right. I do prefer mailing lists (i.e. the linux-ppp@vger.kernel.org list). However, since we're here, do you all think the tree in its current state is releaseable, or are there things that must be fixed before release? |
|
Paul, Please understand that asking to subscribe on mailing list to ask the question is kind of overkill. Mailing lists are OK in case of long term commitment to join the project. For anything else github issue tickets are all what is needed. |
|
So going back to the subject: do you have any plans to make new release soon? :) |
|
@paulusmack: The list is a big mess... Can you look for integrate patches here too? https://www.nikhef.nl/~janjust/ppp/ Please look this ticket: #131 EDIT : Merged, good job @jjkeijser! |
What list? The mailing list, or are you referring to some list of patches, or some other list?
That's rather large. I would need a signoff at least. I would also want James Carlson to review and ack it. Please post it to linux-ppp (with a Signed-off-by, and a From: line if you are not the author) so he sees it. Please also consider splitting it, if that would make a series of logical steps that are easier to review. |
Puke. My experience of autoconf/automake has been that their output is verbose, unreadable and undebuggable. For a small project like ppp that only supports Linux and Solaris there isn't a lot that needs to be configured, so I prefer something that's simple and direct and understandable. |
Yes, I would like to make a new release soon. The question is whether I just release what's in the tree now, or if there are some patches that are really needed before release. |
|
@paulusmack I suggest release a version first, and then bugfixes it |
|
I propose to release the tree as it currently is (plus updates to README and pppd/patchlevel.h) as ppp-2.4.8 shortly. |
|
Feels OK, further fixes/features can go into 2.4.9 |
|
Maybe add #53 too? |
|
IMO priority for now should be first release all already committed changes than as often as possible release all pending things in smaller chunks. |
|
I'm going to do another release soon with the CVE fix. I am not going to merge any of the EAP patches for this release, as they are all fairly large and all need more review. I have merged a few small and safe fixes and I could add some more as long as they are obviously correct. |
|
If may I ask you to have look on all Fedora parches https://src.fedoraproject.org/rpms/ppp/tree/master |
There is also a lot of pending patches, at least, in Debian and OpenWrt: Debian (individual patches in debian/patches/ ): OpenWrt (individual patches in package/network/services/ppp/patches/ ): Fedora: |
Thanks for the references, that is helpful. While there are some patches in those repos that I can (and will) apply, most of the patches in those distro packages are generally not up to the standard that I want:
(The openwrt patches do seem to be better than most in these respects.) |
Added current maintainers to the thread: Chris Boot(debian), Jaroslav Škarvada(fedora) and Felix Fietkau(openwrt) |
|
First it would be good to make new release ASAP with what is already commuted. Problem is so far maintainer is not responding (hopefully he is just only busy :) ) |
|
@bootc (Debian), @yarda (Fedora), @nbd168 (OpenWrt), @dmbaturin (VyOS) and others: Guys, can you create a PR with your patches? It will be nice to have directly in the next build? Thanks in advance. |
|
@Neustradamus #101 is the only remaining significant difference between our package and the upstream. |
|
I do not have a desire to do PR for the project, where PRs have been hanging for more than 6 years. |
|
@Neustradamus For the record, our "ppp-debian" repo is just Debian's upstream with the patch from #101 and some misc security fixes that are already offered in other PRs. Once #101 is merged we'll be able to switch to the upstream version. @IlyaIndigo This is a rather uncomfortable discussion, but I do believe that if Paul doesn't have time and motivation to maintain the package, he should add someone to the repo owners. In the current situation, I can't see any of the semi-forks becoming a new de facto standard—none of them has enough momentum by itself. In new VyOS releases, we switched from pppd to https://github.com/xebd/accel-ppp for the L2TP/PPTP/PPPoE servers: it offers great performance and it has maintainers passionate about it. I'm pretty sure there's someone equally passionate about the original ppp who can take up the maintenance work, given enough permissions to do that. |
I have notified Reinhard about this issue. |
|
Today, one year after 2.4.9 and a lot of contributions for a new release! Hope merging of current PRs and and hope to have a new PR from @EasyNetDev with this contribution before the new build. @enaess: When you will have time to look? cc: @paulusmack. |
|
@Neustradamus time to look for what exactly? I likely won't be able to do any significant work before mid February at this point. |
|
@enaess: Have you looked?: "Just haven't had the time to work on it :-" @EasyNetDev.: Have you looked for your PR? If not, maybe @paulusmack can now release the new version after several months? There is a new PR and new tickets: |
|
@ALL OS PPP maintainers, if you have done a or several patches which are not in master, please look to add PR in upstream. |
|
@Neustradamus I agree it's time for a next release. Could you please indicate expected time lines? Would it be possible to consider my suggestions here? Happy to write the code if it'll be considered, and can be done within your expected time lines. I don't want to be the cause of additional delay. |
|
@jkroonza: I do not know, it is a recall to all devs ^^ The project is managed by @paulusmack, but I think soon... Normally the build has been planned a long time ago, January 2022 but one PR has been changed it... |
|
Would love to, time is constrained at the moment so interface naming isn't particularly high on our list of priorities unfortunately. Happy to tack that onto my list but that won't be able to receive attention in the next while unfortunately. |
|
nixpkgs currently carries these patches:
|
Does this still apply to master? I can't find a Makefile.linux of any kind in the tree. And ./configure contains the logic you're looking to implement via --with-openssl= here (you can hand it a path, or it'll try the usual culprits, but yes, trying pkg-config first may not be a bad idea in the case of --with-openssl=yes, it looks like pkg-config is tried only after the check the set of paths has failed. So some logic improvements may be considered here, but IMHO is probably overkill. The comments in ./configure are stating that pkg-config is preferred, but looking at the actual code that's not the case.
No, that seems very, very wrong, basically privileged = true there :). Ie, non-root users have privileges of root. |
|
Yocto project currently carries this: |
It does apply to 2.4.9: But yes, it looks like we'll have to revise that for a new release anyway since it's indeed no longer present on master.
Indeed. I just found the justification for it: Even if that's the case, this is certainly the wrong approach and I'll remove it. |
Don't think you have to, ./configure already does what you need, so you might just need to adjust your ./configure arguments.
Doesn't seem to be a problem on at least 2.4.9. Quick glance at the code agrees, no special privileges should be required. |
|
@kanavin: I think yes, it has been removed: Note: when we search IPX in code: |
Will also no longer apply. And I think you're sorted: There are still references in the sources to IPX if you grep -ri ipx in there, but I suspect these can for the most part safely remain. I would suggest that these all be removed, except for the README which should probably be updated to mention that IPX is no longer supported since 2.5.0 (or whatever the new release version will be). Perhaps the stuff in options.c was left specifically to still allow "noipx" to be passed, but (in my personal opinion) that's counter-productive since that implies there may still be support, which there is not, and perhaps a warning to be output is more appropriate. Basic support to recognise the protocol should the remote try to initiate should be fine, basically warn locally that the remote is looking to bring up IPX and then nack the protocol. If noipx is passed on a cli a similar warning may be more suitable that simply setting to option locally and then not doing anything it. |
|
I think last PRs before the new build will be: |
|
@Neustradamus I actually concur with the decision to keep things blocking a new release minimal, however, may I please request consideration of #367 - or at least guidance on required, and if I can get that ready prior to confirmed merge of above PRs to be considered? We're already using that in productio, and so far it solves all the issues we picked up with earlier versions of the patches which lead to that, and I believe most of the concerns raised were addressed in one way or another, but I'm happy to take further consideration into regards, I do think there are other concepts I would have LIKED to implement mentioned there, but I do not believe those are practical, for reasons indicated in same PR. |
|
On 12/29/22 10:10, Jaco Kroon wrote:
@Neustradamus <https://github.com/Neustradamus> I actually concur with
the decision to keep things blocking a new release minimal, however, may
I please request consideration of #367
<#367> - or at least guidance on
required, and if I can get that ready prior to confirmed merge of above
PRs to be considered? We're already using that in productio, and so far
it solves all the issues we picked up with earlier versions of the
patches which lead to that, and I believe most of the concerns raised
were addressed in one way or another, but I'm happy to take further
consideration into regards, I do think there are other concepts I would
have LIKED to implement mentioned there, but I do not believe those are
practical, for reasons indicated in same PR.
I'm a little confused by the intent of this change (#367). There are a
couple of reasons that the sifup/sifdown paths (which are by design
intended *ONLY* to set flags, not to run scripts) are separated. One is
that different platforms have different ways to represent multiple
addresses on a given interface.
This is a place where I think Solaris differs substantially from Linux.
Solaris has "logical interfaces." A logical interface is the entity that
holds the address and a set of independent flags for *EACH* address.
That is, unlike Linux, each network layer protocol (actually each
*address*) can be marked up or down independently.
On Solaris, the SIOC[SG]IF* ioctls and the SIOC[SG]LIF* ioctls are
different. The [SG]IF ioctls deal with legacy-style BSD-ish interfaces
(dating back to SunOS 4.x days), while the [SG]LIF ioctls deal with
logical interfaces. In the proposed change, all of the "LIF" handling
has been deleted from the code base. I haven't tested this, but I
wouldn't be surprised if this caused problems. (The ioctl split exists
in order to maintain binary compatibility with SunOS applications.)
For example, on a Solaris 11 machine, I see this:
lo0: flags=2001000849<UP,LOOPBACK,RUNNING,MULTICAST,IPv4,VIRTUAL> mtu
8232 index 1
inet 127.0.0.1 netmask ff000000
net0: flags=1000843<UP,BROADCAST,RUNNING,MULTICAST,IPv4> mtu 1500 index 2
inet 10.50.31.94 netmask ffff0000 broadcast 10.50.255.255
ether 8c:73:6e:c0:e:8a
lo0: flags=2002000849<UP,LOOPBACK,RUNNING,MULTICAST,IPv6,VIRTUAL> mtu
8252 index 1
inet6 ::1/128
net0: flags=20002004841<UP,RUNNING,MULTICAST,DHCP,IPv6> mtu 1500 index 2
inet6 fe80::8e73:6eff:fec0:e8a/10
ether 8c:73:6e:c0:e:8a
net0:1: flags=20002004841<UP,RUNNING,MULTICAST,DHCP,IPv6> mtu 1500 index 2
inet6 fd48:3e6a:e278::e30/128
Things to note in this example (sorry about the line wrap): there are
two distinct instances of "net0." They are distinguished by address
family. And there's a separate "net0:1" logical interface that was
created by DHCPv6 and is managed by that protocol.
And for IPv6, in general, only the LIF ioctls will work on Solaris, due
to the change in interface (wider values in the ioctl data structure).
As for the rest of the change, I'm not sure I follow what the ultimate
architectural goal of this might be. To me, each NCP is (somewhat
unfortunately) different in terms of what it actually negotiates and
what the units of currency are. This means that, for instance, IPv4
negotiates distinct endpoint addresses that are used at the application
layer, but IPv6 negotiates only the IDs used for link-locals (which no
application should touch), and MPLS has no addresses at all (LDP does
that). And that's ignoring other more exotic protocols such as AppleTalk
that negotiate a separate network number and node address. All of this
means that the environment variables and the command line arguments
(again, unfortunately) differ for these external scripts, and is another
reason these code paths are separate. I don't see what the grand script
unification does except maybe cause compatibility issues.
Moreover, there's no notion in PPP of having reached a stage of "fully
up." Any link can bring up or tear down an NCP at any time, so if the
ultimate goal is to get to a point where we can just invoke one "I'm up"
script, I don't think that can be done in any coherent way. (I'm not
sure if that's where this was going, but just a guess.)
Some minor issues:
- If we're going to delete 'unit' numbers, let's not do it piecemeal.
Let's have a single change that nukes them all, rather than a series
of changes that arbitrarily yank them from one old interface or
another. I'm fine with removal of that, as it was used only be a few
odd non-public branches of the code, and is hard to maintain
correctly when "nobody" is using it. I'm just not fine with
incremental breakage; it seems unprincipled to me. (As this change is
doing with sifup et al. Yes, I know it's not just this change; this is
a pet peeve.)
- Definitely agree with the other commenters saying that comparing
static strings by address only is not a good idea. It saves almost
nothing in execution time, and opens up a neat hole for someone else
to fall into by way of a bad compilation flag, a new pppd plugin,
or some other contrivance. I don't see why this mechanism is so
complex, when we already have per-NCP registration mechanisms for
the FSM and this sort of state information could be carried there,
but if it must be separate and must be done with a global array,
then at least use strcmp.
- I saw some discussion on the issue of running the "down" script
before the link goes down. It might seem nice to run the script
before a locally-initiated tear-down starts, but that should never
be given as a feature to the user, because there's no general way to
make it work. The other end can terminate at any time, and in practice
the network connection itself may well have ended before any sort of
notification is received (which may also come in the form of a simple
loss of carrier). So, sure, if you can invoke it early, and believe it
to be important, then do so, but don't advertise it too strongly in
any documentation or suggest that anyone should rely on it.
(For what it's worth, I like interfaces that give consistent semantics
rather than "helpful" ones. In other words, I'd prefer to see the
"down" script run after the link goes down in all cases, so users
can't become dependent on happenstance. But I guess others may
differ or have particular limited usage cases in mind.)
…--
James Carlson 42.703N 71.076W FN42lq08 ***@***.***>
|
OK.
Right. I was not aware of this. Does that actually happen that way? Because the various functions simply marks the interface up/down in the case of pppd, not a specific protocol/address here.
I can only test on Linux unfortunately. I looked at the code, and as far as I could determine, the code which was removed was 100% identical. I could have missed something. The primary aim here was to have a consolidated if up/down interface, which then did all the common work, around dispatching to the OS specific functions. Not to change behaviour, but to enable adding OS and protocol-independent up/down work to interfaces without having to do this in multiple places.
OK, so the question here is if it's possible to UP and/or DOWN the IPv4 vs IPv6 net0 interfaces (and even net0:1 independently of net0)? Based on what you stated, it sounds like a "yes it can", but can you please confirm? From what I recall the pppd code most certainly did not do that. And in fact, there was quite a bit of confusing stuff around upping and downing interfaces, and it looked like it's possible for IPv6 to tear down, and bring down the entire interface (ie, ifdown) without actually tearing down LCP as a whole, leaving pppd in a situation where it could consider IPv4 and MPLS etc to be "up", but due to the interface being down it's actually down. If you're right (I'm not saying you're wrong, just looking to understand better) about the way solaris handled this then I'm way off base and will probably need to rethink this a bit.
I think this touches on some of the discussion and issue points. So what I desperately want is an init script to execute right at the start of pppd (net-init as implemented in that PR), then scripts to execute just before upping the interface (protocol independent, net-preup, after auth, but prior to setting the interface up), and then to execute when tearing down (less critical, but we do have lated ifb interfaces used for shaping ingress traffic from the ppp interfaces which needs to be torn down too).
Wasn't my intention to break anything, and I did take as much care as I could to maintain backwards compatibility. I do agree with your pet peeve and I can only state that was not my intention. You reference 'unit' numbers here specifically, I do recall yanking certain parameters which I think could have been unit numbers, they were yanked from functions I was modifying where to the best of my ability to determine they were not being used anyway. You reference "odd non-public branches" that may have used them, of this I was not aware. I also didn't spot specific other places in the (fairly large) codebase where they could be removed, but I could well have missed stuff, obviously, which is why we have reviews of code before merging :).
OK, so add this to the per protocol structure is what I'm getting? This may also allow the one requested additional feature called net-up to be invoked once all protocols have been brought up. The one other requested change was to only actually up the interface once all desired protocols have been negotiated and brought up, but as stated, that would require changes to most sub protocols which can obviously be done. Specifically, sub protocols would need to signal "configuration done, ready to be upped, please call this callback when interface is up". This can be done, and I'm happy to put in the effort for the in-tree protocols. I would, however, recommend that this be configurable behaviour. If we're going that far, I'd further recommend that any and all other pre-up scripts be nuked, since already they have no guarantee that the interface is actually still down. Alternatively, we need to follow the above methodology and enforce that. This is perhaps a bigger change than what should be merged at this point though, and could cause further delays to an overdue ppp release.
Agreed about the consistency, will gladly make this change. They way I understood it was that locally the interface will still reflect UP, but I do agree with you, it will already no longer be operational if remote side tears down, so conceding to rather execute post down. This was a debate in my head and honestly it doesn't particularly matter that much. Appreciate your inputs, I'll copy this discussion to that PR to be furthered there, so that the actual release can be discussed here, rather than alternative PRs. |
|
On 12/30/22 01:58, Jaco Kroon wrote:
There are a couple of reasons that the sifup/sifdown paths (which
are by design intended /ONLY/ to set flags, not to run scripts) are
separated. One is that different platforms have different ways to
represent multiple addresses on a given interface. This is a place
where I think Solaris differs substantially from Linux. Solaris has
"logical interfaces." A logical interface is the entity that holds
the address and a set of independent flags for /EACH/ address. That
is, unlike Linux, each network layer protocol (actually each
/address/) can be marked up or down independently.
Right. I was not aware of this. Does that actually happen that way?
Because the various functions simply marks the /interface/ up/down in
the case of pppd, not a specific protocol/address here.
In Solaris, a logical interface *is* a specific protocol and a specific
address. Marking an interface by name does that. It's not the same as Linux.
On Solaris, the SIOC[SG]IF* ioctls and the SIOC[SG]LIF* ioctls are
different. The [SG]IF ioctls deal with legacy-style BSD-ish
interfaces (dating back to SunOS 4.x days), while the [SG]LIF ioctls
deal with logical interfaces. In the proposed change, all of the
"LIF" handling has been deleted from the code base. I haven't tested
this, but I wouldn't be surprised if this caused problems.
I can only test on Linux unfortunately. I looked at the code, and as far
as I could determine, the code which was removed was 100% identical. I
could have missed something. The primary aim here was to have a
consolidated if up/down interface, which then did all the common work,
around dispatching to the OS specific functions. Not to change
behaviour, but to enable adding OS and protocol-independent up/down work
to interfaces without having to do this in multiple places.
Here's what was missed:
if (ioctl(ipfd, SIOCGIFFLAGS, &ifr) < 0) {
if (ioctl(fd, SIOCGLIFFLAGS, &lifr) < 0) {
Two big differences there:
1. The IPv4 code uses the common "ipfd" file descriptor, which is open
on "/dev/udp" for somewhat legacy reasons. The IPv6 code uses a
temporary IPv6 UDP socket. (Not sure why; the code likely predates
the addition of the ipfd6 variable. If I ever run into Adi Masputra
again, I'll have to ask.) This is where the address family comes
in.
2. The IPv4 code uses SIOCGIFFLAGS. The IPv6 code uses SIOCGLIFFLAGS.
The two are not the same; GIF != GLIF. I agree the naming here is
an eye test, though. It predates my time at Sun. (Technically, the
IPv4 code could use SIOCGLIFFLAGS if it wanted, but it doesn't.
This is likely a nod to systems that predate Solaris 8. The world
didn't know about "LIF" before the year 2000.)
As for the consolidation, it doesn't seem to match the existing system
design, so I find it just confusing. I'm really not sure how it helps.
With other network protocols, I think there's no guarantee at all that
they'd use the somewhat IP-specific infrastructure for interfaces across
platforms. I guess maybe they could.
ioctl data structure). As for the rest of the change, I'm not sure I
follow what the ultimate architectural goal of this might be. To me,
each NCP is (somewhat unfortunately) different in terms of what it
actually negotiates and what the units of currency are. This means
that, for instance, IPv4 negotiates distinct endpoint addresses that
are used at the application layer, but IPv6 negotiates only the IDs
used for link-locals (which no application should touch), and MPLS
has no addresses at all (LDP does that).
OK, so the question here is if it's possible to UP and/or DOWN the IPv4
vs IPv6 net0 interfaces (and even net0:1 independently of net0)?
Yep.
Based
on what you stated, it sounds like a "yes it can", but can you please
confirm?
Yes. Here's an example from a running system. I'll add a new IPv6
address to net0:
# ifconfig net0 inet6 addif 5::2
Created new logical interface net0:2
That just created a new logical interface. That interface is not yet
"up" (administratively enabled, vs the "running" flag which is a
hardware indication):
net0: flags=20002004841<UP,RUNNING,MULTICAST,DHCP,IPv6> mtu 1500 index 2
inet6 fe80::8e73:6eff:fec0:e8a/10
ether 8c:73:6e:c0:e:8a
net0:1: flags=20002004841<UP,RUNNING,MULTICAST,DHCP,IPv6> mtu 1500 index 2
inet6 fd48:3e6a:e278::e30/128
net0:2: flags=20002000840<RUNNING,MULTICAST,IPv6> mtu 1500 index 2
inet6 5::2/64
Note that last entry and compare with the others. If the interface isn't
up, the system behaves as if the address doesn't exist. It'll never
choose it as a source address. It'll never treat a packet sent to that
address as "for self" and will instead attempt to forward to some other
machine via the IP forwarding table.
If I try to mark it as "up" using IPv4, it doesn't work, because the
logical interfaces are segregated by address family:
# ifconfig net0:2 up
ifconfig: setifflags: SIOCGLIFFLAGS: net0:2: no such interface
I can mark it up using IPv6 instead:
# ifconfig net0:2 inet6 up
And then I see this:
net0:2: flags=20002000841<UP,RUNNING,MULTICAST,IPv6> mtu 1500 index 2
inet6 5::2/64
The notion of up vs down for logical interfaces is deeply embedded in
Solaris. For instance, this is how duplicate address detection is
implemented on broadcast type media: if a duplicate address is detected,
the system marks the affected logical interface with the IFF_DUPLICATE
flag and removes IFF_UP, so the interface is taken off-line until the
problem is fixed.
It's an inherent part of the system model.
(I admit I dunno how that works on Linux.)
From what I recall the pppd code most certainly did not do
that. And in fact, there was quite a bit of confusing stuff around
upping and downing interfaces, and it looked like it's possible for IPv6
to tear down, and bring down the entire interface (ie, ifdown) without
actually tearing down LCP as a whole, leaving pppd in a situation where
it could consider IPv4 and MPLS etc to be "up", but due to the interface
being down it's actually down. If you're right (I'm not saying you're
wrong, just looking to understand better) about the way solaris handled
this then I'm way off base and will probably need to rethink this a bit.
I don't think any of that is true. That's why Linux had the "if_is_up"
counter. Because on Linux the whole "IP interface" is up if any of the
IP protocols is up, and it goes down only when the last protocol goes away.
Other systems are different.
And that's ignoring other more exotic protocols such as AppleTalk
that negotiate a separate network number and node address. All of
this means that the environment variables and the command line
arguments (again, unfortunately) differ for these external scripts,
and is another reason these code paths are separate. I don't see
what the grand script unification does except maybe cause
compatibility issues. Moreover, there's no notion in PPP of having
reached a stage of "fully up." Any link can bring up or tear down an
NCP at any time, so if the ultimate goal is to get to a point where
we can just invoke one "I'm up" script, I don't think that can be
done in any coherent way.
I think this touches on some of the discussion and issue points. So what
I desperately want is an init script to execute right at the start of
pppd (net-init as implemented in that PR), then scripts to execute just
before upping the interface (protocol independent, net-preup, after
auth, but prior to setting the interface up), and then to execute when
tearing down (less critical, but we do have lated ifb interfaces used
for shaping ingress traffic from the ppp interfaces which needs to be
torn down too).
Here are the points that we have:
1. pppd is initially launched. No need for a special script here, as
the launcher can just do whatever is needed.
2. pppd completes negotiating LCP but has not yet done AUTH (if any).
We don't have this now. Not sure if it's important. It could be
added.
3. pppd completes authentication of the peer and has not yet done
any of the NCPs. This is auth-up.
4. pppd completes negotiating an NCP but has not yet marked the
network interface as "up" for traffic so no data is flowing yet.
This is ip-pre-up. (Sadly, we have this for IPv4 only. Someone
might want to add ipv6-pre-up.)
5. pppd has marked the network interface "up" and traffic can now
flow. This is ip-up and ipv6-up (and others where supported).
6. pppd has lost a network interface that was "up." This is ip-down
and ipv6-down (and others).
7. pppd has lost an authenticated peer. This is auth-down.
"net-init" sounds a whole lot like point (1) to me. I don't see why
anyone needs a special hook for this. The really narrow case that could
be made (in the current system design) would be to have a script that
fires when the interface is activated, just to support the rarely used
"on-demand" feature in pppd, and also have that script fire immediately
on the usual sort of pppd invocation. I guess I could get behind that,
but it's definitely not like this proposed change at all.
"net-preup" already exists. That's point (4). Just use ip-pre-up.
Tearing down also exists. That's point (6).
So I'm not exactly sure what's being fixed here.
(I'm not sure if that's where this was going, but just a guess.)
Some minor issues:
* If we're going to delete 'unit' numbers, let's not do it
piecemeal. Let's have a single change that nukes them all,
rather than a series of changes that arbitrarily yank them from
one old interface or another. I'm fine with removal of that, as
it was used only be a few odd non-public branches of the code,
and is hard to maintain correctly when "nobody" is using it. I'm
just not fine with incremental breakage; it seems unprincipled
to me. (As this change is doing with sifup et al. Yes, I know
it's not just this change; this is a pet peeve.)
Wasn't my intention to break anything, and I did take as much care as I
could to maintain backwards compatibility. I do agree with your pet
peeve and I can only state that was not my intention. You reference
'unit' numbers here specifically, I do recall yanking certain parameters
which I think could have been unit numbers, they were yanked from
functions I was modifying where to the best of my ability to determine
they were not being used anyway. You reference "odd non-public branches"
that may have used them, of this I was not aware. I also didn't spot
specific other places in the (fairly large) codebase where they could be
removed, but I could well have missed stuff, obviously, which is why we
have reviews of code before merging :).
I was specifically referring to the change for sifup() and sifdown() to
remove the unit number and replace with the renamed set_ifup() and
set_ifdown().
* Definitely agree with the other commenters saying that comparing
static strings by address only is not a good idea. It saves
almost nothing in execution time, and opens up a neat hole for
someone else to fall into by way of a bad compilation flag, a
new pppd plugin, or some other contrivance. I don't see why this
mechanism is so complex, when we already have per-NCP
registration mechanisms for the FSM and this sort of state
information could be carried there, but if it must be separate
and must be done with a global array, then at least use strcmp.
OK, so add this to the per protocol structure is what I'm getting? This
may also allow the one requested additional feature called net-up to be
invoked once all protocols have been brought up.
There's no way to do that in the general case. The underlying protocol
doesn't really support what you're asking for here. The NCPs are
intentionally independent, so there's no "last" one.
We could do something that's somewhat like this, but the semantics would
be at least a bit odd. At the point where all enabled network layer
protocols have either moved to stopped or opened state following the
completion of authentication, we could fire a script. The semantics
would be something like "all configured network layers ready." But it'd
be a little weird, because you could have a link where the remote admin
has initially left IPv6 turned off, but then decides to turn it on
later. In that case, we'd fire that "all ready" script, and then much
later fire "ipv6-up" when it's administratively enabled.
As long as that sort of oddity is ok, and can be reasonably documented,
I guess I'm ok with that.
The proposed change doesn't seem to resolve this issue at all. Doing
that sort of thing requires a survey across the NCPs.
The one other requested change was to only actually up the interface
once all desired protocols have been negotiated and brought up, but as
stated, that would require changes to most sub protocols which can
obviously be done. Specifically, sub protocols would need to signal
"configuration done, ready to be upped, please call this callback when
interface is up". This can be done, and I'm happy to put in the effort
for the in-tree protocols. I would, however, recommend that this be
configurable behaviour.
Oof.
Why?
I think that roughly 99 and 44/100ths of this can be done using
reasonably well crafted scripts with the existing interfaces. Granted,
it looks like if you negotiate both IPV6CP and IPCP on the same link
then it might be possible that ip-pre-up can fire "too late" but ON
LINUX ONLY, so that's a Linux specific bug.
If we're going that far, I'd further recommend that any and all other
pre-up scripts be nuked, since already they have no guarantee that the
interface is actually still down. Alternatively, we need to follow the
above methodology and enforce that.
That is, I think, a Linux-specific problem, not a generic issue with
pppd. It's a shame that Linux only has a single flag for the whole
interface, and I don't know exactly how to work that out in a reasonable
form for Linux. It sounds like a system defect to me.
I guess I wouldn't mind seeing "if-pre-up", "if-up", and "if-down"
scripts that are invoked on Linux only when the whole interface is
marked up and down by pppd. Not entirely sure if that fixes your
problem, but it sounds like something that sys-linux.c could do without
hurting others.
It's also possible that what you're really proposing is introducing a
new "phase" for pppd, and I think that'd need a little more discussion.
This would be a phase after NCP that represents "all NCPs resolved" (as
described above), and possibly on Linux only, would be a place where the
interface is finally marked "up." That seems like a more radical change
than what's proposed here, but at least seems consistent with the
existing design.
Or, in a simpler model, we could just have a new pppd option that
disables the modification of the IFF_UP flag by pppd, leaving that part
to external scripts (or programs) to resolve. Then you can do whatever
you want in your "if-up" and set the flag at your leisure. You might
still need the "all NCPs" script to make that work.
Otherwise, removing the existing scripts would also be a pretty big and
incompatible change. I can't see how something like that would ever
reach consensus without a fork. Even then, I think it'd be a long term
disaster.
…--
James Carlson 42.703N 71.076W FN42lq08 ***@***.***>
|
|
Drop #367 from a new release, I'll see if I can clean that up for a future release rather. |
|
From @paulusmack: "I think the tree is ready for release. Last call for anything that really needs to be fixed before 2.5.0..." |
|
2.5.0 is out now. |
|
Hello all, About 2.5.0: @bootc, @rfc1036, @sthibaul: Any progress for Debian? @nbd168: It is in progress for OpenWrt by @icegood: @yarda, @AdamWill, @Athwale: Fedora OK @nekopsykose, @WhyNotHugo, @ncopa, @oliv3r, @kaniini, @maxice8, @nero, @jirutka, @kunkku, @vakartel, @fabled, @clandmeter: Alpine OK @thesamesam, @juippis, @arthurzam, @floppym, @SoapGentoo, @jsmolic, @asarubbo, @xen0n, @inode33, @ulm, @dm0-, @Zlogene: Gentoo OK @demmm: KaOS OK @Chocobo1: AUR (Arch Linux User Repository) OK @rpurdie, @kanavin, @rossburton, @zeddii, @rpjday, ...: OpenEmbedded / Yocto OK @rxrbln: T2 SDE OK @peterhoeg, @yu-re-ka, @Artturin, @alyssais, @mweinelt: NixOS OK CRUX OK Thanks to all people who have updated PPP version in OS... |
|
The debianization of 2.5.0 has been on https://salsa.debian.org/debian/ppp/-/merge_requests/12 for two months, but I have never received any feedback from @bootc, who is the maintainer. |
|
The problem with gitlab is that by default you don't necessarily get a notification of a new merge request etc. It's probably worth reaching the maintainer by mail to make sure that he did get a notification. |
|
please don't ping dozens of people just to say "OK", thanks |
|
@nekopsykose: Maybe you have not seen the last sentence: "Thanks to all people who have updated PPP version in OS..." :) Thanks @rfc1036 and @sthibaul for your answer, I have commented the @rfc1036 PR... |
|
Dear all, @paulusmack will prepare a 2.5.1 release build and ask some questions: Can you look it? Thanks in advance. |
and others
The last ppp release was quite some time ago. Is a new one forthcoming ?
The text was updated successfully, but these errors were encountered: