Bug 203695
| Summary: | [patch] net/mpd5: Add RFC 4638 client support (PPPoE MTU > 1492) | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | Ports & Packages | Reporter: | David Wood <david> | ||||||
| Component: | Individual Port(s) | Assignee: | Alexander Motin <mav> | ||||||
| Status: | Closed FIXED | ||||||||
| Severity: | Affects Some People | CC: | franco, garga, koobs | ||||||
| Priority: | --- | Keywords: | needs-qa, patch | ||||||
| Version: | Latest | Flags: | koobs:
maintainer-feedback-
|
||||||
| Hardware: | Any | ||||||||
| OS: | Any | ||||||||
| URL: | https://github.com/opnsense/core/issues/572 | ||||||||
| Attachments: |
|
||||||||
|
Description
David Wood
2015-10-11 10:53:06 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. @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? 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.
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. The right way is to release 5.8. We will try to do it soon. 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. (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. Feeling better now? Happy new year. :) net/mpd5 port updated to version 5.8, that includes RFC 4638 client support. |
