net/upnp: Complete service improvements 2/2#5256
net/upnp: Complete service improvements 2/2#5256Self-Hosting-Group wants to merge 3 commits intoopnsense:masterfrom
Conversation
| //$fw->registerAnchor('miniupnpd', 'fw'); | ||
| //$fw->registerAnchor('miniupnpd', 'nat', 0, 'head'); | ||
| //$fw->registerAnchor('miniupnpd', 'binat'); |
There was a problem hiding this comment.
I'm not sure breaking with historically established code is a wise thing to do.
both rdr and "fw" are historically anchored in the code opnsense/core@edc40978997e
nat and binat were added in #3165
| } | ||
|
|
||
| if (!empty($upnp_config['allow_third_party_mapping'])) { | ||
| if (($upnp_config['allow_third_party_mapping'] ?? '') == '1' || ($upnp_config['allow_third_party_mapping'] ?? '') == 'upnp-igd') { |
There was a problem hiding this comment.
feels like this overcomplicates things and imposes "upnp-igd" on the meaning when in reality we're flipping pcp with this and the secure mode simply is handled as a separate toggle?
There was a problem hiding this comment.
It may look overcomplicated to you, but that's only because the daemon initially had the meaningless option name secure_mode for UPnP IGD, PCP support was added later. secure_mode only applies to UPnP IGD, whereas pcp_allow_thirdparty (reversed) only applies to PCP. The same option is also available in OpenWrt, and I cannot imagine why anyone would make two options out of it.
There was a problem hiding this comment.
Fair, still don't like the handling of this with four times ?? '' and two times checking the same flag. something that an in_array() can do much more concisely.
Self-Hosting-Group
force-pushed
the
not-applied
branch
4 times, most recently
from
7216aa4 to
b0852db
Compare
Self-Hosting-Group
changed the title
2 participants
No description provided.