Skip to content

openvpn: add back hotplug handling#28688

Open
feckert wants to merge 9 commits intoopenwrt:masterfrom
TDT-AG:pr/20260305-openvpn
Open

openvpn: add back hotplug handling#28688
feckert wants to merge 9 commits intoopenwrt:masterfrom
TDT-AG:pr/20260305-openvpn

Conversation

@feckert
Copy link
Member

@feckert feckert commented Mar 5, 2026

📦 Package Details

Maintainer: ?

Description:
Add back hotplug handling @systemcrash

I only have one setup where I could test this. But from my point of view, it should now behave exactly as it did before the proto change. What is still missing here is the implementation for the ucode proto. I haven't done that yet. But I would look into it in a follow-up PR.


🧪 Run Testing Details

  • OpenWrt Version:
  • OpenWrt Target/Subtarget:
  • OpenWrt Device:

✅ Formalities

  • I have reviewed the CONTRIBUTING.md file for detailed contributing guidelines.

--syslog "openvpn_$config" \
--tmp-dir "/var/run" \
$exec_params
eval "proto_run_command '$config' openvpn $exec_params"
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't eval in protos.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I don't do it this way, it doesn't work. I suspect it's because of the append command.
I found in the repo others that do the same thing for the same reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

What doesn't work? Testing showed it worked fine. What is the error? Eval means that proto is in control of a subshell, and not the openvpn process. Not sure of behaviour in that case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have noticed that if I don't do this and the default hotplug handling is active, the service is in a restart loop. As I understand it, a new option is added via append for each whitespace.
The argument does have a two whitspaces one between the option and the argument and one in the argument itself --ipchange '/usr/libexec/openvpn-hotplug ipchange'
The call via eval prevents this.
@systemcrash Please try it yourself.

@systemcrash
Copy link
Contributor

Good work :)

@egc112
Copy link
Contributor

egc112 commented Mar 5, 2026

You guys have been busy 👍

I compiled this morning but with all the changes I will try again tomorrow.
This morning I had the problem with proto / ovpnproto I could hack my way around that but just saw you already solved that 🙂

No luck though with the use of the config file that is not working yet but I am sure you can solve it the coming days

Thank you both for all your work

@systemcrash
Copy link
Contributor

No luck though with the use of the config file that is not working yet but I am sure you can solve it the coming days

Handled in the UI already - might take another day to get built tho.

@feckert feckert force-pushed the pr/20260305-openvpn branch 2 times, most recently from d2f6e39 to 84bb35e Compare March 6, 2026 10:11
Copy link
Contributor

@marcinmajsc marcinmajsc left a comment

Choose a reason for hiding this comment

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

@feckert, the commit title 2e1da0f is 62 characters long, while the allowed limit is 60, which is why your change is not passing the test. Shorten it and the test should pass successfully.

hnyman referenced this pull request Mar 8, 2026
Since proto was migrated to ovpnproto to avoid collision
with netifd proto, this shall be handled separately.

Also avoid using uci commands to migrate the config which
requires knowing property types; use awk instead.

follow-up to 2607b76

Signed-off-by: Paul Donald <newtwen+github@gmail.com>
feckert added 9 commits March 9, 2026 11:54
There is always only one blank line between the sections.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
This was forgotten during renameing of this option.

Fixes: e026ce0 ("openvpn: handle ovpnproto exclusively")

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
The variable 'auth_file' is not used in the following programme sequence.
It therefore only makes sense to add it as a call parameter via 'append'
when calling the the 'proto_run_commmand'.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
The common pattern for global variable is, to write the variable name
capital letters. This improves maintainability in shell scripts.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
There is already the 'append' command, which assembles all parameters that
are called 'proto_run_command'. Let´s use that. To ensure that the
sequence is correct, the parameters must be added at the beginning, so that
user parameters can overwrite them.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
There is already the 'append' command, which assembles all parameters that
are called 'proto_run_command'. Let´s move also the last params to the
top. To ensure that the sequence is correct, the parameters must be added
at the beginning, so that user parameters can overwrite them.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
Properly quote arguments when assembling the command line and eval the
proto_run_command() invocation in order to prevent the shell from
improperly splitting the command arguments on '$IFS'.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
This commit adds hotplug handling back in.

Fixes: 2607b76 ("openvpn: introduce proto handler")

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
Increment PKG_RELEASE by one.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
@feckert feckert force-pushed the pr/20260305-openvpn branch from 84bb35e to 72c5156 Compare March 9, 2026 10:58
@feckert
Copy link
Member Author

feckert commented Mar 9, 2026

@systemcrash
I have implemented everything except eval. I have described the problem with eval. From my point of view, we will have to deal with it for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants