diff mbox series

common/rc: Fix check for SCRATCH_DEV_POOL presence in _scratch_dev_pool_get

Message ID 20211101135658.385131-1-nborisov@suse.com (mailing list archive)
State New, archived
Headers show
Series common/rc: Fix check for SCRATCH_DEV_POOL presence in _scratch_dev_pool_get | expand

Commit Message

Nikolay Borisov Nov. 1, 2021, 1:56 p.m. UTC 
Current check is buggy because it can never trigger as even if
SCRATCH_DEV_POOL is not defined config_ndevs will get a value of 0 from
'wc -w', this in turn makes 'typeset -p config_ndevs' always return 0,
triggering the existing check a noop.

Fix this by explicitly checking for the presence of SCHRATC_DEV_POOL

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

Eryu,

Please use this patch as it's a more proper fix

 common/rc | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

--
2.17.1

Comments

Nikolay Borisov Nov. 1, 2021, 2:15 p.m. UTC | #1 
On 1.11.21 г. 16:26, Zorro Lang wrote:
> On Mon, Nov 01, 2021 at 03:56:58PM +0200, Nikolay Borisov wrote:
>> Current check is buggy because it can never trigger as even if
>> SCRATCH_DEV_POOL is not defined config_ndevs will get a value of 0 from
>> 'wc -w', this in turn makes 'typeset -p config_ndevs' always return 0,
>> triggering the existing check a noop.
>>
>> Fix this by explicitly checking for the presence of SCHRATC_DEV_POOL
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>
>> Eryu,
>>
>> Please use this patch as it's a more proper fix
>>
>>  common/rc | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/common/rc b/common/rc
>> index 7f693d3922e8..07b69880eea6 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -917,15 +917,15 @@ _scratch_dev_pool_get()
>>  		_fail "Usage: _scratch_dev_pool_get ndevs"
>>  	fi
>>
>> -	local test_ndevs=$1
>> -	local config_ndevs=`echo $SCRATCH_DEV_POOL| wc -w`
>> -	local -a devs="( $SCRATCH_DEV_POOL )"
>> -
>> -	typeset -p config_ndevs >/dev/null 2>&1
>> +	typeset -p SCRATCH_DEV_POOL >/dev/null 2>&1
>>  	if [ $? -ne 0 ]; then
>>  		_fail "Bug: cant find SCRATCH_DEV_POOL ndevs"
>>  	fi
>>
>> +	local test_ndevs=$1
>> +	local config_ndevs=`echo $SCRATCH_DEV_POOL| wc -w`
>> +	local -a devs="( $SCRATCH_DEV_POOL )"
> 
> Yes, the logic of:
>      "local config_ndevs=`echo $SCRATCH_DEV_POOL| wc -w`
>       typeset -p config_ndevs >/dev/null 2>&1"
> looks weird.

I can only speculate the idea was that `echo $SCRATCH_DEV_POOL| wc -w`
would fail to declare config_ndevs hence typeset -p config_ndevs would
return != 0, however that's not what is happening.

> 
> Checking SCRATCH_DEV_POOL makes sense to me.
> 
> Reviewed-by: Zorro Lang <zlang@redhat.com>
> 
> 
>> +
>>  	if [ $config_ndevs -lt $test_ndevs ]; then
>>  		_notrun "Need at least test requested number of ndevs $test_ndevs"
>>  	fi
>> --
>> 2.17.1
>>
>
Zorro Lang Nov. 1, 2021, 2:26 p.m. UTC | #2 
On Mon, Nov 01, 2021 at 03:56:58PM +0200, Nikolay Borisov wrote:
> Current check is buggy because it can never trigger as even if
> SCRATCH_DEV_POOL is not defined config_ndevs will get a value of 0 from
> 'wc -w', this in turn makes 'typeset -p config_ndevs' always return 0,
> triggering the existing check a noop.
> 
> Fix this by explicitly checking for the presence of SCHRATC_DEV_POOL
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> Eryu,
> 
> Please use this patch as it's a more proper fix
> 
>  common/rc | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 7f693d3922e8..07b69880eea6 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -917,15 +917,15 @@ _scratch_dev_pool_get()
>  		_fail "Usage: _scratch_dev_pool_get ndevs"
>  	fi
> 
> -	local test_ndevs=$1
> -	local config_ndevs=`echo $SCRATCH_DEV_POOL| wc -w`
> -	local -a devs="( $SCRATCH_DEV_POOL )"
> -
> -	typeset -p config_ndevs >/dev/null 2>&1
> +	typeset -p SCRATCH_DEV_POOL >/dev/null 2>&1
>  	if [ $? -ne 0 ]; then
>  		_fail "Bug: cant find SCRATCH_DEV_POOL ndevs"
>  	fi
> 
> +	local test_ndevs=$1
> +	local config_ndevs=`echo $SCRATCH_DEV_POOL| wc -w`
> +	local -a devs="( $SCRATCH_DEV_POOL )"

