Re: Improve error message in rcctl(8) again

[Anthony Coulter]

> Hi Anthony.
> 
> Thanks for the patch.
> Slightly modified version but it should do the same.
> Does this work for you?
> 
> Index: rcctl.sh
> ===
> RCS file: /cvs/src/usr.sbin/rcctl/rcctl.sh,v
> retrieving revision 1.116
> diff -u -p -r1.116 rcctl.sh
> --- rcctl.sh  24 Apr 2023 14:31:15 -  1.116
> +++ rcctl.sh  13 Jul 2023 09:58:39 -
> @@ -535,13 +535,17 @@ case ${action} in
>   shift 1
>   svcs="$*"
>   [ -z "${svcs}" ] && usage
> - # it's ok to disable a non-existing daemon
> - if [ "${action}" != "disable" ]; then
> - for svc in ${svcs}; do
> + for svc in ${svcs}; do
> + # it's ok to disable a non-existing daemon
> + if [ "${action}" != "disable" ]; then
>   svc_is_avail ${svc} || \
>   rcctl_err "service ${svc} does not 
> exist" 2
>-  done
>-  fi
> + # but still check for bad input
> + else
> + _rc_check_name "${svc}" || \
> + rcctl_err "service ${svc} does not 
> exist" 2
> + fi  <<< TRAILING WHITESPACE >>>
> + done
> [...snip...]

That works, but note the trailing whitespace after the "fi" in the
second-last line of the code snippet above.

Thanks,
Anthony

Re: Improve error message in rcctl(8) again

[Antoine Jacoutot]

On Mon, Jul 10, 2023 at 04:46:28PM -0400, Anthony Coulter wrote:
> Seven years ago I tried to restart a configuration file and it didn't
> work out: https://marc.info/?l=openbsd-tech=147318006722787=2
> 
> This morning I tried to disable a different configuration file and got
> similar results. Maybe my hands get too used to typing ".conf" when I
> am fiddling with config files and restarting services. Or maybe I have
> the mind of a LISP programmer, to whom code and data are the same
> thing. But if I have not stopped passing config files to rcctl after
> seven years I will probably never stop, so we should fix the error
> message again.
> 
> Test cases:
> 
> # rcctl disable foo.bar
> # rcctl set foo.bar status off
> 
> Compare the error messages before and after the patch.
> 
> 
> diff --git usr.sbin/rcctl/rcctl.sh usr.sbin/rcctl/rcctl.sh
> index fb87943ba00..489c0217c45 100644
> --- usr.sbin/rcctl/rcctl.sh
> +++ usr.sbin/rcctl/rcctl.sh
> @@ -541,6 +541,12 @@ case ${action} in
>   svc_is_avail ${svc} || \
>   rcctl_err "service ${svc} does not 
> exist" 2
>   done
> + else
> + # But you still have to catch invalid characters
> + for svc in ${svcs}; do
> + _rc_check_name "${svc}" || \
> + rcctl_err "service ${svc} does not 
> exist" 2
> + done
>   fi
>   ;;
>   get|getdef)
> @@ -572,6 +578,9 @@ case ${action} in
>   if [ "${action} ${var} ${args}" != "set status off" ]; then
>   svc_is_avail ${svc} || \
>   rcctl_err "service ${svc} does not exist" 2
> + else
> + _rc_check_name "${svc}" || \
> + rcctl_err "service ${svc} does not exist" 2
>   fi
>   [[ ${var} != 
> @(class|execdir|flags|logger|rtable|status|timeout|user) ]] && usage
>   svc_is_meta ${svc} && [ "${var}" != "status" ] && \
> 

Hi Anthony.

Thanks for the patch.
Slightly modified version but it should do the same.
Does this work for you?

