Bug #264

rc.d standardization patch

Added by kevin.kane almost 8 years ago. Updated about 7 years ago.

Status:ClosedStart date:
Priority:LowDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

Description

There is quite a bit of non standardness in the rc.d system and this
aims to get us a little closer to the way NetBSD does things and
towards 'normalcy'.

rename start_vinum -> vinum, enable_quotas -> quotas
remove all instances of _enable
respect configs that use old names with _enable but output a warning
saying _enable is deprecated.
kerberos5, kadmind, kpasswd had the program path in a nonstandard
variable added _program variable which seems to be the standard.
These 3 had _server in the name i removed that as well to make
translation between the old config and new config possible.

here is the diff: http://www.uberstyle.net/~kevin/rcenable.diff

If there are any suggestions for renaming and cleaning up other rcvar
names let me know and ill add them in to this patch.

History

#1 Updated by corecode almost 8 years ago

[..]
> @@ -219,6 +220,26 @@
> #
> checkyesno()
> {
> + # this is added for compatibility purposes will
> + # be removed when _enable is deprecated for real
> + eval _value=\$${1}_enable
> + debug "checkyesno: $1 is set to $_value."
> + case $_value in
> +
> + # "yes", "true", "on", or "1"
> + [Yy][Ee][Ss]|[Tt][Rr][Uu][Ee]|[Oo][Nn]|1)
> + warn "\$${1}_enable is deprecated, use: \$${1} instead."
> + return 0
> + ;;
> +
> + # "no", "false", "off", or "0"
> + [Nn][Oo]|[Ff][Aa][Ll][Ss][Ee]|[Oo][Ff][Ff]|0)
> + warn "\$${1}_enable is deprecated, use: \$${1} instead."
> + return 1
> + ;;
> + esac
> + # end compatibility section for _enable
> +

how about making this
if [ -n "$_value" ]
then
warn "\$${1}_enable is deprecated, use: \$${1} instead."
eval \$${1}=\$${1}_enable
fi

cheers
simon

#2 Updated by kevin.kane almost 8 years ago

I just updated patch with a simplified block along the lines you said.
except:
eval ${1}=$_value

http://www.uberstyle.net/~kevin/rcenable.diff

On 7/28/06, Simon 'corecode' Schubert <> wrote:
> Kevin L. Kane wrote:
> > respect configs that use old names with _enable but output a warning
> > saying _enable is deprecated.
> [..]
> > @@ -219,6 +220,26 @@
> > #
> > checkyesno()
> > {
> > + # this is added for compatibility purposes will
> > + # be removed when _enable is deprecated for real
> > + eval _value=\$${1}_enable
> > + debug "checkyesno: $1 is set to $_value."
> > + case $_value in
> > +
> > + # "yes", "true", "on", or "1"
> > + [Yy][Ee][Ss]|[Tt][Rr][Uu][Ee]|[Oo][Nn]|1)
> > + warn "\$${1}_enable is deprecated, use: \$${1} instead."
> > + return 0
> > + ;;
> > +
> > + # "no", "false", "off", or "0"
> > + [Nn][Oo]|[Ff][Aa][Ll][Ss][Ee]|[Oo][Ff][Ff]|0)
> > + warn "\$${1}_enable is deprecated, use: \$${1} instead."
> > + return 1
> > + ;;
> > + esac
> > + # end compatibility section for _enable
> > +
>
> how about making this
> if [ -n "$_value" ]
> then
> warn "\$${1}_enable is deprecated, use: \$${1} instead."
> eval \$${1}=\$${1}_enable
> fi
>
> cheers
> simon
>
> --
> Serve - BSD +++ RENT this banner advert +++ ASCII Ribbon /"\
> Work - Mac +++ space for low €€€ NOW!1 +++ Campaign \ /
> Party Enjoy Relax | http://dragonflybsd.org Against HTML \
> Dude 2c 2 the max ! http://golden-apple.biz Mail + News / \
>
>
>
>

#3 Updated by corecode almost 8 years ago

Kevin L. Kane wrote:
> I just updated patch with a simplified block along the lines you said.
> except:
> eval ${1}=$_value

even nicer :)

cheers
simon

#4 Updated by joerg almost 8 years ago

On Fri, Jul 28, 2006 at 12:00:08PM -0400, Kevin L. Kane wrote:
> kerberos5, kadmind, kpasswd had the program path in a nonstandard
> variable added _program variable which seems to be the standard.

I'd just nuke the extra indirection for those.

Joerg

#5 Updated by kevin.kane almost 8 years ago

