diff mbox series

[OpenWrt-Devel,PATCHv3] ubox: run init script through shellcheck

Message ID 20200414233728.2519084-1-rosenp@gmail.com
State Accepted
Delegated to: Alexander Couzens
Headers show
Series [OpenWrt-Devel,PATCHv3] ubox: run init script through shellcheck | expand

Commit Message

Rosen Penev April 14, 2020, 11:37 p.m. UTC
Warnings fixed:

SC2004: $/${} is unnecessary on arithmetic variables.
SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
SC2086: Double quote to prevent globbing and word splitting.

Removed most usages of ${} with $. There's no need for it. ${} is
mainly useful with substrings and arrays, which are not used here.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v3: Added quoting fixes.
 v2: Fixed mistake with executing PIDCOUNT.
 package/system/ubox/Makefile       |  2 +-
 package/system/ubox/files/log.init | 32 +++++++++++++++---------------
 2 files changed, 17 insertions(+), 17 deletions(-)

Comments

Alexander 'lynxis' Couzens April 17, 2020, 4:01 a.m. UTC | #1
Hi Rosen,

do you plan to do a v4? Otherwise I would merge it.

Best,
lynxis
Rosen Penev April 17, 2020, 5:25 a.m. UTC | #2
Sent from my iPhone

> On Apr 16, 2020, at 9:01 PM, Alexander 'lynxis' Couzens <lynxis@fe80.eu> wrote:
> 
> Hi Rosen,
> 
> do you plan to do a v4? Otherwise I would merge it.
V4 for what?
> Best,
> lynxis
> -- 
> Alexander Couzens
> 
> mail: lynxis@fe80.eu
> jabber: lynxis@fe80.eu
> gpg: 390D CF78 8BF9 AA50 4F8F  F1E2 C29E 9DA6 A0DF 8604
Adrian Schmutzler April 17, 2020, 8:50 a.m. UTC | #3
Hi,

