Re: [PATCH v7 01/11] fstests: add _loop_image_create_clone() helper

[Anand Suveer Jain <asj@kernel.org>]

>> +		_mkfs_dev "-s size=4096" ${loop_devs[0]}
> 
> Not sure why you pass two separate cli arguments as a quoted string, but
> my guess is it "doesn't matter" because _try_mkfs_dev uses $* unquoted,
> which separates them again.
> 
> I HATE BASH.
>> +		;;
>> +	btrfs)
>> +		_mkfs_dev ${loop_devs[0]}
> 
> And while I'm whining:    ^^^^^^^^^^^^^^^ actually should be quoted.
> Not that fstests is at all good at getting this right.
> 

Damn! Fixed the two issues above.

>> +		;;
>> +	*)
>> +		_mkfs_dev ${loop_devs[0]}
>> +		;;
>> +	esac
>> +
>> +	# Only execute if the function argument is not empty
>> +	if [ -n "$pre_clone_tune_func" ]; then
>> +		$pre_clone_tune_func ${loop_devs[0]}
>> +	fi
>> +
>> +	sync ${loop_devs[0]}
>> +	cp $img_file $img_file_clone
> 
> What if cp doesn't create a reflink copy?  Can we fill up the $TEST_DIR
> despite having checked it for sufficient free space?  Especially on
> filesystems that don't support sparse holes?
> 

We have _require_fs_space a few lines above (yes, I have doubled the
size requirement). If we reach this point, cp is unlikely to fail, but
if it does, it will be messy.

        _require_fs_space $TEST_DIR $((size * 1024 * 2))

Added a call to _fail to be more direct.

        cp $img_file $img_file_clone || _fail "Failed to copy cloned image"



>> +
>> +	loop_devs="$loop_devs $(_create_loop_device $img_file_clone)"
>> +
>> +	_ret=($loop_devs)
> 
> Hmm.  Should this function return nonzero if any part of the clone
> creation fails?  Or are callers expected to notice that _ret only has
> one element?
> 

If we are here, both images have been created successfully; otherwise,
we would have called _fail.

> --D

Thanks for the reviews Darrick.
-Anand