Index: rcctl.sh
===
RCS file: /cvs/src/usr.sbin/rcctl/rcctl.sh,v
retrieving revision 1.116
diff -u -p -r1.116 rcctl.sh
--- rcctl.sh24 Apr 2023 14:31:15 -  1.116
+++ rcctl.sh13 Jul 2023 09:58:39 -
@@ -535,13 +535,17 @@ case ${action} in
shift 1
svcs="$*"
[ -z "${svcs}" ] && usage
-   # it's ok to disable a non-existing daemon
-   if [ "${action}" != "disable" ]; then
-   for svc in ${svcs}; do
+   for svc in ${svcs}; do
+   # it's ok to disable a non-existing daemon
+   if [ "${action}" != "disable" ]; then
svc_is_avail ${svc} || \
rcctl_err "service ${svc} does not 
exist" 2
-   done
-   fi
+   # but still check for bad input
+   else
+   _rc_check_name "${svc}" || \
+   rcctl_err "service ${svc} does not 
exist" 2
+   fi  
+   done
;;
get|getdef)
svc=$2
@@ -571,6 +575,10 @@ case ${action} in
# it's ok to disable a non-existing daemon
if [ "${action} ${var} ${args}" != "set status off" ]; then
svc_is_avail ${svc} || \
+   rcctl_err "service ${svc} does not exist" 2
+   # but still check for bad input
+   else
+   _rc_check_name "${svc}" || \
rcctl_err "service ${svc} does not exist" 2
fi
[[ ${var} != 
@(class|execdir|flags|logger|rtable|status|timeout|user) ]] && usage

Improve error message in rcctl(8) again

[Anthony Coulter]

Seven years ago I tried to restart a configuration file and it didn't
work out: https://marc.info/?l=openbsd-tech=147318006722787=2

This morning I tried to disable a different configuration file and got
similar results. Maybe my hands get too used to typing ".conf" when I
am fiddling with config files and restarting services. Or maybe I have
the mind of a LISP programmer, to whom code and data are the same
thing. But if I have not stopped passing config files to rcctl after
seven years I will probably never stop, so we should fix the error
message again.

Test cases:

# rcctl disable foo.bar
# rcctl set foo.bar status off

Compare the error messages before and after the patch.


diff --git usr.sbin/rcctl/rcctl.sh usr.sbin/rcctl/rcctl.sh
index fb87943ba00..489c0217c45 100644
--- usr.sbin/rcctl/rcctl.sh
+++ usr.sbin/rcctl/rcctl.sh
@@ -541,6 +541,12 @@ case ${action} in
svc_is_avail ${svc} || \
rcctl_err "service ${svc} does not 
exist" 2
done
+   else
+   # But you still have to catch invalid characters
+   for svc in ${svcs}; do
+   _rc_check_name "${svc}" || \
+   rcctl_err "service ${svc} does not 
exist" 2
+   done
fi
;;
get|getdef)
@@ -572,6 +578,9 @@ case ${action} in
if [ "${action} ${var} ${args}" != "set status off" ]; then
svc_is_avail ${svc} || \
rcctl_err "service ${svc} does not exist" 2
+   else
+   _rc_check_name "${svc}" || \
+   rcctl_err "service ${svc} does not exist" 2
fi
[[ ${var} != 
@(class|execdir|flags|logger|rtable|status|timeout|user) ]] && usage
svc_is_meta ${svc} && [ "${var}" != "status" ] && \

Re: Improve error message in rcctl(8)

[John Boeske]

Antoine,
I've been following this thread and noted that you did add _rc_check_name
to rc.subr and call it in rcctl.  This looks clean and comprehensive. Two
thoughts, however:

1. I had to look at the pattern match in _rc_check_name for a while
   before I understood it; the suggestion below makes it clearer for 
   someone like myself
2. The second suggestion handles the edge case where the argument to rcctl
   matches the name of a subdirectory of rc.d.  Note that ls_rcscripts()
   earlier in the script has the same test

John

 