> -----Original Message-----
> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
> On Behalf Of Rosen Penev
> Sent: Mittwoch, 15. April 2020 01:37
> To: openwrt-devel@lists.openwrt.org
> Subject: [OpenWrt-Devel] [PATCHv3] ubox: run init script through shellcheck
> 
> Warnings fixed:
> 
> SC2004: $/${} is unnecessary on arithmetic variables.
> SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
> SC2086: Double quote to prevent globbing and word splitting.
> 
> Removed most usages of ${} with $. There's no need for it. ${} is mainly
> useful with substrings and arrays, which are not used here.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  v3: Added quoting fixes.
>  v2: Fixed mistake with executing PIDCOUNT.
>  package/system/ubox/Makefile       |  2 +-
>  package/system/ubox/files/log.init | 32 +++++++++++++++---------------
>  2 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/package/system/ubox/Makefile
> b/package/system/ubox/Makefile index cfa0b594e4..3970a4fc9c 100644
> --- a/package/system/ubox/Makefile
> +++ b/package/system/ubox/Makefile
> @@ -1,7 +1,7 @@
>  include $(TOPDIR)/rules.mk
> 
>  PKG_NAME:=ubox
> -PKG_RELEASE:=3
> +PKG_RELEASE:=4
> 
>  PKG_SOURCE_PROTO:=git
>  PKG_SOURCE_URL=$(PROJECT_GIT)/project/ubox.git
> diff --git a/package/system/ubox/files/log.init
> b/package/system/ubox/files/log.init
> index 250f805b44..4873a24472 100644
> --- a/package/system/ubox/files/log.init
> +++ b/package/system/ubox/files/log.init
> @@ -32,19 +32,19 @@ validate_log_daemon()
> 
>  start_service_daemon()
>  {
> -	[ $log_buffer_size -eq 0 -a $log_size -gt 0 ] &&
> log_buffer_size=$log_size
> -	[ $log_buffer_size -eq 0 ] && log_buffer_size=64
> +	[ "$log_buffer_size" -eq 0 ] && [ "$log_size" -gt 0 ] &&

I'm never sure whether a comparison [ "$string" -eq 0 ], i.e. one with quotes and one without using -eq works as expected in all cases.
I typically use [ "$string" = "0" ] instead, but I'm not sure whether that's effectively just the same.

Rest seems fine, despite some similar cases with -eq/-ne below.

Best

Adrian

> log_buffer_size="$log_size"
> +	[ "$log_buffer_size" -eq 0 ] && log_buffer_size=64
>  	procd_open_instance
>  	procd_set_param command "/sbin/logd"
> -	procd_append_param command -S "${log_buffer_size}"
> +	procd_append_param command -S "$log_buffer_size"
>  	procd_set_param respawn 5 1 -1
>  	procd_close_instance
>  }
> 
>  start_service_file()
>  {
> -	PIDCOUNT="$(( ${PIDCOUNT} + 1))"
> -	local pid_file="/var/run/logread.${PIDCOUNT}.pid"
> +	PIDCOUNT=$((PIDCOUNT + 1))
> +	local pid_file="/var/run/logread.$PIDCOUNT.pid"
> 
>  	[ "$2" = 0 ] || {
>  		echo "validation failed"
> @@ -52,34 +52,34 @@ start_service_file()
>  	}
>  	[ -z "${log_file}" ] && return
> 
> -	mkdir -p "$(dirname "${log_file}")"
> +	mkdir -p "$(dirname "$log_file")"
> 
>  	procd_open_instance
>  	procd_set_param command "$PROG" -f -F "$log_file" -p "$pid_file"
> -	[ -n "${log_size}" ] && procd_append_param command -S "$log_size"
> +	[ -n "$log_size" ] && procd_append_param command -S "$log_size"
>  	procd_close_instance
>  }
> 
>  start_service_remote()
>  {
> -	PIDCOUNT="$(( ${PIDCOUNT} + 1))"
> -	local pid_file="/var/run/logread.${PIDCOUNT}.pid"
> +	PIDCOUNT=$((PIDCOUNT + 1))
> +	local pid_file="/var/run/logread.$PIDCOUNT.pid"
> 
>  	[ "$2" = 0 ] || {
>  		echo "validation failed"
>  		return 1
>  	}
> -	[ "${log_remote}" -ne 0 ] || return
> -	[ -z "${log_ip}" ] && return
> -	[ -z "${log_hostname}" ] && log_hostname=$(cat
> /proc/sys/kernel/hostname)
> +	[ "$log_remote" -ne 0 ] || return
> +	[ -z "$log_ip" ] && return
> +	[ -z "$log_hostname" ] && log_hostname=$(cat
> +/proc/sys/kernel/hostname)
> 
>  	procd_open_instance
> -	procd_set_param command "$PROG" -f -h "$log_hostname" -r
> "$log_ip" "${log_port}" -p "$pid_file"
> -	case "${log_proto}" in
> +	procd_set_param command "$PROG" -f -h "$log_hostname" -r
> "$log_ip" "$log_port" -p "$pid_file"
> +	case "$log_proto" in
>  		"udp") procd_append_param command -u;;
> -		"tcp") [ "${log_trailer_null}" -eq 1 ] && procd_append_param
> command -0;;
> +		"tcp") [ "$log_trailer_null" -eq 1 ] && procd_append_param
> command
> +-0;;
>  	esac
> -	[ -z "${log_prefix}" ] || procd_append_param command -P
> "${log_prefix}"
> +	[ -z "$log_prefix" ] || procd_append_param command -P
> "$log_prefix"
>  	procd_close_instance
>  }
> 
> --
> 2.25.2
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Daniel Golle April 17, 2020, 9:24 a.m. UTC | #4
On Fri, Apr 17, 2020 at 10:50:32AM +0200, mail@adrianschmutzler.de wrote:
> Hi,
> 
> > -----Original Message-----
> > From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
> > On Behalf Of Rosen Penev
> > Sent: Mittwoch, 15. April 2020 01:37
> > To: openwrt-devel@lists.openwrt.org
> > Subject: [OpenWrt-Devel] [PATCHv3] ubox: run init script through shellcheck
> > 
> > Warnings fixed:
> > 
> > SC2004: $/${} is unnecessary on arithmetic variables.
> > SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
> > SC2086: Double quote to prevent globbing and word splitting.
> > 
> > Removed most usages of ${} with $. There's no need for it. ${} is mainly
> > useful with substrings and arrays, which are not used here.
> > 
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  v3: Added quoting fixes.
> >  v2: Fixed mistake with executing PIDCOUNT.
> >  package/system/ubox/Makefile       |  2 +-
> >  package/system/ubox/files/log.init | 32 +++++++++++++++---------------
> >  2 files changed, 17 insertions(+), 17 deletions(-)
> > 
> > diff --git a/package/system/ubox/Makefile
> > b/package/system/ubox/Makefile index cfa0b594e4..3970a4fc9c 100644
> > --- a/package/system/ubox/Makefile
> > +++ b/package/system/ubox/Makefile
> > @@ -1,7 +1,7 @@
> >  include $(TOPDIR)/rules.mk
> > 
> >  PKG_NAME:=ubox
> > -PKG_RELEASE:=3
> > +PKG_RELEASE:=4
> > 
> >  PKG_SOURCE_PROTO:=git
> >  PKG_SOURCE_URL=$(PROJECT_GIT)/project/ubox.git
> > diff --git a/package/system/ubox/files/log.init
> > b/package/system/ubox/files/log.init
> > index 250f805b44..4873a24472 100644
> > --- a/package/system/ubox/files/log.init
> > +++ b/package/system/ubox/files/log.init
> > @@ -32,19 +32,19 @@ validate_log_daemon()
> > 
> >  start_service_daemon()
> >  {
> > -	[ $log_buffer_size -eq 0 -a $log_size -gt 0 ] &&
> > log_buffer_size=$log_size
> > -	[ $log_buffer_size -eq 0 ] && log_buffer_size=64
> > +	[ "$log_buffer_size" -eq 0 ] && [ "$log_size" -gt 0 ] &&
> 
> I'm never sure whether a comparison [ "$string" -eq 0 ], i.e. one with quotes and one without using -eq works as expected in all cases.
> I typically use [ "$string" = "0" ] instead, but I'm not sure whether that's effectively just the same.

It's not:
i="0 -eq 0 -o 1"

[ $i -eq 1 ] && echo 1
1

[ "$i" -eq 0 ] && echo 1
bash: [: 0 -eq 0 -o 0: integer expression expected

So as a general rule, it does make sense to use quotes.

> 
> Rest seems fine, despite some similar cases with -eq/-ne below.
> 
> Best
> 
> Adrian
> 
> > log_buffer_size="$log_size"
> > +	[ "$log_buffer_size" -eq 0 ] && log_buffer_size=64
> >  	procd_open_instance
> >  	procd_set_param command "/sbin/logd"
> > -	procd_append_param command -S "${log_buffer_size}"
> > +	procd_append_param command -S "$log_buffer_size"
> >  	procd_set_param respawn 5 1 -1
> >  	procd_close_instance
> >  }
> > 
> >  start_service_file()
> >  {
> > -	PIDCOUNT="$(( ${PIDCOUNT} + 1))"
> > -	local pid_file="/var/run/logread.${PIDCOUNT}.pid"
> > +	PIDCOUNT=$((PIDCOUNT + 1))
> > +	local pid_file="/var/run/logread.$PIDCOUNT.pid"
> > 
> >  	[ "$2" = 0 ] || {
> >  		echo "validation failed"
> > @@ -52,34 +52,34 @@ start_service_file()
> >  	}
> >  	[ -z "${log_file}" ] && return
> > 
> > -	mkdir -p "$(dirname "${log_file}")"
> > +	mkdir -p "$(dirname "$log_file")"
> > 
> >  	procd_open_instance
> >  	procd_set_param command "$PROG" -f -F "$log_file" -p "$pid_file"
> > -	[ -n "${log_size}" ] && procd_append_param command -S "$log_size"
> > +	[ -n "$log_size" ] && procd_append_param command -S "$log_size"
> >  	procd_close_instance
> >  }
> > 
> >  start_service_remote()
> >  {
> > -	PIDCOUNT="$(( ${PIDCOUNT} + 1))"
> > -	local pid_file="/var/run/logread.${PIDCOUNT}.pid"
> > +	PIDCOUNT=$((PIDCOUNT + 1))
> > +	local pid_file="/var/run/logread.$PIDCOUNT.pid"
> > 
> >  	[ "$2" = 0 ] || {
> >  		echo "validation failed"
> >  		return 1
> >  	}
> > -	[ "${log_remote}" -ne 0 ] || return
> > -	[ -z "${log_ip}" ] && return
> > -	[ -z "${log_hostname}" ] && log_hostname=$(cat
> > /proc/sys/kernel/hostname)
> > +	[ "$log_remote" -ne 0 ] || return
> > +	[ -z "$log_ip" ] && return
> > +	[ -z "$log_hostname" ] && log_hostname=$(cat
> > +/proc/sys/kernel/hostname)
> > 
> >  	procd_open_instance
> > -	procd_set_param command "$PROG" -f -h "$log_hostname" -r
> > "$log_ip" "${log_port}" -p "$pid_file"
> > -	case "${log_proto}" in
> > +	procd_set_param command "$PROG" -f -h "$log_hostname" -r
> > "$log_ip" "$log_port" -p "$pid_file"
> > +	case "$log_proto" in
> >  		"udp") procd_append_param command -u;;
> > -		"tcp") [ "${log_trailer_null}" -eq 1 ] && procd_append_param
> > command -0;;
> > +		"tcp") [ "$log_trailer_null" -eq 1 ] && procd_append_param
> > command
> > +-0;;
> >  	esac
> > -	[ -z "${log_prefix}" ] || procd_append_param command -P
> > "${log_prefix}"
> > +	[ -z "$log_prefix" ] || procd_append_param command -P
> > "$log_prefix"
> >  	procd_close_instance
> >  }
> > 
> > --
> > 2.25.2
> > 
> > 
> > _______________________________________________
> > openwrt-devel mailing list
> > openwrt-devel@lists.openwrt.org
> > https://lists.openwrt.org/mailman/listinfo/openwrt-devel



> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Rosen Penev April 18, 2020, 12:56 a.m. UTC | #5
On Fri, Apr 17, 2020 at 1:50 AM <mail@adrianschmutzler.de> wrote:
>
> Hi,
>
> > -----Original Message-----
> > From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
> > On Behalf Of Rosen Penev
> > Sent: Mittwoch, 15. April 2020 01:37
> > To: openwrt-devel@lists.openwrt.org
> > Subject: [OpenWrt-Devel] [PATCHv3] ubox: run init script through shellcheck
> >
> > Warnings fixed:
> >
> > SC2004: $/${} is unnecessary on arithmetic variables.
> > SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
> > SC2086: Double quote to prevent globbing and word splitting.
> >
> > Removed most usages of ${} with $. There's no need for it. ${} is mainly
> > useful with substrings and arrays, which are not used here.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  v3: Added quoting fixes.
> >  v2: Fixed mistake with executing PIDCOUNT.
> >  package/system/ubox/Makefile       |  2 +-
> >  package/system/ubox/files/log.init | 32 +++++++++++++++---------------
> >  2 files changed, 17 insertions(+), 17 deletions(-)
> >
> > diff --git a/package/system/ubox/Makefile
> > b/package/system/ubox/Makefile index cfa0b594e4..3970a4fc9c 100644
> > --- a/package/system/ubox/Makefile
> > +++ b/package/system/ubox/Makefile
> > @@ -1,7 +1,7 @@
> >  include $(TOPDIR)/rules.mk
> >
> >  PKG_NAME:=ubox
> > -PKG_RELEASE:=3
> > +PKG_RELEASE:=4
> >
> >  PKG_SOURCE_PROTO:=git
> >  PKG_SOURCE_URL=$(PROJECT_GIT)/project/ubox.git
> > diff --git a/package/system/ubox/files/log.init
> > b/package/system/ubox/files/log.init
> > index 250f805b44..4873a24472 100644
> > --- a/package/system/ubox/files/log.init
> > +++ b/package/system/ubox/files/log.init
> > @@ -32,19 +32,19 @@ validate_log_daemon()
> >
> >  start_service_daemon()
> >  {
> > -     [ $log_buffer_size -eq 0 -a $log_size -gt 0 ] &&
> > log_buffer_size=$log_size
> > -     [ $log_buffer_size -eq 0 ] && log_buffer_size=64
> > +     [ "$log_buffer_size" -eq 0 ] && [ "$log_size" -gt 0 ] &&
>
> I'm never sure whether a comparison [ "$string" -eq 0 ], i.e. one with quotes and one without using -eq works as expected in all cases.
> I typically use [ "$string" = "0" ] instead, but I'm not sure whether that's effectively just the same.
Sounds bogus. log_buffer_size and log_size are stated to be uintegers above.
>
> Rest seems fine, despite some similar cases with -eq/-ne below.
-eq/-ne vs = is a stylistic difference.
>
> Best
>
> Adrian
>
> > log_buffer_size="$log_size"
> > +     [ "$log_buffer_size" -eq 0 ] && log_buffer_size=64
> >       procd_open_instance
> >       procd_set_param command "/sbin/logd"
> > -     procd_append_param command -S "${log_buffer_size}"
> > +     procd_append_param command -S "$log_buffer_size"
> >       procd_set_param respawn 5 1 -1
> >       procd_close_instance
> >  }
> >
> >  start_service_file()
> >  {
> > -     PIDCOUNT="$(( ${PIDCOUNT} + 1))"
> > -     local pid_file="/var/run/logread.${PIDCOUNT}.pid"
> > +     PIDCOUNT=$((PIDCOUNT + 1))
> > +     local pid_file="/var/run/logread.$PIDCOUNT.pid"
> >
> >       [ "$2" = 0 ] || {
> >               echo "validation failed"
> > @@ -52,34 +52,34 @@ start_service_file()
> >       }
> >       [ -z "${log_file}" ] && return
> >
> > -     mkdir -p "$(dirname "${log_file}")"
> > +     mkdir -p "$(dirname "$log_file")"
> >
> >       procd_open_instance
> >       procd_set_param command "$PROG" -f -F "$log_file" -p "$pid_file"
> > -     [ -n "${log_size}" ] && procd_append_param command -S "$log_size"
> > +     [ -n "$log_size" ] && procd_append_param command -S "$log_size"
> >       procd_close_instance
> >  }
> >
> >  start_service_remote()
> >  {
> > -     PIDCOUNT="$(( ${PIDCOUNT} + 1))"
> > -     local pid_file="/var/run/logread.${PIDCOUNT}.pid"
> > +     PIDCOUNT=$((PIDCOUNT + 1))
> > +     local pid_file="/var/run/logread.$PIDCOUNT.pid"
> >
> >       [ "$2" = 0 ] || {
> >               echo "validation failed"
> >               return 1
> >       }
> > -     [ "${log_remote}" -ne 0 ] || return
> > -     [ -z "${log_ip}" ] && return
> > -     [ -z "${log_hostname}" ] && log_hostname=$(cat
> > /proc/sys/kernel/hostname)
> > +     [ "$log_remote" -ne 0 ] || return
> > +     [ -z "$log_ip" ] && return
> > +     [ -z "$log_hostname" ] && log_hostname=$(cat
> > +/proc/sys/kernel/hostname)
> >
> >       procd_open_instance
> > -     procd_set_param command "$PROG" -f -h "$log_hostname" -r
> > "$log_ip" "${log_port}" -p "$pid_file"
> > -     case "${log_proto}" in
> > +     procd_set_param command "$PROG" -f -h "$log_hostname" -r
> > "$log_ip" "$log_port" -p "$pid_file"
> > +     case "$log_proto" in
> >               "udp") procd_append_param command -u;;
> > -             "tcp") [ "${log_trailer_null}" -eq 1 ] && procd_append_param
> > command -0;;
> > +             "tcp") [ "$log_trailer_null" -eq 1 ] && procd_append_param
> > command
> > +-0;;
> >       esac
> > -     [ -z "${log_prefix}" ] || procd_append_param command -P
> > "${log_prefix}"
> > +     [ -z "$log_prefix" ] || procd_append_param command -P
> > "$log_prefix"
> >       procd_close_instance
> >  }
> >
> > --
> > 2.25.2
> >
> >
> > _______________________________________________
> > openwrt-devel mailing list
> > openwrt-devel@lists.openwrt.org
> > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Kevin 'ldir' Darbyshire-Bryant April 18, 2020, 9:22 a.m. UTC | #6
> On 18 Apr 2020, at 01:56, Rosen Penev <rosenp@gmail.com> wrote:
> 
> On Fri, Apr 17, 2020 at 1:50 AM <mail@adrianschmutzler.de> wrote:
>> 
>>> 
>>> -     [ $log_buffer_size -eq 0 -a $log_size -gt 0 ] &&
>>> log_buffer_size=$log_size
>>> -     [ $log_buffer_size -eq 0 ] && log_buffer_size=64
>>> +     [ "$log_buffer_size" -eq 0 ] && [ "$log_size" -gt 0 ] &&
>> 
>> I'm never sure whether a comparison [ "$string" -eq 0 ], i.e. one with quotes and one without using -eq works as expected in all cases.
>> I typically use [ "$string" = "0" ] instead, but I'm not sure whether that's effectively just the same.
> Sounds bogus. log_buffer_size and log_size are stated to be uintegers above.
>> 
>> Rest seems fine, despite some similar cases with -eq/-ne below.
> -eq/-ne vs = is a stylistic difference.