Yes, the logic of:
     "local config_ndevs=`echo $SCRATCH_DEV_POOL| wc -w`
      typeset -p config_ndevs >/dev/null 2>&1"
looks weird.

Checking SCRATCH_DEV_POOL makes sense to me.

Reviewed-by: Zorro Lang <zlang@redhat.com>


> +
>  	if [ $config_ndevs -lt $test_ndevs ]; then
>  		_notrun "Need at least test requested number of ndevs $test_ndevs"
>  	fi
> --
> 2.17.1
>
Zorro Lang Nov. 1, 2021, 2:42 p.m. UTC | #3 
On Mon, Nov 01, 2021 at 04:15:15PM +0200, Nikolay Borisov wrote:
> 
> 
> On 1.11.21 г. 16:26, Zorro Lang wrote:
> > On Mon, Nov 01, 2021 at 03:56:58PM +0200, Nikolay Borisov wrote:
> >> Current check is buggy because it can never trigger as even if
> >> SCRATCH_DEV_POOL is not defined config_ndevs will get a value of 0 from
> >> 'wc -w', this in turn makes 'typeset -p config_ndevs' always return 0,
> >> triggering the existing check a noop.
> >>
> >> Fix this by explicitly checking for the presence of SCHRATC_DEV_POOL
> >>
> >> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> >> ---
> >>
> >> Eryu,
> >>
> >> Please use this patch as it's a more proper fix
> >>
> >>  common/rc | 10 +++++-----
> >>  1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/common/rc b/common/rc
> >> index 7f693d3922e8..07b69880eea6 100644
> >> --- a/common/rc
> >> +++ b/common/rc
> >> @@ -917,15 +917,15 @@ _scratch_dev_pool_get()
> >>  		_fail "Usage: _scratch_dev_pool_get ndevs"
> >>  	fi
> >>
> >> -	local test_ndevs=$1
> >> -	local config_ndevs=`echo $SCRATCH_DEV_POOL| wc -w`
> >> -	local -a devs="( $SCRATCH_DEV_POOL )"
> >> -
> >> -	typeset -p config_ndevs >/dev/null 2>&1
> >> +	typeset -p SCRATCH_DEV_POOL >/dev/null 2>&1
> >>  	if [ $? -ne 0 ]; then
> >>  		_fail "Bug: cant find SCRATCH_DEV_POOL ndevs"
> >>  	fi
> >>
> >> +	local test_ndevs=$1
> >> +	local config_ndevs=`echo $SCRATCH_DEV_POOL| wc -w`
> >> +	local -a devs="( $SCRATCH_DEV_POOL )"
> > 
> > Yes, the logic of:
> >      "local config_ndevs=`echo $SCRATCH_DEV_POOL| wc -w`
> >       typeset -p config_ndevs >/dev/null 2>&1"
> > looks weird.
> 
> I can only speculate the idea was that `echo $SCRATCH_DEV_POOL| wc -w`
> would fail to declare config_ndevs hence typeset -p config_ndevs would
> return != 0, however that's not what is happening.

Yes, "typeset -p $var" can help to check if a variable is declared, the original
logic "typeset -p config_ndevs" always return 0. I think they might want to
check if [ $config_ndevs -eq 0 ], or check "typeset -p SCRATCH_DEV_POOL" as you
did above.

Thanks,
Zorro

> 
> > 
> > Checking SCRATCH_DEV_POOL makes sense to me.
> > 
> > Reviewed-by: Zorro Lang <zlang@redhat.com>
> > 
> > 
> >> +
> >>  	if [ $config_ndevs -lt $test_ndevs ]; then
> >>  		_notrun "Need at least test requested number of ndevs $test_ndevs"
> >>  	fi
> >> --
> >> 2.17.1
> >>
> > 
>
diff mbox series

Patch

diff --git a/common/rc b/common/rc
index 7f693d3922e8..07b69880eea6 100644
--- a/common/rc
+++ b/common/rc
@@ -917,15 +917,15 @@  _scratch_dev_pool_get()
 		_fail "Usage: _scratch_dev_pool_get ndevs"
 	fi

-	local test_ndevs=$1
-	local config_ndevs=`echo $SCRATCH_DEV_POOL| wc -w`
-	local -a devs="( $SCRATCH_DEV_POOL )"
-
-	typeset -p config_ndevs >/dev/null 2>&1
+	typeset -p SCRATCH_DEV_POOL >/dev/null 2>&1
 	if [ $? -ne 0 ]; then
 		_fail "Bug: cant find SCRATCH_DEV_POOL ndevs"
 	fi

+	local test_ndevs=$1
+	local config_ndevs=`echo $SCRATCH_DEV_POOL| wc -w`
+	local -a devs="( $SCRATCH_DEV_POOL )"
+
 	if [ $config_ndevs -lt $test_ndevs ]; then
 		_notrun "Need at least test requested number of ndevs $test_ndevs"
 	fi