Shinichiro Kawasaki <shinichiro.kawasaki@xxxxxxx> 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