I disagree.  ‘= != < >’ are string comparisons, -eq/-ne/gt/lt/ge/le are numeric/arithmetic comparisons.

Consider this slightly contrived case where to emphasise the difference between string and numeric comparison I compare to ’00’ which is arithmetically zero, but not just a simple, lone ‘0’ string.

#!/bin/sh

set -x

[ "$foo" -eq 00 ] && echo Z
[ "$foo" = 00 ] && echo Z
[ $foo -eq 00 ] && echo Z
[ $foo = 00 ] && echo Z

foo=“0"
[ "$foo" -eq 00 ] && echo Z
[ "$foo" = 00 ] && echo Z
[ $foo -eq 00 ] && echo Z
[ $foo = 00 ] && echo Z

foo=0
[ "$foo" -eq 00 ] && echo Z
[ "$foo" = 00 ] && echo Z
[ $foo -eq 00 ] && echo Z
[ $foo = 00 ] && echo Z

The unquoted expansions of $foo in the first block will lead to unknown operand errors since $foo expands to nothing.  The quoted $foo in the first block will lead to ’sh: out of range’ because “” is not an integer suitable for the integer -eq comparison.  A solution:

[ "$foo" ] && [ "$foo" -eq 00 ] && echo Z

In later blocks, because $foo is now set it always expands to something so there’s no difference between quoted vs unquoted behaviour (in this example!)  we’re just into the differences between string vs numeric value comparisons, ie. string ‘0’ is not equal to ’00’ but value ‘0’ is equal to ’00'

