Bug 203695 - [patch] net/mpd5: Add RFC 4638 client support (PPPoE MTU > 1492)
Summary: [patch] net/mpd5: Add RFC 4638 client support (PPPoE MTU > 1492)
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Alexander Motin
URL: https://github.com/opnsense/core/issu...
Keywords: needs-qa, patch
Depends on:
Blocks:
 
Reported: 2015-10-11 10:53 UTC by David Wood
Modified: 2016-01-06 18:49 UTC (History)
3 users (show)

See Also:
koobs: maintainer-feedback-


Attachments
Patch adding RFC 4638 client support (generated using svn diff) (12.86 KB, patch)
2015-10-11 10:53 UTC, David Wood
no flags Details | Diff
Patch (version 2) adding RFC 4638 client support (generated using svn diff) (15.54 KB, patch)
2015-12-28 16:25 UTC, David Wood
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Wood 2015-10-11 10:53:06 UTC
Created attachment 161904 [details]
Patch adding RFC 4638 client support (generated using svn diff)

Kernel support for the RFC 4638 PPP-Max-Payload tag was added to HEAD in base r287654 and MFCed to stable/10 in base r288918 .

The user space support has been developed by Dmitry Luhtionov, one of the mpd5 maintainers. I backported the RFC 4638 support to mpd-5.7, debugged and tested it - see https://sourceforge.net/p/mpd/bugs/54/#14d2 for details.

The attached patch is is patch-rfc4638 and patch-rfc4638-supplementary from the bug comment merged together, then split into one source file per patch file, following normal FreeBSD ports tree practice. I have built and tested the resulting port on my machine, which works correctly.

The patch should not affect the compilation or operation of mpd5 on machines that lack kernel support for the PPP-Max-Payload tag.


DOCUMENTATION

To set an MTU less than or equal to 1492:
set link mtu <n>
(as normal)

To set an MTU greater than 1492:
set pppoe max-payload <n>
(do not use set link mtu, as this leads to a configuration error before being ignored)


IMPORTANT

You must set the MTU of the interface(s) used for PPPoE to a minimum of 8 bytes higher than the desired PPPoE MTU. This means jumbo frame support is required for a PPPoE MTU > 1492.
Comment 1 David Wood 2015-10-12 06:49:52 UTC
This patch has been submitted as a pull request to pfSense at https://github.com/pfsense/FreeBSD-ports/pull/1 where you will find further history and test results.
Comment 2 Kubilay Kocak freebsd_committer freebsd_triage 2015-12-28 09:24:37 UTC
@David given the support has been added to CURRENT and 10-STABLE, where does that leave 9.x? Can/should it be MFC'd there too?

If you can also confirm this change passes QA (portlint, poudriere) that would be great.

This issue is now "maintainer timeout". Mav, can you provide feedback/instructions?
Comment 3 David Wood 2015-12-28 16:25:42 UTC
Created attachment 164770 [details]
Patch (version 2) adding RFC 4638 client support (generated using svn diff)

As in the original commits to mpd5 CVS, the substantive elements of the changes introduced by the patch are wrapped in #ifdef NGM_PPPOE_SETMAXP_COOKIE ... #endif, so will not compile if the kernel lacks RFC 4638 support. Those elements that are not conditionally compiled should amount to a no-op in the absence of kernel RFC 4638 support.

However, in case those expectations are incorrect, I've respun the patch to include the RFC 4638 patches only on FreeBSD when __FreeBSD_version guarantees RFC 4638 kernel support is available.


Ideally, a package built with the RFC 4638 patches included should refuse to install in the absence of kernel support. I can't justify the development time needed to develop a binary that tests directly for RFC 4638 kernel support. Unless I've missed something, there is no way to use __FreeBSD_version to guarantee RFC 4638 kernel support during the PRE-INSTALL run of pkg-install as:

* you cannot query the __FreeBSD_version of the running kernel

* you cannot derive __FreeBSD_version of the running kernel from kernel source, as kernel source might not be installed or might not match the running kernel


The netgraph code in the kernel is mature and has been almost untouched for years, so I am almost certain the RFC 4638 kernel support can be trivially MFCed to 9.x. However, I believe the justification for patching 9.x is too slender to be worth bothering. 9.3 was released in July 2014, no further 9.x releases are planned, and 9.3/9.x is slated to receive security fixes only until end 2016. I believe the wealth of networking enhancements in FreeBSD 10 and up make FreeBSD >= 10 a better networking platform than 9.x to my mind.