I moved the location of the default command into the rc.d script so we
no longer need the _program variable in defaults/rc.conf. (For
kerberos5, kadmind and kpasswdd)

Updated patch: http://www.uberstyle.net/~kevin/rcenable.diff

On 7/28/06, Joerg Sonnenberger <> wrote:
> On Fri, Jul 28, 2006 at 12:00:08PM -0400, Kevin L. Kane wrote:
> > kerberos5, kadmind, kpasswd had the program path in a nonstandard
> > variable added _program variable which seems to be the standard.
>
> I'd just nuke the extra indirection for those.
>
> Joerg
>

#6 Updated by kevin.kane almost 8 years ago

Is there any other issues people want me to clear up with this patch,
anymore thoughts on rc.d in general?

-Kevin

On 7/28/06, Kevin L. Kane <> wrote:
> I moved the location of the default command into the rc.d script so we
> no longer need the _program variable in defaults/rc.conf. (For
> kerberos5, kadmind and kpasswdd)
>
> Updated patch: http://www.uberstyle.net/~kevin/rcenable.diff
>
> On 7/28/06, Joerg Sonnenberger <> wrote:
> > On Fri, Jul 28, 2006 at 12:00:08PM -0400, Kevin L. Kane wrote:
> > > kerberos5, kadmind, kpasswd had the program path in a nonstandard
> > > variable added _program variable which seems to be the standard.
> >
> > I'd just nuke the extra indirection for those.
> >
> > Joerg
> >
>
>
> --
> Kevin L. Kane
> kevin.kane at gmail.com
>

#7 Updated by TGEN almost 8 years ago

Kevin L. Kane wrote:
> Is there any other issues people want me to clear up with this patch,
> anymore thoughts on rc.d in general?

Include proper /etc/ifconfig.if support (from NetBSD). And if possible,
make sure that an /etc/rc.d/network stop followed by a start restores
IPv6 link-local addresses.

Cheers,
--
Thomas E. Spanjaard

#8 Updated by joerg almost 8 years ago

On Tue, Aug 01, 2006 at 03:18:32PM -0400, Kevin L. Kane wrote:
> Is there any other issues people want me to clear up with this patch,
> anymore thoughts on rc.d in general?

Sync /etc/rc.subr with NetBSD, adds some nice features which can be used
e.g. by smbd :-)

Joerg

#9 Updated by check+j3df9e00rshfwkvl almost 8 years ago

Kevin L. Kane wrote:
> Simon 'corecode' Schubert wrote:
> > Kevin L. Kane wrote:
> > > [...]
> > > + eval _value=\$${1}_enable
> > [...]
> > eval \$${1}=\$${1}_enable
> [...]
> I just updated patch with a simplified block along the lines you said.
> except:
> eval ${1}=$_value

You have to be very careful with "eval" in shell scripts.
It is best to always escape dollar signs on the right, so
they're expanded on the second pass, not on the first, and
enclose the whole expression in escaped double quotes:

eval $1=\"\$_value\"

or:

eval $1='"$_value"'

That's the _only_ way that will always work if the value
of the variable contains white space, semicolons or other
nasty things. If the value comes from a configuration file
and hasn't be checked for errors, then you cannot guarantee
that it's "clean". You probably don't want your rc scripts
abort due to a shell syntax error during boot just because
you accidentally inserted a space in one of the variables.
;-)

Best regards
Oliver

#10 Updated by kevin.kane almost 8 years ago

I updated the patch to include ifconfig.if support and man page. I
took olivers suggestion and changed my eval statement some. I also
pulled in some changes from rc.subr in netbsd, I left out a couple
that didnt seem relevant like _systrace support, some perl daemon
stuff that seemed like we already had it, and also some other minor
changes that we already had.

still availabe at:
http://www.uberstyle.net/~kevin/rcenable.diff

On 02 Aug 2006 12:58:35 GMT, Oliver Fromme
<> wrote:
> Kevin L. Kane wrote:
> > Simon 'corecode' Schubert wrote:
> > > Kevin L. Kane wrote:
> > > > [...]
> > > > + eval _value=\$${1}_enable
> > > [...]
> > > eval \$${1}=\$${1}_enable
> > [...]
> > I just updated patch with a simplified block along the lines you said.
> > except:
> > eval ${1}=$_value
>
> You have to be very careful with "eval" in shell scripts.
> It is best to always escape dollar signs on the right, so
> they're expanded on the second pass, not on the first, and
> enclose the whole expression in escaped double quotes:
>
> eval $1=\"\$_value\"
>
> or:
>
> eval $1='"$_value"'
>
> That's the _only_ way that will always work if the value
> of the variable contains white space, semicolons or other
> nasty things. If the value comes from a configuration file
> and hasn't be checked for errors, then you cannot guarantee
> that it's "clean". You probably don't want your rc scripts
> abort due to a shell syntax error during boot just because
> you accidentally inserted a space in one of the variables.
> ;-)
>
> Best regards
> Oliver
>
> --
> Oliver Fromme, secnetix GmbH & Co. KG, Marktplatz 29, 85567 Grafing
> Dienstleistungen mit Schwerpunkt FreeBSD: http://www.secnetix.de/bsd
>
> Any opinions expressed in this message may be personal to the author
> and may not necessarily reflect the opinions of secnetix in any way.
>