Index: etc/rc.d/rc.subr
===
RCS file: /cvs/src/etc/rc.d/rc.subr,v
retrieving revision 1.116
diff -u -p -r1.116 rc.subr
--- etc/rc.d/rc.subr7 Sep 2016 13:12:42 -   1.116
+++ etc/rc.d/rc.subr15 Sep 2016 15:58:29 -
@@ -61,7 +61,7 @@ _rc_rm_runfile() {
 }

 _rc_check_name() {
-   [[ $1 == +([_[:alpha:]])+(|[_[:alnum:]]) ]]
+   [[ $1 == +([_[:alpha:]])*([_[:alnum:]]) ]]
 }

 _rc_do() {


Index: usr.sbin/rcctl/rcctl.sh
===
RCS file: /cvs/src/usr.sbin/rcctl/rcctl.sh,v
retrieving revision 1.105
diff -u -p -r1.105 rcctl.sh
--- usr.sbin/rcctl/rcctl.sh 7 Sep 2016 13:13:13 -   1.105
+++ usr.sbin/rcctl/rcctl.sh 15 Sep 2016 15:58:29 -
@@ -141,8 +141,8 @@ svc_is_avail()
local _svc=$1
_rc_check_name "${_svc}" || return

-   [ -x "/etc/rc.d/${_svc}" ] && return
-   svc_is_special ${_svc}
+   [[ -x "/etc/rc.d/${_svc}" && ! -d "/etc/rc.d/${_svc}" ]] \
+   || svc_is_special ${_svc}
 }

 svc_is_base()

 
 

