igmpproxy: add config parameter to enable ssdp #25156
Conversation
BKPepe
force-pushed
the
igmpproxy-ssdp-clean
branch
from
d5e9681 to
b770968
Compare
|
Hi, what is required to accept this pull request? As far as I can tell, all tests passed in the past and @systemcrash did already give his approval. Thanks! |
|
No merge commits. Just rebase on master. |
thebub
force-pushed
the
igmpproxy-ssdp-clean
branch
from
803dbc4 to
b770968
Compare
|
I hope I fixed it @systemcrash. Sorry for that. |
github-actions
bot
added
Add package
Drop package
and removed
Add package
Drop package
labels
|
Nope - there's still a merge commit in there. Your commit branch should be based on openwrt:master. |
thebub
force-pushed
the
igmpproxy-ssdp-clean
branch
2 times, most recently
from
b770968 to
0d8c1ca
Compare
|
@systemcrash Thanks for the feedback and your patience. I retried. Please advise. |
|
Looks good now. |
So right now, a review by a maintainer with write access to the repo is missing, right? |
thebub
force-pushed
the
igmpproxy-ssdp-clean
branch
from
0d8c1ca to
6658490
Compare
thebub
force-pushed
the
igmpproxy-ssdp-clean
branch
from
6658490 to
d9e2266
Compare
thebub
changed the title
|
@BKPepe sorry for failed run. I fixed the findings of the Copilot code review. |
I can‘t see an issue in the logs. Please advise! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
net/igmpproxy/files/igmpproxy.init
Outdated
| config_get_bool ssdp $1 ssdp 0 | ||
|
|
||
| if [ $ssdp -gt 0 ]; then | ||
| local ssdp_target="ACCEPT" |
There was a problem hiding this comment.
The 'local' keyword should not be used when reassigning an already declared variable. Line 67 already declares 'ssdp_target' as local, so line 82 should be 'ssdp_target="ACCEPT"' without the 'local' keyword. Using 'local' here creates a new variable in the nested scope, which means the assignment won't affect the ssdp_target variable used on line 92, and the target will always be "DROP" regardless of the config setting.
| local ssdp_target="ACCEPT" | |
| ssdp_target="ACCEPT" |
Adde a config parameter to enable SSDP Keep existing default behavior Bump release version Maintainer: @nbd168 Compile tested: openwrt/sdk:x86-64-openwrt-23.05 & openwrt/sdk:ramips-mt7621-23.05 Run tested: ramips/mt7621, TP-Link Archer C6 v3, OpenWrt 23.05.5 r24106-10cc5fcd00, tested with default config, disabled option & enabled option Description: The previous implementation of the init script always disabled SSDP. The new implementation adds a config flag bearing the option not to disable SSDP. However, the default behavior will not change and SSDP will stay disabled by default. Especially Sonos users, who want to use SSDP forwarding, will benefit from having the choice whether to DROP or ACCEPT. Signed-off-by: Daniel Bub <daniel.bub@thebub.net>
Signed-off-by: Daniel Bub <daniel.bub@thebub.net>
thebub
force-pushed
the
igmpproxy-ssdp-clean
branch
from
455a381 to
e49367e
Compare
2 participants
Maintainer: @nbd168
Compile tested: openwrt/sdk:x86-64-openwrt-23.05 & openwrt/sdk:ramips-mt7621-23.05
Run tested: ramips/mt7621, TP-Link Archer C6 v3, OpenWrt 23.05.5 r24106-10cc5fcd00, tested with default config, disabled option & enabled option
Description:
The previous implementation of the init script always disabled SSDP. The new implementation adds a config flag bearing the option not to disable SSDP. However, the default behavior will not change and SSDP will stay disabled by default.
Especially Sonos users, who want to use SSDP forwarding, will benefit from having the choice whether to DROP or ACCEPT.