Re: [PATCH] blktests: replace module removal with patient module removal

[Bart Van Assche <bvanassche@acm.org>]

On 2/8/22 13:21, Luis Chamberlain wrote:
> On Mon, Feb 07, 2022 at 05:10:24PM -0800, Bart Van Assche wrote:
>> On 11/16/21 09:29, Luis Chamberlain wrote:
>>> +	if [[ -f /sys/module/$module/refcnt ]]; then
>>
>> Has this patch been analyzed with 'make check'? I expect checkpatch to
>> complain about missing double quotes for the above statement.
> 
> No, I was not aware this was a thing. I fixed all the stupid complaints
> however I should note at least tests/srp/rc has existing complaints
> before my changes. I won't address those.

Please consider opening a pull request for the 
https://github.com/osandov/blktests repository. That will cause the 
blktests presubmit tests to be run. See also .github/workflows/ci.yml in 
the blktests source code.

> I can't say I agree with the changes to remove checks for $?, but
> whatever. Consinstancy is better than not having one. Perhaps we should
> embrace this on fstests as well.

+1

> No, I'll clarify to this:
> 
> # Tries to wait patiently to remove a module by ensuring first
> # the refcnt is 0 and then trying to remove the module over and over
> # again within the time allowed

Looks better to me :-)

>>>    	for m in ib_srpt iscsi_target_mod target_core_pscsi target_core_iblock \
>>>    			 target_core_file target_core_stgt target_core_user \
>>>    			 target_core_mod
>>> -	do
>>> -		unload_module $m 10 || return $?
>>> -	done
>>> +	_patient_rmmod $m || return $1
>>
>> Please proofread your changes and test your changes before posting these. I
>> think that both "do" and "done" should have been preserved above.
> 
> And the return value shoudl be $? not $1. I couldn't test srp so wanted
> someone who would to help review, but sure, I should have caught this.

Hmm ... the SRP tests can be run inside a virtual machine so it is not 
clear to me what prevents to run these tests?

Thanks,

Bart.