olsrd: don't start service when ignored

[dddaniel]

When olsrd was disabled by uci (olsrd.olsrd.ignore=true),
the service got started anyway.
This results in olsrd spamming the syslog when getting started by
procd without a valid configuration:

daemon.err olsrd[8223]: olsrd exit: main: Bad configuration

This commit only starts the olsrd service when not set to ignore.

Signed-off-by: Daniel Danzberger [email protected]

Maintainer: me / @<github-user> (find it by checking history of the package Makefile)
Compile tested: (put here arch, model, OpenWrt version)
Run tested: (put here arch, model, OpenWrt version, tests done)

Description:

[PolynomialDivision]

Why do you need a disable flag, when you can disable the complete service?

[dddaniel]

Why do you need a disable flag, when you can disable the complete service?

So I can disable it via config without having to call /etc/inti.d/olsrd disable, like most services in openwrt

[PolynomialDivision]

If you do that and set it on ignore, what is /etc/init.d/olsrd status returning?

[dddaniel]

If you do that and set it on ignore, what is /etc/init.d/olsrd status returning?

root@openwrt:~# /etc/init.d/olsrd status

active with no instances


[zioproto]

This seems to me a duplication of functionality. You can already disable the service.
Can you bring an example of another service in OpenWrt that does the same ? Can you confirm, showing data, that this is a best practice ?

When olsrd was disabled by uci (olsrd.olsrd.ignore=true),
the service got started anyway.

Reading the above it seems that you are proposing a workaround for a bug of another component. You should fix the root cause of the UCI config not correctly disabling the service.

[dddaniel]

Can you bring an example of another service in OpenWrt that does the same ? Can you confirm, showing data, that this is a best practice ?

dnsmasq, for example:
`dnsmasq_start()
{
local cfg="$1"
local disabled user_dhcpscript
local resolvfile resolvdir localuse=0

config_get_bool disabled "$cfg" disabled 0
[ "$disabled" -gt 0 ] && return 0`

[mwarning]

I have to say to I also have ignore or disable options in my projects. The use case is when I have multiple sections in one configuration file to start multiple instances, but only want to start some of them. /etc/init.d/<sevice> disable would disable all instances.

In face of this use case, this MR seem to have a valid use case. @zioproto what do you think?

[zioproto]

Is the second olsrd word a typo ?

[zioproto]

Ah wait, I understand how, it is because UCI expects olsrd.olsrd.ignore=true

[zioproto]

@mwarning your use case with multiple instances makes sense.

But this PR to olsrd does not involve multiple instances to be turned off individually. It seems to me this is a workaround to fix a UCI problem expecting olsrd.olsrd.ignore=true. Not only the ignore option is added, but also the config stanza is changed from config olsrd to config olsrd olsrd to make another system that expects that syntax happy.

@dddaniel can you clarify if the olsrd.olsrd.ignore=true comes from disabling the service using a UCI cli command or using the LUCI web interface ?

Olsrd config is using the "unnamed section" that is a valid UCI syntax:
https://openwrt.org/docs/guide-user/base-system/uci#uci_dataobject_model
So I dont understand why the move to config olsrd olsrd that could potentially cause problem to other things.

Please provide more context so we can identify the correct place where to fix this issue. I suspect this is just a web interface implementation bug.

thank you

[dddaniel]

Hi @zioproto,

But this PR to olsrd does not involve multiple instances to be turned off individually. It seems to me this is a workaround to fix a UCI problem expecting olsrd.olsrd.ignore=true. Not only the ignore option is added, but also the config stanza is changed from config olsrd to config olsrd olsrd to make another system that expects that syntax happy.

There is no multi instance for the current olsrd. The init script always just starts one instance in the start_service() hook.
The section must be named for reading it's values with config_get. Otherwise I have iterate over the whole config with config_foreach, which makes no sense when there is no multi instance support.

So I dont understand why the move to config olsrd olsrd that could potentially cause problem to other things.

Naming the section doesn't break anything, If there would be code that iterates over all sections with config_foreach, it would still work when the section has a name. It get's an anonymous name derived form it's hash anyway if no name is set.

I could rename it to 'global' which would make it more readable/obvious.

[Hurricos]

The main use I can see is for disabling olsrd6 and maintaining that disablement over a sysupgrade - by default, service disablement isn't preserved over a sysupgrade. I'm testing now whether a symlink of /etc/rc.d/S65olsrd6 -> /dev/null + a correctly configured /etc/sysupgrade.conf would work.

Update: Yes, it does work. Unfortunately it assumes the priority of olsrd remains 65, but it's probably OK that way.