#11 Updated by TGEN almost 8 years ago

Kevin L. Kane wrote:
> I updated the patch to include ifconfig.if support and man page.

I don't think you can just import NetBSD's /etc/rc.d/network; we have
/etc/rc.d/netif and /etc/network.subr to do such things already. I'll
leave it to you to integrate ifconfig.if(5) support into network.subr
and netif ;).

Cheers,
--
Thomas E. Spanjaard

#12 Updated by victor almost 8 years ago

Hi,
I've found a little problem with your patch: if some user made script
does have something like checkyesno foo_enable it will fail. Will work
with foo_enable_enable="YES|NO" and foo_enable="YES|NO", but not
with foo="YES|NO". ie, the new expected behaviour.

I don't think that we should switch to foo="YES|NO", it will require
a lot of documentation changes and will break POLA.

I've created a patch[1] inspired by yours that allows to use
foo_enable="YES|NO" and foo="YES|NO" with pkgsrc and base apps.
It's much smaller, don't require documentation changes and gives
us the choice of working the way we want without imposing anything
to the user.

This is a bit hackish, because the place it should be fixed is on
pkgsrc, but i doubt this is doable as of now.

[1]: http://bsdes.net/~victor/dfbsd/rc.subr.patch
--
La prueba más fehaciente de que existe vida inteligente en otros
planetas, es que no han intentado contactar con nosotros.

#13 Updated by swildner almost 8 years ago

Victor Balada Diaz wrote:
> I've created a patch[1] inspired by yours that allows to use
> foo_enable="YES|NO" and foo="YES|NO" with pkgsrc and base apps.
> It's much smaller, don't require documentation changes and gives
> us the choice of working the way we want without imposing anything
> to the user.
>
> This is a bit hackish, because the place it should be fixed is on
> pkgsrc, but i doubt this is doable as of now.

I like your patch because (even though it's kind of a hack) it gives
everyone the freedom to use the style he prefers with a minimum of effort.

Personally, I prefer "foo_enable" to just "foo". I tend to think we
shouldn't deprecate it just because pkgsrc doesn't properly adapt to the
rc semantics of all its target platforms. :)

I think this should be committed.

Sascha

#14 Updated by joerg almost 8 years ago

On Fri, Sep 29, 2006 at 06:18:28PM +0200, Victor Balada Diaz wrote:
> I've found a little problem with your patch: if some user made script
> does have something like checkyesno foo_enable it will fail. Will work
> with foo_enable_enable="YES|NO" and foo_enable="YES|NO", but not
> with foo="YES|NO". ie, the new expected behaviour.

Hard-coding foo_enable is already a bug and incorrect use of rcNG
anyway.

Joerg

#15 Updated by victor almost 8 years ago

On Fri, Sep 29, 2006 at 06:47:39PM +0200, Joerg Sonnenberger wrote:
> Hard-coding foo_enable is already a bug and incorrect use of rcNG
> anyway.

We hard code it in some places, take /etc/rc.d/ipfw as an example.
So it's not too hard to think that some user is doing it too.

#16 Updated by TGEN almost 8 years ago

Joerg Sonnenberger wrote:
> Hard-coding foo_enable is already a bug and incorrect use of rcNG
> anyway.

nitpick {
rcNG is incorrect naming anyway ;);
};

Cheers,
--
Thomas E. Spanjaard

#17 Updated by joerg almost 8 years ago

On Sat, Sep 30, 2006 at 11:54:19AM +0000, Thomas E. Spanjaard wrote:
> Joerg Sonnenberger wrote:
> >Hard-coding foo_enable is already a bug and incorrect use of rcNG
> >anyway.
>
> nitpick {
> rcNG is incorrect naming anyway ;);
> };

Wrong, rcNG is what FreeBSD called and what we originally imported. The
_enable thingy is a FreeBSD-local change.

Joerg

#18 Updated by luxh about 7 years ago

Also available in: Atom PDF