Maybe that helps.
Rosen Penev April 19, 2020, 2:07 a.m. UTC | #7
On Sat, Apr 18, 2020 at 2:22 AM Kevin 'ldir' Darbyshire-Bryant
<ldir@darbyshire-bryant.me.uk> wrote:
>
>
>
> > On 18 Apr 2020, at 01:56, Rosen Penev <rosenp@gmail.com> wrote:
> >
> > On Fri, Apr 17, 2020 at 1:50 AM <mail@adrianschmutzler.de> wrote:
> >>
> >>>
> >>> -     [ $log_buffer_size -eq 0 -a $log_size -gt 0 ] &&
> >>> log_buffer_size=$log_size
> >>> -     [ $log_buffer_size -eq 0 ] && log_buffer_size=64
> >>> +     [ "$log_buffer_size" -eq 0 ] && [ "$log_size" -gt 0 ] &&
> >>
> >> I'm never sure whether a comparison [ "$string" -eq 0 ], i.e. one with quotes and one without using -eq works as expected in all cases.
> >> I typically use [ "$string" = "0" ] instead, but I'm not sure whether that's effectively just the same.
> > Sounds bogus. log_buffer_size and log_size are stated to be uintegers above.
> >>
> >> Rest seems fine, despite some similar cases with -eq/-ne below.
> > -eq/-ne vs = is a stylistic difference.
>
> I disagree.  ‘= != < >’ are string comparisons, -eq/-ne/gt/lt/ge/le are numeric/arithmetic comparisons.
These are not strings. These are integers.

