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.subr 7 Sep 2016 13:12:42 -0000 1.116
+++ etc/rc.d/rc.subr 15 Sep 2016 15:58:29 -0000
@@ -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 -0000 1.105
+++ usr.sbin/rcctl/rcctl.sh 15 Sep 2016 15:58:29 -0000
@@ -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 -0000 1.104
> +++ usr.sbin/rcctl/rcctl.sh 6 Sep 2016 20:03:33 -0000
> @@ -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