portlint -a is not very quiet with the port as it stands. I've addressed most of the historic issues with the port in the revised patch:

* trimming pkg-descr

* Adding 'LICENSE= BSD3CLAUSE' to Makefile

* the comment about the use of an absolute pathname can be ignored, as this is a deliberate fallback when the location of the kernel modules cannot be determined

* the comment about the use of != can be ignored, as there really isn't a more efficient way of expressing this construct

Respinning the patches already in the port using 'make clean patch makepatch' sorts out all the other complaints raised by portlint -a. I don't have a poudriere setup to hand.


My revised patch proposes PORTREVISION= 5 to keep pfSense and FreeBSD ports in sync, though whoever commits this might feel that skipping a PORTREVISION in FreeBSD ports is unacceptable. It would help if pkg had a 'local revision level' for forks or development purposes, which would avoid this problem.


koobs@:

If mav@ is not in a position to carry on as maintainer, I suggest you and garga@ confer about which one of you takes over the port. If neither of you are willing to become the maintainer, I am willing but rather reluctant to become the maintainer as I will be very busy for the next year.
Comment 4 Franco Fichtner 2015-12-29 08:58:15 UTC
The version checking is not a good addition for the simple fact that it'll have to be removed as soon as MPD 5.8 is released. In the meantime, it'll also lock downstream vendors out of the (easily backported) feature unless they modify their ports tree back to a (nonexistent) revision 4.
Comment 5 Alexander Motin freebsd_committer 2015-12-29 13:17:26 UTC
The right way is to release 5.8. We will try to do it soon.
Comment 6 David Wood 2015-12-31 11:46:28 UTC
I wish you well in your moves towards a 5.8 release.

Can you review the patch in https://sourceforge.net/p/mpd/bugs/56/ before you release? It has already been incorporated in the port via this bug.
Comment 7 David Wood 2015-12-31 15:32:46 UTC
(In reply to Franco Fichtner from comment #4)
I put quite a bit of time into testing this mpd5 feature at the point it was just a couple of commits in mpd5 CVS with an associated feature bug. I then looked for the best way to bring it to the wider FreeBSD community in advance of a future mpd5 release, giving due acknowledgement to Dmitry, who committed the code to mpd5 CVS, and the original mpd5 feature bug.

I hadn't carried out sufficient QA to prove the patch was a no-op in absence of kernel support for the RFC 4638 Max-Payload tag. I therefore felt it best to submit a more cautious version of the patch, where inclusion of the RFC 4638 patches was conditional on __FreeBSD_version indicating the necessary kernel support existed. This is far from the only example of a port Makefile using ${OSVERSION} (i.e. __FreeBSD_version) to take conditional actions.

If you choose to cherry pick features from base svn in your downstream project, you must be prepared to deal with consequences of your choice. You are not "locked out" of the feature - you just have to make a single line change that I drew explicitly to your attention in this bug. Ultimately, I left it open to the committer whether to commit the conditional or unconditional version of the patch. I expressed a preference for a particular PORTREVISION and gave justification for that choice, but again, it was up to the committer what to commit.

You added yourself to the CC list on this bug shortly after it was first submitted, Franco. You could have implemented RFC 4638 support in your project using the original version of the patch in early October. I'm baffled as to why you felt the need to contact me for help when someone opened the issue against your project (see https://github.com/opnsense/core/issues/572#issuecomment-167424484 ), as the original bug contained all the documentation you needed. Nevertheless, I spent some time explaining the necessary kernel patch and some implementation considerations in your open issue at https://github.com/opnsense/core/issues/572#issuecomment-167602766

If you are going to patch base in a downstream project, you must accept that some patches against ports will likely be necessary.

I suggest that it is in poor taste to complain publicly about the actions of those who have gone out of their way to help you, especially when I had, via this bug, placed everything you needed to implement RFC 4638 client support in your project into the public domain over two months ago.
Comment 8 Franco Fichtner 2015-12-31 17:01:07 UTC
Feeling better now? Happy new year. :)
Comment 9 Alexander Motin freebsd_committer 2016-01-06 18:49:36 UTC
net/mpd5 port updated to version 5.8, that includes RFC 4638 client support.