Re: [PATCH nvme-7.3 3/4] nvme-fabrics: add helpers for subsystem and host identity options

[Sagi Grimberg]

On 21/08/2026 9:25, raoxu wrote:
> From: Xu Rao <raoxu@xxxxxxxxxxxxx>
>
> The nqn, hostnqn and hostid options also start with match_strdup(), but
> unlike the direct string replacements handled by nvmf_parse_string_option()
> they perform option-specific validation or conversion before parsing is
> complete.

Does this warrant that they get a special handler? I am not sure I see 
how this helps.

> Move each lifetime into a helper whose name identifies the value being
> parsed:
>
>    nvmf_parse_subsysnqn() handles NVMF_OPT_NQN.  It replaces
>    opts->subsysnqn, checks NVMF_NQN_SIZE and updates discovery_nqn.  Using
>    subsysnqn in the helper name distinguishes the subsystem NQN from the
>    host NQN at the call site.
>
>    nvmf_parse_hostnqn() handles the hostnqn option.  It keeps the existing
>    host-assignment check, validates the temporary NQN, copies it to the
>    hostnqn buffer and frees the temporary allocation before returning.
>
>    nvmf_parse_hostid() handles the hostid option.  It converts the temporary
>    string to uuid_t and frees the duplicated string on both success and
>    failure paths.
>
> These three helpers are grouped because they parse non-sensitive identity
> values that require validation or conversion rather than a simple owned
> string replacement.  Preserve the existing validation and ownership
> ordering while moving the code.
>
> No functional change is intended.
>
> Suggested-by: Christoph Hellwig <hch@xxxxxx>
> Signed-off-by: Xu Rao <raoxu@xxxxxxxxxxxxx>
> ---
>   drivers/nvme/host/fabrics.c | 118 +++++++++++++++++++++---------------
>   1 file changed, 69 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index aab3fd279d0e..120e57964cdd 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -725,13 +725,75 @@ static int nvmf_parse_string_option(substring_t *args, char **dst)
>   	return 0;
>   }
>   
> +static int nvmf_parse_subsysnqn(struct nvmf_ctrl_options *opts, substring_t *args)
> +{
> +	char *nqn;
> +
> +	nqn = match_strdup(args);
> +	if (!nqn)
> +		return -ENOMEM;
> +
> +	kfree(opts->subsysnqn);
> +	opts->subsysnqn = nqn;

The above is nvmf_parse_string_option() no?
> +	if (strlen(opts->subsysnqn) >= NVMF_NQN_SIZE) {
> +		pr_err("%s needs to be < %d bytes\n",
> +		       opts->subsysnqn, NVMF_NQN_SIZE);
> +		return -EINVAL;
> +	}
> +
> +	opts->discovery_nqn = !strcmp(opts->subsysnqn, NVME_DISC_SUBSYS_NAME);

Not sure that this helper existence is really needed, but ok.