Skip to content

Commit

Permalink
Don't run openwrt hotplug script if $DEVICE is unset
Browse files Browse the repository at this point in the history 
Turns out hotplug scripts don't always set $DEVICE, which can leave
interfaces in an unconfigured state.

Thanks to David Ell for reporting the bug. Fixes #69.

Signed-off-by: Toke Høiland-Jørgensen <[email protected]>
  • Loading branch information
@tohojo
tohojo committed Aug 13, 2018
1 parent e8021be commit d0ac824
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions platform/openwrt/sqm-hotplug
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#!/bin/sh

[ -n "$DEVICE" ] || exit 0

restart_sqm() {
/usr/lib/sqm/run.sh stop ${DEVICE}
/usr/lib/sqm/run.sh start ${DEVICE}
Expand Down

3 comments on commit d0ac824

@moeller0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks! Should we add a comment that this should not happen, but when it does it should not have catastrophic side effects? Or just do a

abort_action() {
if [ -n "$DEVICE" ]  ; then
        logger -t SQM -s "Hotplug script called with action ${ACTION} but empty DEVICE, aborting..."
        exit 0
fi
}

[ -n "$DEVICE" ] || abort_action

This assumes that empty DEVICE is not the desired bahaviour...

@tohojo
Copy link
Owner Author

@tohojo tohojo commented on d0ac824 Aug 13, 2018 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@moeller0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it is, but it would have saved David Ell some time during debugging... Also your elegant solution while not doing anything stupid, might end up not doing anything if ifup comes without a DEVICE, but doing that silently... ?
I am quite fine with your solution.

Please sign in to comment.