[PATCH blktests 11/15] common/nvme, nvme/003: add --no_port option to _nvme_connect_subsys

[Daniel Wagner]

On Mon, Oct 28, 2024 at 07:07:15AM GMT, Shinichiro Kawasaki wrote:
> > I am not so sure about the --no_port argument
> > is a good idea. And if a test wants to use a specific port, we have the
> > port option, no?
> 
> AFAIU, there will be three kinds of _nvme_connect_subsys() calls from port
> parameter point of view:
> 
> 1) Default port(s)
> 
>    No port related option is specified to _nvme_connect_subsys().
>    _nvme_connect_subsys() gets the ports prepared, and refer to them.
> 
> 2) Specific multiple ports
> 
>    --port option is specified to _nvme_connect_subsys().
>    Following series will add tests for them.
> 
> 3) No port
> 
>    nvme/003 creates a nvmet subsystem with default port. The test case
>    also connects to another subsys "nqn.2014-08.org.nvmexpress.discovery"
>    by calling _nvmet_connect_subsys(). In this case, port related parameters
>    should not be specified.
> 
> To clarify that the _nvmet_connect_subsys() call in nvme/003 is the 3) case, I
> suggest to introduce --no_port option. It will explicitly show that
> _nvmet_connect_subsys() does the exceptional parameter preparation.

After looking this and previous patch, I think we should aim to make
this a bit more readable. I would suggest to move the whole port
handling code inside _get_nvmet_port_params(), this is

-       ARGS=(--transport "${nvme_trtype}" --nqn "${subsysnqn}")
-       if [[ "${nvme_trtype}" == "fc" ]] ; then
+       if [[ -z "${port}" ]]; then
                local ports
+
                _get_nvmet_ports "${subsysnqn}" ports
-               if ((${#ports[@]} != 1)); then
-                       echo only one port is supported at this moment...
-                       exit 1
+               port="${ports[0]##*/}"
+               if [[ -z "${port}" ]]; then
+                       echo "WARNING: no port found"
+                       return 1
                fi
-               local port=${ports[0]}
-               ARGS+=(--traddr "$(_fc_traddr "$port")")
-               ARGS+=(--host-traddr "$(_fc_host_traddr "$port")")
-       elif [[ "${nvme_trtype}" != "loop" ]]; then
-               ARGS+=(--traddr "${def_traddr}" --trsvcid "${def_trsvcid}")
        fi

and just pass the --port argument to _get_nvmet_port_params().

Instead adding a new --no_port, just allow '--port none', '--port
discovery' or something similar. I would like to avoid to have a --port
and --no_port argument, because this is very confusing IMO, e.g. I'd ask
myself "If I don't use --port is that not the same as --no_port?"

And also this next block should be splitted somehow:

+       if [[ $port != none ]]; then
+               [[ -d "${cfs_path}" ]] || exit 1
+               trtype=$(cat "${cfs_path}/addr_trtype")
+               traddr=$(cat "${cfs_path}/addr_traddr")
+               args+=(--traddr "${traddr}")
+               if [[ "${trtype}" == "tcp" ]] || [[ "${trtype}" == "rdma" ]]; then
+                       trsvcid=$(cat "${cfs_path}/addr_trsvcid")
+               fi
+       elif [[ "${trtype}" != "loop" ]]; then
+               args+=(--traddr "${traddr}")
+       fi

The first block is guarded by the port argument and the second with
trtype. It's not obvious how these things belong together. Maybe make a
big switch at the beginning of the function on the trtype and add the
arguments accordingly. I think being explicit here and having some
redundancy/repetition is better to maintain.

What do you think?