Re: Improve error message in rcctl(8) again

2023-07-13 Thread 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

2023-07-13 Thread 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

2023-07-10 Thread 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" ] && \