I did as was asked and shellcheck now complains:

SC2071: > is for string comparisons. Use -gt instead.
>
> Consider this slightly contrived case where to emphasise the difference between string and numeric comparison I compare to ’00’ which is arithmetically zero, but not just a simple, lone ‘0’ string.
>
> #!/bin/sh
>
> set -x
>
> [ "$foo" -eq 00 ] && echo Z
> [ "$foo" = 00 ] && echo Z
> [ $foo -eq 00 ] && echo Z
> [ $foo = 00 ] && echo Z
>
> foo=“0"
> [ "$foo" -eq 00 ] && echo Z
> [ "$foo" = 00 ] && echo Z
> [ $foo -eq 00 ] && echo Z
> [ $foo = 00 ] && echo Z
>
> foo=0
> [ "$foo" -eq 00 ] && echo Z
> [ "$foo" = 00 ] && echo Z
> [ $foo -eq 00 ] && echo Z
> [ $foo = 00 ] && echo Z
>
> The unquoted expansions of $foo in the first block will lead to unknown operand errors since $foo expands to nothing.  The quoted $foo in the first block will lead to ’sh: out of range’ because “” is not an integer suitable for the integer -eq comparison.  A solution:
>
> [ "$foo" ] && [ "$foo" -eq 00 ] && echo Z
>
> In later blocks, because $foo is now set it always expands to something so there’s no difference between quoted vs unquoted behaviour (in this example!)  we’re just into the differences between string vs numeric value comparisons, ie. string ‘0’ is not equal to ’00’ but value ‘0’ is equal to ’00'
>
> Maybe that helps.
>
>
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
diff mbox series

