Changeset View
Standalone View
libexec/rc/rc.d/routing
Show First 20 Lines • Show All 286 Lines • ▼ Show 20 Lines | |||||
ropts_init() | ropts_init() | ||||
{ | { | ||||
if [ -z "${_ropts_initdone}" ]; then | if [ -z "${_ropts_initdone}" ]; then | ||||
echo -n "Additional $1 routing options:" | echo -n "Additional $1 routing options:" | ||||
_ropts_initdone=yes | _ropts_initdone=yes | ||||
fi | fi | ||||
} | } | ||||
options_inet() | options_inet() | ||||
hrs: This function should simply return true or false, not expect another function use the… | |||||
{ | { | ||||
_ropts_initdone= | _ropts_initdone= | ||||
if checkyesno icmp_bmcastecho; then | if checkyesno icmp_bmcastecho; then | ||||
ropts_init inet | ropts_init inet | ||||
echo -n ' broadcast ping responses=YES' | echo -n ' broadcast ping responses=YES' | ||||
${SYSCTL} net.inet.icmp.bmcastecho=1 > /dev/null | ${SYSCTL} net.inet.icmp.bmcastecho=1 > /dev/null | ||||
else | else | ||||
${SYSCTL} net.inet.icmp.bmcastecho=0 > /dev/null | ${SYSCTL} net.inet.icmp.bmcastecho=0 > /dev/null | ||||
fi | fi | ||||
if checkyesno icmp_drop_redirect; then | _icmp_drop_redirect="${icmp_drop_redirect}" | ||||
Done Inline ActionsThis loop does not work well because there are more complex cases such as set_rcvar() and set_rcvar_obsolete(). Invoking "rc.d/foo enabled" is the only way to get value of $foo_enable reliably. I do not like invoking grep(1) multiple times because it slows down the boot time. How about doing grep "^# PROVIDE: .*dynamicrouting.*" for all of rc.d scripts listed by rcorder(8) at once, and then parsing the output and checking the results of "rc.d/foo enabled" individually? hrs: This loop does not work well because there are more complex cases such as set_rcvar() and… | |||||
Done Inline ActionsThank you for the idea of grepping before evaluating. The >eval "$(grep rcvar $file)"< part should handle all the "set_rcvar" issues, you mentioned. This part was taken from /usr/sbin/service -e donner: Thank you for the idea of grepping before evaluating.
The >eval "$(grep rcvar $file)"< part… | |||||
Done Inline ActionsNo, please do not try to parse scripts by using grep. It is quite fragile. service(8) is broken in this regard, too. "set_rcvar foo_enable YES" is another way to define foo_enable while it was not populated well. And name= and rcvar= can appear multiple times in a single script as in rc.d/sendmail and there is a plan to extend it to support multiple profiles which can have multiple rcvar= lines. So a handcrafted routine to extract foo_enable by parsing in rc.d/routnig is unnecessarily complex and harmful for future maintenance. Why just calling the listed scripts with "enabled" argument is not enough? Calling load_rc_config_var() is so heavy and has substantially the same load as calling the script itself with "enabled". hrs: No, please do not try to parse scripts by using grep. It is quite fragile. service(8) is… | |||||
Done Inline ActionsI'm lost. What's your proposal to determine the existence of an enabled service providing a special tag (dynamicrouting)? If there is no workable solution, I'll just modify the man page the clarify the usage of "yes"/"no". donner: I'm lost. What's your proposal to determine the existence of an enabled service providing a… | |||||
Done Inline ActionsI have kept saying the same proposal:
Again, please use calls of the scripts listed by grep with "enabled" instead of parsing them. More specifically, replacing l.306 to l.317 with the following should work for your purpose: for file in $( rcorder ${skip} /etc/rc.d/* ${local_rc} 2>/dev/null | xargs grep -lE '^# PROVIDE:.*\<dynamicrouting\>' ); do (set -- enabled; . $file) && return 0 done hrs: I have kept saying the same proposal:
> Invoking "rc.d/foo enabled" is the only way to get… | |||||
case "${_icmp_drop_redirect}" in | |||||
[Aa][Uu][Tt][Oo] | "") | |||||
_icmp_drop_redirect="${routed_enable}" | |||||
melifaroUnsubmitted Done Inline ActionsThere are a bunch of routing daemons that can be used: frr, quagga, bird, openbgp, ...etc I guess we should depend on the "dynamicrouting" (if the daemons above are consistent about PROVIDE in their rc.d files). melifaro: There are a bunch of routing daemons that can be used: frr, quagga, bird, openbgp, ...etc
I… | |||||
donnerAuthorUnsubmitted Done Inline ActionsThey are third party ports. The setting here is to deal with the base system primary. There might be a better solution, but I'm not in the position to overlook all the consequences and future developments. donner: They are third party ports. The setting here is to deal with the base system primary. There… | |||||
melifaroUnsubmitted Done Inline ActionsNo one uses routed. In the current form this change is useless. melifaro: No one uses routed. In the current form this change is useless. | |||||
donnerAuthorUnsubmitted Done Inline ActionsSo let's drop the idea completely, because there is no common pattern available? donner: So let's drop the idea completely, because there is no common pattern available?
Let's instead… | |||||
donnerAuthorUnsubmitted Done Inline ActionsThe various ports depend on "routing" but that's all they have in common. donner: The various ports depend on "routing" but that's all they have in common. | |||||
melifaroUnsubmitted Done Inline ActionsWell, one can check for "bird frr quagga .." in PROVIDES as a hack. @olivier: what do you think? melifaro: Well, one can check for "bird frr quagga .." in PROVIDES as a hack.
However, I think it… | |||||
olivierUnsubmitted Done Inline ActionsYes I can fix all routing ports to be sure they all PROVIDE the dynamicrouting keyword. olivier: Yes I can fix all routing ports to be sure they all PROVIDE the dynamicrouting keyword. | |||||
donnerAuthorUnsubmitted Done Inline ActionsOkay, so I'll depend on this keyword for "auto", too. Will take some time to understand how it can be done at the correct time. donner: Okay, so I'll depend on this keyword for "auto", too. Will take some time to understand how it… | |||||
;; | |||||
esac | |||||
if checkyesno _icmp_drop_redirect; then | |||||
Done Inline ActionsWhy another variable is required here? Just overriding the value depending on _search_dynamicrouting() should work. hrs: Why another variable is required here? Just overriding the value depending on… | |||||
Done Inline ActionsI do not want to overwrite a configuration variable. To me this is bad practice. We do not know, if somebody will rely on this variable later on in a third party package. Trying to handle all cases explicitly would require to duplicate the checkyesno code from rc.subr (i.e. for the debugging output). That's path I'd like to avoid. But you are right, I updated the patch to avoid environment pollution. donner: I do not want to overwrite a configuration variable. To me this is bad practice. We do not know… | |||||
ropts_init inet | ropts_init inet | ||||
Done Inline ActionsCan we just change this to "NO" ? melifaro: Can we just change this to "NO" ? | |||||
Done Inline ActionsWe can do this, if rc.d/routed would provide "dynamicrouting". donner: We can do this, if rc.d/routed would provide "dynamicrouting".
So you propose to set it to "no"… | |||||
Done Inline ActionsI propose removing anything related to routed. melifaro: I propose removing anything related to routed. | |||||
Done Inline ActionsPlease keep this outside of this patch. donner: Please keep this outside of this patch.
Add another patch to remove routed, there you can… | |||||
Done Inline ActionsLet me try to rephrase it. There is nothing specific about routed - technically it's one of the routing daemons, same as bird, frr or quagga. There should be no specific case handling it in rc.d/routing. If you want, you can add the relevant PROVIDE to the etc/rc.d/routed in the same review. melifaro: Let me try to rephrase it. There is nothing specific about routed - technically it's one of the… | |||||
echo -n ' ignore ICMP redirect=YES' | echo -n ' ignore ICMP redirect=YES' | ||||
${SYSCTL} net.inet.icmp.drop_redirect=1 > /dev/null | ${SYSCTL} net.inet.icmp.drop_redirect=1 > /dev/null | ||||
else | else | ||||
${SYSCTL} net.inet.icmp.drop_redirect=0 > /dev/null | ${SYSCTL} net.inet.icmp.drop_redirect=0 > /dev/null | ||||
fi | fi | ||||
if checkyesno icmp_log_redirect; then | if checkyesno icmp_log_redirect; then | ||||
ropts_init inet | ropts_init inet | ||||
▲ Show 20 Lines • Show All 58 Lines • Show Last 20 Lines |
This function should simply return true or false, not expect another function use the $_dynamicrouting variable as the return value. I think the function name should also be changed because the search result will not be actually used.