Re: [PATCH blktests v4 1/5] nvme/rc: introduce remote target support

[Aurelien Aptel <aaptel@nvidia.com>]

Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com> writes:
>> @@ -208,6 +213,18 @@ _cleanup_nvmet() {
>>
>>  _setup_nvmet() {
>>       _register_test_cleanup _cleanup_nvmet
>> +
>> +     if [[ -n "${nvme_target_control}" ]]; then
>> +             def_hostnqn="$(${nvme_target_control} config --show-hostnqn)"
>> +             def_hostid="$(${nvme_target_control} config --show-hostid)"
>> +             def_host_traddr="$(${nvme_target_control} config --show-host-traddr)"
>
> I suggest to remove the line above. It caused ShellCheck warning SC2034. I think
> def_host_traddr is not used anywhere.

Ok.

>> @@ -811,6 +836,29 @@ _nvmet_target_setup() {
>>               fi
>>       fi
>>
>> +     if [[ -n "${hostkey}" ]]; then
>> +             ARGS+=(--hostkey "${hostkey}")
>> +     fi
>> +     if [[ -n "${ctrlkey}" ]]; then
>> +             ARGS+=(--ctrkey "${ctrlkey}")
>> +     fi
>
> This part above sets arguments --hostkey and --ctrkey in ARGS to pass to
> _create_nvmet_subsystem(), but I find that _create_nvmet_subsystem() does not
> refer to the arguments. Though I know this part was in v3 also, I suggest drop
> this part.

Good point.

>> +
>> +     if [[ -n "${nvme_target_control}" ]]; then
>> +             eval "${nvme_target_control}" setup \
>> +                     --subsysnqn "${subsysnqn}" \
>> +                     --subsys-uuid "${subsys_uuid:-$def_subsys_uuid}" \
>> +                     --hostnqn "${def_hostnqn}" \
>> +                     "${ARGS[@]}" &> /dev/null
>
> The line above causes the ShellCheck warning SC 2294. Let's replace ${ARGS[@]}
> with ${ARGS[*]}.

I think using quoting [*] will merge the args as one and is not what we
want. I will remove the eval instead. It came from v3, not sure why it is
needed.

>> +             return
>> +     fi
>> +
>> +     truncate -s "${NVME_IMG_SIZE}" "$(_nvme_def_file_path)"
>> +     if [[ "${blkdev_type}" == "device" ]]; then
>> +             blkdev="$(losetup -f --show "$(_nvme_def_file_path)")"
>> +     else
>> +             blkdev="$(_nvme_def_file_path)"
>> +     fi
>
> This truncate and blkdev setup part causes failure of nvme/052:
> [...]
> Also, this part looks duplicated with the other part in _nvmet_target_setup().
> Please see the 'if [[ "${blkdev_type}" != "none" ]]' block.

You're correct, this was a rebasing mistake. I will remove it.

> I guess this is the part you added "to specify the backing block device on the
> target, instead of hardcoding '/dev/vdc'". If so, I think such changes should
> be done under 'if [[ -n "${nvme_target_control}" ]]' condition.

No that part is done in the contrib/ script and the template.

Thanks