Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use UCI config_get_bool enabled instead of config_get enabled #24187

Merged
merged 1 commit into from
May 25, 2024

Conversation

stokito
Copy link
Contributor

@stokito stokito commented May 18, 2024

Maintainer: @myszsoda @csonsino @1715173329 @dangowrt @ysc3839 @feckert @blocktrron
Run tested: VirtualBox, but tested only the one

Description:
When reading an enabled option from the UCI config we should use the config_get_bool function instead of just config_get. The reason is because a user may not remember and put not just the 1 but on, yes, true or enabled.

To avoid potential problems and to make all the code looking similar I changed all places from the config_get enabled to config_get_bool enabled. Other boolean options may also need for the change. The only place that I didn't changed in tor-hs package because this fixed in #21642

Another potential problem is that some UCI configs use the option name enable instead of more widelly used enabled.
A user may type enabled and be wondered why it doesn't have any effect.
We have seven places which are using the enable:

net/atftp/files/atftpd.init
net/natmap/files/natmap.init
net/nut/files/nut-cgi.init
net/openvpn/files/openvpn.init
net/sslh/files/sslh.init
net/usbip/files/usbipd.init
utils/collectd/files/collectd.init

We may add an additonal call to config_get_bool enabled and deprecate the old option enable but I believe this is minor thing. We just have to check for this error during a code review to not allow for a new code that uses the enable.

Copy link
Member

@1715173329 1715173329 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@neheb
Copy link
Contributor

neheb commented May 22, 2024

hmm? commit is missing description.

The config_get_bool function parses not just the 1 but on/yes, true/false or enabled/disabled.

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
@stokito
Copy link
Contributor Author

stokito commented May 22, 2024

@neheb sorry, I fixed ad added the comment:

The config_get_bool function parses not just the 1 but on/yes, true/false or enabled/disabled.

@1715173329 1715173329 merged commit 4c4a7bd into openwrt:master May 25, 2024
12 checks passed
@stokito stokito deleted the enabled branch May 25, 2024 19:47
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.

None yet

3 participants