Patch

diff --git a/package/system/ubox/Makefile b/package/system/ubox/Makefile
index cfa0b594e4..3970a4fc9c 100644
--- a/package/system/ubox/Makefile
+++ b/package/system/ubox/Makefile
@@ -1,7 +1,7 @@ 
 include $(TOPDIR)/rules.mk
 
 PKG_NAME:=ubox
-PKG_RELEASE:=3
+PKG_RELEASE:=4
 
 PKG_SOURCE_PROTO:=git
 PKG_SOURCE_URL=$(PROJECT_GIT)/project/ubox.git
diff --git a/package/system/ubox/files/log.init b/package/system/ubox/files/log.init
index 250f805b44..4873a24472 100644
--- a/package/system/ubox/files/log.init
+++ b/package/system/ubox/files/log.init
@@ -32,19 +32,19 @@  validate_log_daemon()
 
 start_service_daemon()
 {
-	[ $log_buffer_size -eq 0 -a $log_size -gt 0 ] && log_buffer_size=$log_size
-	[ $log_buffer_size -eq 0 ] && log_buffer_size=64
+	[ "$log_buffer_size" -eq 0 ] && [ "$log_size" -gt 0 ] && log_buffer_size="$log_size"
+	[ "$log_buffer_size" -eq 0 ] && log_buffer_size=64
 	procd_open_instance
 	procd_set_param command "/sbin/logd"
-	procd_append_param command -S "${log_buffer_size}"
+	procd_append_param command -S "$log_buffer_size"
 	procd_set_param respawn 5 1 -1
 	procd_close_instance
 }
 
 start_service_file()
 {
-	PIDCOUNT="$(( ${PIDCOUNT} + 1))"
-	local pid_file="/var/run/logread.${PIDCOUNT}.pid"
+	PIDCOUNT=$((PIDCOUNT + 1))
+	local pid_file="/var/run/logread.$PIDCOUNT.pid"
 
 	[ "$2" = 0 ] || {
 		echo "validation failed"
@@ -52,34 +52,34 @@  start_service_file()
 	}
 	[ -z "${log_file}" ] && return
 
-	mkdir -p "$(dirname "${log_file}")"
+	mkdir -p "$(dirname "$log_file")"
 
 	procd_open_instance
 	procd_set_param command "$PROG" -f -F "$log_file" -p "$pid_file"
-	[ -n "${log_size}" ] && procd_append_param command -S "$log_size"
+	[ -n "$log_size" ] && procd_append_param command -S "$log_size"
 	procd_close_instance
 }
 
 start_service_remote()
 {
-	PIDCOUNT="$(( ${PIDCOUNT} + 1))"
-	local pid_file="/var/run/logread.${PIDCOUNT}.pid"
+	PIDCOUNT=$((PIDCOUNT + 1))
+	local pid_file="/var/run/logread.$PIDCOUNT.pid"
 
 	[ "$2" = 0 ] || {
 		echo "validation failed"
 		return 1
 	}
-	[ "${log_remote}" -ne 0 ] || return
-	[ -z "${log_ip}" ] && return
-	[ -z "${log_hostname}" ] && log_hostname=$(cat /proc/sys/kernel/hostname)
+	[ "$log_remote" -ne 0 ] || return
+	[ -z "$log_ip" ] && return
+	[ -z "$log_hostname" ] && log_hostname=$(cat /proc/sys/kernel/hostname)
 
 	procd_open_instance
-	procd_set_param command "$PROG" -f -h "$log_hostname" -r "$log_ip" "${log_port}" -p "$pid_file"
-	case "${log_proto}" in
+	procd_set_param command "$PROG" -f -h "$log_hostname" -r "$log_ip" "$log_port" -p "$pid_file"
+	case "$log_proto" in
 		"udp") procd_append_param command -u;;
-		"tcp") [ "${log_trailer_null}" -eq 1 ] && procd_append_param command -0;;
+		"tcp") [ "$log_trailer_null" -eq 1 ] && procd_append_param command -0;;
 	esac
-	[ -z "${log_prefix}" ] || procd_append_param command -P "${log_prefix}"
+	[ -z "$log_prefix" ] || procd_append_param command -P "$log_prefix"
 	procd_close_instance
 }