Sent: Tuesday, September 06, 2016 at 2:13 PM
From: "Antoine Jacoutot" <ajacou...@bsdfrog.org>
To: "Anthony Coulter" <b...@anthonycoulter.name>
Cc: tech@openbsd.org
Subject: Re: Improve error message in rcctl(8)
On Tue, Sep 06, 2016 at 04:09:49PM -0400, Anthony Coulter wrote:
> Regarding Jiri's suggestion: Here is a diff that makes
> `rcctl ls all' only list executable files with valid service
> names.
>
> This diff also fixes two problems with my original submission:
> 1. The use of `[' instead of `[[' causes filename expansion to
> take place on the right-hand side of the comparison; you get
> different results depending on which directory you're sitting
> in when you test. I have switched to `[[' to fix that problem.
> 2. There was a stray closing brace that somehow did not cause
> problems in testing.

That's not enough. It cannot start with a digit either.
I'm working on a better diff.

> Index: usr.sbin/rcctl/rcctl.sh
> ===
> RCS file: /open/anoncvs/cvs/src/usr.sbin/rcctl/rcctl.sh,v
> retrieving revision 1.104
> diff -u -p -r1.104 rcctl.sh
> --- usr.sbin/rcctl/rcctl.sh 30 Jul 2016 06:25:21 - 1.104
> +++ usr.sbin/rcctl/rcctl.sh 6 Sep 2016 20:03:33 -
> @@ -53,8 +53,8 @@ ls_rcscripts()
>
> cd /etc/rc.d && set -- *
> for _s; do
> - [[ ${_s} = *.* ]] && continue
> - [ ! -d "${_s}" ] && echo "${_s}"
> + [[ "${_s}" != +([_0-9A-Za-z]) ]] && continue
> + [ -x "${_s}" ] && echo "${_s}"
> done
> }
>
> @@ -182,7 +182,7 @@ svc_is_meta()
> svc_is_special()
> {
> local _svc=$1
> - [ -n "${_svc}" ] || return
> + [[ "${_svc}" = +([_0-9A-Za-z]) ]] || return
>
> local _cached _ret
>
>

--
Antoine
 

Re: Improve error message in rcctl(8)

[lists]

Tue, 06 Sep 2016 22:41:53 +0200 Jeremie Courreges-Anglas

> li...@wrant.com writes:
> 
> > Tue, 6 Sep 2016 19:54:33 + Robert Peichaer   
> >> > Hi tech@,
> >> > 
> >> > Daemon names historically match Antoine's alphanumeric proposal, and I
> >> > think underscore is a bit too much, if it's present use minus instead.
> >> > The logic behind this?  Match this to word termination symbols in ksh.
> >> > 
> >> > Kind regards,
> >> > Anton
> >> 
> >> $ find /usr/ports -name '*_*.rc' -type f | wc -l
> >>   85  
> >
> > Hi Robert,
> >
> > I'll not be arguing about this, from usability end point the underscore
> > is pretty bad especially if you want to go back word at a time.  If you
> > think the underscore is enough, I agree yet I would still prefer minus,
> > where we have variation & need to go back to the differentiation point.  
> 
> On such a trivial subject, you suggest costly changes to existing
> practices, just to please your preferences.  Of course, without even
> publishing a possible implementation.

Hi Jeremie,

Could you, please just fix the word delimiters in ksh(1) no?  Then this
costly change is further not acceptable and I merely mentioned it, only
to selfishly please my preferences, sorry for the noise and time waste.

Kind regards,
Anton

Re: Improve error message in rcctl(8)

[Jeremie Courreges-Anglas]

li...@wrant.com writes:

> Tue, 6 Sep 2016 19:54:33 + Robert Peichaer 
>> > Hi tech@,
>> > 
>> > Daemon names historically match Antoine's alphanumeric proposal, and I
>> > think underscore is a bit too much, if it's present use minus instead.
>> > The logic behind this?  Match this to word termination symbols in ksh.
>> > 
>> > Kind regards,
>> > Anton  
>> 
>> $ find /usr/ports -name '*_*.rc' -type f | wc -l
>>   85
>
> Hi Robert,
>
> I'll not be arguing about this, from usability end point the underscore
> is pretty bad especially if you want to go back word at a time.  If you
> think the underscore is enough, I agree yet I would still prefer minus,
> where we have variation & need to go back to the differentiation point.

On such a trivial subject, you suggest costly changes to existing
practices, just to please your preferences.  Of course, without even
publishing a possible implementation.

I'd find it funny, if only you were not wasting the time of the folks on
this mailing-list.

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: Improve error message in rcctl(8)

[Antoine Jacoutot]

On Tue, Sep 06, 2016 at 04:09:49PM -0400, Anthony Coulter wrote:
> Regarding Jiri's suggestion: Here is a diff that makes
> `rcctl ls all' only list executable files with valid service
> names.
> 
> This diff also fixes two problems with my original submission:
> 1. The use of `[' instead of `[[' causes filename expansion to
>take place on the right-hand side of the comparison; you get
>different results depending on which directory you're sitting
>in when you test. I have switched to `[[' to fix that problem.
> 2. There was a stray closing brace that somehow did not cause
>problems in testing.

That's not enough. It cannot start with a digit either.
I'm working on a better diff.

> Index: usr.sbin/rcctl/rcctl.sh
> ===
> RCS file: /open/anoncvs/cvs/src/usr.sbin/rcctl/rcctl.sh,v
> retrieving revision 1.104
> diff -u -p -r1.104 rcctl.sh
> --- usr.sbin/rcctl/rcctl.sh   30 Jul 2016 06:25:21 -  1.104
> +++ usr.sbin/rcctl/rcctl.sh   6 Sep 2016 20:03:33 -
> @@ -53,8 +53,8 @@ ls_rcscripts()
>  
>   cd /etc/rc.d && set -- *
>   for _s; do
> - [[ ${_s} = *.* ]] && continue
> - [ ! -d "${_s}" ] && echo "${_s}"
> + [[ "${_s}" != +([_0-9A-Za-z]) ]] && continue
> + [ -x "${_s}" ] && echo "${_s}"
>   done
>  }
>  
> @@ -182,7 +182,7 @@ svc_is_meta()
>  svc_is_special()
>  {
>   local _svc=$1
> - [ -n "${_svc}" ] || return
> + [[ "${_svc}" = +([_0-9A-Za-z]) ]] || return
>  
>   local _cached _ret
>  
> 

-- 
Antoine

Re: Improve error message in rcctl(8)

[lists]

Tue, 6 Sep 2016 19:54:33 + Robert Peichaer 
> > Hi tech@,
> > 
> > Daemon names historically match Antoine's alphanumeric proposal, and I
> > think underscore is a bit too much, if it's present use minus instead.
> > The logic behind this?  Match this to word termination symbols in ksh.
> > 
> > Kind regards,
> > Anton  
> 
> $ find /usr/ports -name '*_*.rc' -type f | wc -l
>   85

Hi Robert,

I'll not be arguing about this, from usability end point the underscore
is pretty bad especially if you want to go back word at a time.  If you
think the underscore is enough, I agree yet I would still prefer minus,
where we have variation & need to go back to the differentiation point.

Kind regards,
Anton

Re: Improve error message in rcctl(8)

[Anthony Coulter]

Regarding Jiri's suggestion: Here is a diff that makes
`rcctl ls all' only list executable files with valid service
names.

This diff also fixes two problems with my original submission:
1. The use of `[' instead of `[[' causes filename expansion to
   take place on the right-hand side of the comparison; you get
   different results depending on which directory you're sitting
   in when you test. I have switched to `[[' to fix that problem.
2. There was a stray closing brace that somehow did not cause
   problems in testing.


Index: usr.sbin/rcctl/rcctl.sh
===
RCS file: /open/anoncvs/cvs/src/usr.sbin/rcctl/rcctl.sh,v
retrieving revision 1.104
diff -u -p -r1.104 rcctl.sh
--- usr.sbin/rcctl/rcctl.sh 30 Jul 2016 06:25:21 -  1.104
+++ usr.sbin/rcctl/rcctl.sh 6 Sep 2016 20:03:33 -
@@ -53,8 +53,8 @@ ls_rcscripts()
 
cd /etc/rc.d && set -- *
for _s; do
-   [[ ${_s} = *.* ]] && continue
-   [ ! -d "${_s}" ] && echo "${_s}"
+   [[ "${_s}" != +([_0-9A-Za-z]) ]] && continue
+   [ -x "${_s}" ] && echo "${_s}"
done
 }
 
@@ -182,7 +182,7 @@ svc_is_meta()
 svc_is_special()
 {
local _svc=$1
-   [ -n "${_svc}" ] || return
+   [[ "${_svc}" = +([_0-9A-Za-z]) ]] || return
 
local _cached _ret
 

Re: Improve error message in rcctl(8)

[Robert Peichaer]

> Hi tech@,
> 
> Daemon names historically match Antoine's alphanumeric proposal, and I
> think underscore is a bit too much, if it's present use minus instead.
> The logic behind this?  Match this to word termination symbols in ksh.
> 
> Kind regards,
> Anton

$ find /usr/ports -name '*_*.rc' -type f | wc -l
  85

-- 
-=[rpe]=-

Re: Improve error message in rcctl(8)

[lists]

Tue, 6 Sep 2016 21:04:55 +0200 Antoine Jacoutot 
> On Tue, Sep 06, 2016 at 09:01:08PM +0200, ludovic coues wrote:
> > 2016-09-06 20:53 GMT+02:00 Antoine Jacoutot :  
> > > On Tue, Sep 06, 2016 at 12:29:58PM -0400, Anthony Coulter wrote:  
> > >> Sometimes when I restart a service after changing its configuration file
> > >> I accidentally type:
> > >>
> > >>  # rcctl restart smtpd.conf
> > >>  /usr/sbin/rcctl: ${cached_svc_is_special_smtpd.conf}: bad substitution
> > >>  /usr/sbin/rcctl[556]: set: cached_svc_is_special_smtpd.conf: is not an
> > >>  identifier
> > >>  rcctl: service smtpd.conf does not exist
> > >>
> > >> The message about a bad substitution is not helpful to the user, who
> > >> only needs to know that smtpd.conf is not a service.
> > >>
> > >> The problem is the period in "smtpd.conf". Line 189 of rcctl fails:
> > >>   _cached=$(eval print \${cached_svc_is_special_${_svc}})
> > >>
> > >> Special service names are thus limited to underscores and alphanumerics
> > >> because they're concatenated into shell variable names. So instead of
> > >> checking for [ -n ${_svc} ] at the top of svc_is_special, we ought to
> > >> check that ${_svc} contains only legal characters.
> > >>
> > >> I check only in svc_is_special and not in any of the other places that
> > >> test [ -n ${_svc} ] my only goal is to fix the error message people get
> > >> when they try to start or enable configuration files, and this is the
> > >> only place that needs the error. Adding a similar check to svc_is_avail
> > >> would block an error message when someone creates an executable file
> > >> called /etc/rc.d/foo.bar and then calls "rcctl enable foo.bar", but in
> > >> that case I think the message "${foo.bar_flags}: bad substitution" is
> > >> more helpful---the user is trying to create a service with an illegal
> > >> name and the system is telling him why it will never work.  
> > >
> > > Yes I agree this should be fixed.
> > > What about this?
> > >
> > > Index: rcctl.sh
> > > ===
> > > RCS file: /cvs/src/usr.sbin/rcctl/rcctl.sh,v
> > > retrieving revision 1.104
> > > diff -u -p -r1.104 rcctl.sh
> > > --- rcctl.sh30 Jul 2016 06:25:21 -  1.104
> > > +++ rcctl.sh6 Sep 2016 18:51:18 -
> > > @@ -139,7 +139,7 @@ rcconf_edit_end()
> > >  svc_is_avail()
> > >  {
> > > local _svc=$1
> > > -   [ -n "${_svc}" ] || return
> > > +   [[ "${_svc}" == +([_/+[:alnum:]]) ]] || return
> > >
> > > [ -x "/etc/rc.d/${_svc}" ] && return
> > > svc_is_special ${_svc}
> > >
> > >
> > > --
> > > Antoine
> > >  
> > 
> > If people are using daemon named like fastcgi.exemple.com, this will
> > break there config.  
> 
> The daemon name has no importance. Only the rc.d script name does.
> And in this case, we can install it as /etc/rc.d/fastcgi_exemple_com
> We need to draw a line somewhere to prevent the crazyness...

Hi tech@,

Daemon names historically match Antoine's alphanumeric proposal, and I
think underscore is a bit too much, if it's present use minus instead.
The logic behind this?  Match this to word termination symbols in ksh.

Kind regards,
Anton

Re: Improve error message in rcctl(8)

[Jiri B]

Could a change solve also this annoying situations?
(saved files by editors...)

# rcctl ls all | grep ^tor
tor
tor_2
tor_2~

j.

Re: Improve error message in rcctl(8)

[Anthony Coulter]

Antoine writes:
> What about this?
> + [[ "${_svc}" == +([_/+[:alnum:]]) ]] || return

That doesn't fix the problem. You cannot use plus signs or slashes in a
service name because the options to a service are set in rc.conf.local
with the line

foo_flags=""

where `foo' is replaced by the service name. The ksh man page states
that only letters, numbers, and underscores can be used in variable
names.


Ludovic writes:
> If people are using daemon named like fastcgi.exemple.com, this will
> break there config.

They cannot use that name anyway, for the same reason listed above.
Service names are already restricted to letters, digits, and
underscores. The only thing we can do here is validate user input to
the rcctl command.

Anthony

Re: Improve error message in rcctl(8)

[Antoine Jacoutot]

On Tue, Sep 06, 2016 at 09:01:08PM +0200, ludovic coues wrote:
> 2016-09-06 20:53 GMT+02:00 Antoine Jacoutot :
> > On Tue, Sep 06, 2016 at 12:29:58PM -0400, Anthony Coulter wrote:
> >> Sometimes when I restart a service after changing its configuration file
> >> I accidentally type:
> >>
> >>  # rcctl restart smtpd.conf
> >>  /usr/sbin/rcctl: ${cached_svc_is_special_smtpd.conf}: bad substitution
> >>  /usr/sbin/rcctl[556]: set: cached_svc_is_special_smtpd.conf: is not an
> >>  identifier
> >>  rcctl: service smtpd.conf does not exist
> >>
> >> The message about a bad substitution is not helpful to the user, who
> >> only needs to know that smtpd.conf is not a service.
> >>
> >> The problem is the period in "smtpd.conf". Line 189 of rcctl fails:
> >>   _cached=$(eval print \${cached_svc_is_special_${_svc}})
> >>
> >> Special service names are thus limited to underscores and alphanumerics
> >> because they're concatenated into shell variable names. So instead of
> >> checking for [ -n ${_svc} ] at the top of svc_is_special, we ought to
> >> check that ${_svc} contains only legal characters.
> >>
> >> I check only in svc_is_special and not in any of the other places that
> >> test [ -n ${_svc} ] my only goal is to fix the error message people get
> >> when they try to start or enable configuration files, and this is the
> >> only place that needs the error. Adding a similar check to svc_is_avail
> >> would block an error message when someone creates an executable file
> >> called /etc/rc.d/foo.bar and then calls "rcctl enable foo.bar", but in
> >> that case I think the message "${foo.bar_flags}: bad substitution" is
> >> more helpful---the user is trying to create a service with an illegal
> >> name and the system is telling him why it will never work.
> >
> > Yes I agree this should be fixed.
> > What about this?
> >
> > Index: rcctl.sh
> > ===
> > RCS file: /cvs/src/usr.sbin/rcctl/rcctl.sh,v
> > retrieving revision 1.104
> > diff -u -p -r1.104 rcctl.sh
> > --- rcctl.sh30 Jul 2016 06:25:21 -  1.104
> > +++ rcctl.sh6 Sep 2016 18:51:18 -
> > @@ -139,7 +139,7 @@ rcconf_edit_end()
> >  svc_is_avail()
> >  {
> > local _svc=$1
> > -   [ -n "${_svc}" ] || return
> > +   [[ "${_svc}" == +([_/+[:alnum:]]) ]] || return
> >
> > [ -x "/etc/rc.d/${_svc}" ] && return
> > svc_is_special ${_svc}
> >
> >
> > --
> > Antoine
> >
> 
> If people are using daemon named like fastcgi.exemple.com, this will
> break there config.

The daemon name has no importance. Only the rc.d script name does.
And in this case, we can install it as /etc/rc.d/fastcgi_exemple_com
We need to draw a line somewhere to prevent the crazyness...

-- 
Antoine

Re: Improve error message in rcctl(8)

[ludovic coues]

2016-09-06 20:53 GMT+02:00 Antoine Jacoutot :
> On Tue, Sep 06, 2016 at 12:29:58PM -0400, Anthony Coulter wrote:
>> Sometimes when I restart a service after changing its configuration file
>> I accidentally type:
>>
>>  # rcctl restart smtpd.conf
>>  /usr/sbin/rcctl: ${cached_svc_is_special_smtpd.conf}: bad substitution
>>  /usr/sbin/rcctl[556]: set: cached_svc_is_special_smtpd.conf: is not an
>>  identifier
>>  rcctl: service smtpd.conf does not exist
>>
>> The message about a bad substitution is not helpful to the user, who
>> only needs to know that smtpd.conf is not a service.
>>
>> The problem is the period in "smtpd.conf". Line 189 of rcctl fails:
>>   _cached=$(eval print \${cached_svc_is_special_${_svc}})
>>
>> Special service names are thus limited to underscores and alphanumerics
>> because they're concatenated into shell variable names. So instead of
>> checking for [ -n ${_svc} ] at the top of svc_is_special, we ought to
>> check that ${_svc} contains only legal characters.
>>
>> I check only in svc_is_special and not in any of the other places that
>> test [ -n ${_svc} ] my only goal is to fix the error message people get
>> when they try to start or enable configuration files, and this is the
>> only place that needs the error. Adding a similar check to svc_is_avail
>> would block an error message when someone creates an executable file
>> called /etc/rc.d/foo.bar and then calls "rcctl enable foo.bar", but in
>> that case I think the message "${foo.bar_flags}: bad substitution" is
>> more helpful---the user is trying to create a service with an illegal
>> name and the system is telling him why it will never work.
>
> Yes I agree this should be fixed.
> What about this?
>
> Index: rcctl.sh
> ===
> RCS file: /cvs/src/usr.sbin/rcctl/rcctl.sh,v
> retrieving revision 1.104
> diff -u -p -r1.104 rcctl.sh
> --- rcctl.sh30 Jul 2016 06:25:21 -  1.104
> +++ rcctl.sh6 Sep 2016 18:51:18 -
> @@ -139,7 +139,7 @@ rcconf_edit_end()
>  svc_is_avail()
>  {
> local _svc=$1
> -   [ -n "${_svc}" ] || return
> +   [[ "${_svc}" == +([_/+[:alnum:]]) ]] || return
>
> [ -x "/etc/rc.d/${_svc}" ] && return
> svc_is_special ${_svc}
>
>
> --
> Antoine
>

If people are using daemon named like fastcgi.exemple.com, this will
break there config.

-- 

Cordialement, Coues Ludovic
+336 148 743 42

Re: Improve error message in rcctl(8)

[Antoine Jacoutot]

On Tue, Sep 06, 2016 at 12:29:58PM -0400, Anthony Coulter wrote:
> Sometimes when I restart a service after changing its configuration file
> I accidentally type:
> 
>  # rcctl restart smtpd.conf
>  /usr/sbin/rcctl: ${cached_svc_is_special_smtpd.conf}: bad substitution
>  /usr/sbin/rcctl[556]: set: cached_svc_is_special_smtpd.conf: is not an
>  identifier
>  rcctl: service smtpd.conf does not exist
> 
> The message about a bad substitution is not helpful to the user, who
> only needs to know that smtpd.conf is not a service.
> 
> The problem is the period in "smtpd.conf". Line 189 of rcctl fails:
>   _cached=$(eval print \${cached_svc_is_special_${_svc}})
> 
> Special service names are thus limited to underscores and alphanumerics
> because they're concatenated into shell variable names. So instead of
> checking for [ -n ${_svc} ] at the top of svc_is_special, we ought to
> check that ${_svc} contains only legal characters.
> 
> I check only in svc_is_special and not in any of the other places that
> test [ -n ${_svc} ] my only goal is to fix the error message people get
> when they try to start or enable configuration files, and this is the
> only place that needs the error. Adding a similar check to svc_is_avail
> would block an error message when someone creates an executable file
> called /etc/rc.d/foo.bar and then calls "rcctl enable foo.bar", but in
> that case I think the message "${foo.bar_flags}: bad substitution" is
> more helpful---the user is trying to create a service with an illegal
> name and the system is telling him why it will never work.

Yes I agree this should be fixed.
What about this?

Index: rcctl.sh
===
RCS file: /cvs/src/usr.sbin/rcctl/rcctl.sh,v
retrieving revision 1.104
diff -u -p -r1.104 rcctl.sh
--- rcctl.sh30 Jul 2016 06:25:21 -  1.104
+++ rcctl.sh6 Sep 2016 18:51:18 -
@@ -139,7 +139,7 @@ rcconf_edit_end()
 svc_is_avail()
 {
local _svc=$1
-   [ -n "${_svc}" ] || return
+   [[ "${_svc}" == +([_/+[:alnum:]]) ]] || return
 
[ -x "/etc/rc.d/${_svc}" ] && return
svc_is_special ${_svc}


-- 
Antoine

Improve error message in rcctl(8)

[Anthony Coulter]

Sometimes when I restart a service after changing its configuration file
I accidentally type:

 # rcctl restart smtpd.conf
 /usr/sbin/rcctl: ${cached_svc_is_special_smtpd.conf}: bad substitution
 /usr/sbin/rcctl[556]: set: cached_svc_is_special_smtpd.conf: is not an
 identifier
 rcctl: service smtpd.conf does not exist

The message about a bad substitution is not helpful to the user, who
only needs to know that smtpd.conf is not a service.

The problem is the period in "smtpd.conf". Line 189 of rcctl fails:
  _cached=$(eval print \${cached_svc_is_special_${_svc}})

Special service names are thus limited to underscores and alphanumerics
because they're concatenated into shell variable names. So instead of
checking for [ -n ${_svc} ] at the top of svc_is_special, we ought to
check that ${_svc} contains only legal characters.

I check only in svc_is_special and not in any of the other places that
test [ -n ${_svc} ] my only goal is to fix the error message people get
when they try to start or enable configuration files, and this is the
only place that needs the error. Adding a similar check to svc_is_avail
would block an error message when someone creates an executable file
called /etc/rc.d/foo.bar and then calls "rcctl enable foo.bar", but in
that case I think the message "${foo.bar_flags}: bad substitution" is
more helpful---the user is trying to create a service with an illegal
name and the system is telling him why it will never work.

Regards,
Anthony
 
Index: usr.sbin/rcctl/rcctl.sh
===
RCS file: /open/anoncvs/cvs/src/usr.sbin/rcctl/rcctl.sh,v
retrieving revision 1.104
diff -u -p -u -r1.104 rcctl.sh
--- usr.sbin/rcctl/rcctl.sh 30 Jul 2016 06:25:21 -  1.104
+++ usr.sbin/rcctl/rcctl.sh 6 Sep 2016 15:07:47 -
@@ -182,7 +182,7 @@ svc_is_meta()
 svc_is_special()
 {
local _svc=$1
-   [ -n "${_svc}" ] || return
+   [ "${_svc}" = +([_0-9A-Za-z])} ] || return
 
local _cached _ret