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

[Shinichiro Kawasaki]

On Oct 28, 2024 / 12:08, Daniel Wagner wrote:
> 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.

Thanks for these constructive comments :)
I agree that this part can be improved.

> 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().
> 

I tried to move this code block inside _get_nvmet_port_params(), but it will add
one more argument to _get_nvmet_port_params() to pass subsysnqn. So, I think
it's a bit better to keep this part in _nvme_connect_subsys().

> 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?"

Agreed, I will drop --no_port option, and will use '--port none' instead.

> 
> 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.

Agreed. I will try to improve this part. The first if block does parameter
preparation, and the second elif block args set up. These two have different
meanings, and I should not have combined them. Will try to improve in v2.

Also, the original patch from Hannes used "local -n" nameref for
_get_nvmet_port_params(), but I dropped it. I will reintroduce the nameref
for _get_nvmet_port_params() in v2.