[PATCH] fabrics: Always pass hostid and hostnqn

[PATCH] fabrics: Always pass hostid and hostnqn

[Israel Rukshin]

After the kernel commit ae8bd606e09b ("nvme-fabrics: prevent overriding
of existing host"), kernel ensures hostid and hostnqn maintain 1:1
mapping. This makes 'nvme discover' and 'nvme connect' commands fail
when providing only hostid or only hostnqn. This issue happens when
the user only enters NQN which doesn't contain UUID, so the generation
of the hostid fails.

There are few more issues that this commit is fixing:
 - When the user provides hostid and NQN, the hostid is overridden
   by generating it from the NQN.
 - hostid is generated from the NQN file instead of the NQN that
   the user enters at the command line.
 - The warning "use generated hostid instead of hostid file" is
   wrong when the user provides hostid via the command line.

The commit fixes those issues by doing the following logic:
 1. If user provided both via command line - pass them as-is
 2. If user doesn't enter them via command line - try to get
    them from files.
 3. If one of them is not provided - generate it from the other.
    Use the new function nvmf_hostid_generate() when NQN doesn't
    have UUID and use nvmf_hostnqn_generate(hostid) to generate
    hostnqn from hostid.
 4. If user provided none - generate them both. Before this commit,
    nvme cli didn't do it.

Signed-off-by: Israel Rukshin <israelr@nvidia.com>
Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 fabrics.c | 74 ++++++++++++++++++++++++++++++-------------------------
 nvme.c    |  4 +--
 2 files changed, 43 insertions(+), 35 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index e081d963..ee15b8bc 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -643,20 +643,9 @@ char *nvmf_hostid_from_hostnqn(const char *hostnqn)
 
 void nvmf_check_hostid_and_hostnqn(const char *hostid, const char *hostnqn, unsigned int verbose)
 {
-	char *hostid_from_file, *hostid_from_hostnqn;
+	char *hostid_from_hostnqn;
 
-	if (!hostid)
-		return;
-
-	hostid_from_file = nvmf_hostid_from_file();
-	if (hostid_from_file && strcmp(hostid_from_file, hostid)) {
-		if (verbose)
-			fprintf(stderr,
-				"warning: use generated hostid instead of hostid file\n");
-		free(hostid_from_file);
-	}
-
-	if (!hostnqn)
+	if (!hostnqn || !hostid)
 		return;
 
 	hostid_from_hostnqn = nvmf_hostid_from_hostnqn(hostnqn);
@@ -668,6 +657,28 @@ void nvmf_check_hostid_and_hostnqn(const char *hostid, const char *hostnqn, unsi
 	}
 }
 
+void nvmf_set_hostid_and_hostnqn(char **hostid, char **hostnqn)
+{
+	if (!*hostid)
+		*hostid = nvmf_hostid_from_file();
+	if (!*hostnqn)
+		*hostnqn = nvmf_hostnqn_from_file();
+
+	if (!*hostid) {
+		if (*hostnqn) {
+			*hostid = nvmf_hostid_from_hostnqn(*hostnqn);
+			if (!*hostid)
+				*hostid = nvmf_hostid_generate();
+		} else {
+			*hostid = nvmf_hostid_generate();
+			*hostnqn = nvmf_hostnqn_generate(*hostid);
+		}
+	}
+
+	if (!*hostnqn)
+		*hostnqn = nvmf_hostnqn_generate(*hostid);
+}
+
 int nvmf_discover(const char *desc, int argc, char **argv, bool connect)
 {
 	char *subsysnqn = NVME_DISC_SUBSYS_NAME;
@@ -747,16 +758,13 @@ int nvmf_discover(const char *desc, int argc, char **argv, bool connect)
 
 	hostnqn_arg = hostnqn;
 	hostid_arg = hostid;
-	if (!hostnqn)
-		hostnqn = hnqn = nvmf_hostnqn_from_file();
-	if (!hostnqn) {
-		hostnqn = hnqn = nvmf_hostnqn_generate();
-		hostid = hid = nvmf_hostid_from_hostnqn(hostnqn);
-	}
-	if (!hostid)
-		hostid = hid = nvmf_hostid_from_file();
-	if (!hostid && hostnqn)
-		hostid = hid = nvmf_hostid_from_hostnqn(hostnqn);
+
+	nvmf_set_hostid_and_hostnqn(&hostid, &hostnqn);
+	if (!hostid_arg)
+		hid = hostid;
+	if (!hostnqn_arg)
+		hnqn = hostnqn;
+
 	nvmf_check_hostid_and_hostnqn(hostid, hostnqn, verbose);
 	h = nvme_lookup_host(r, hostnqn, hostid);
 	if (!h) {
@@ -906,6 +914,7 @@ int nvmf_connect(const char *desc, int argc, char **argv)
 	enum nvme_print_flags flags;
 	struct nvme_fabrics_config cfg = { 0 };
 	char *format = "normal";
+	char *hostnqn_arg, *hostid_arg;
 
 
 	NVMF_ARGS(opts, cfg,
@@ -973,16 +982,15 @@ int nvmf_connect(const char *desc, int argc, char **argv)
 	nvme_read_config(r, config_file);
 	nvme_read_volatile_config(r);
 
-	if (!hostnqn)
-		hostnqn = hnqn = nvmf_hostnqn_from_file();
-	if (!hostnqn) {
-		hostnqn = hnqn = nvmf_hostnqn_generate();
-		hostid = hid = nvmf_hostid_from_hostnqn(hostnqn);
-	}
-	if (!hostid)
-		hostid = hid = nvmf_hostid_from_file();
-	if (!hostid && hostnqn)
-		hostid = hid = nvmf_hostid_from_hostnqn(hostnqn);
+	hostnqn_arg = hostnqn;
+	hostid_arg = hostid;
+
+	nvmf_set_hostid_and_hostnqn(&hostid, &hostnqn);
+	if (!hostid_arg)
+		hid = hostid;
+	if (!hostnqn_arg)
+		hnqn = hostnqn;
+
 	nvmf_check_hostid_and_hostnqn(hostid, hostnqn, verbose);
 	h = nvme_lookup_host(r, hostnqn, hostid);
 	if (!h) {
diff --git a/nvme.c b/nvme.c
index 096d43c9..9f4a179d 100644
--- a/nvme.c
+++ b/nvme.c
@@ -8899,7 +8899,7 @@ static int gen_hostnqn_cmd(int argc, char **argv, struct command *command, struc
 {
 	char *hostnqn;
 
-	hostnqn = nvmf_hostnqn_generate();
+	hostnqn = nvmf_hostnqn_generate(NULL);
 	if (!hostnqn) {
 		nvme_show_error("\"%s\" not supported. Install lib uuid and rebuild.",
 				command->name);
@@ -8916,7 +8916,7 @@ static int show_hostnqn_cmd(int argc, char **argv, struct command *command, stru
 
 	hostnqn = nvmf_hostnqn_from_file();
 	if (!hostnqn)
-		hostnqn =  nvmf_hostnqn_generate();
+		hostnqn =  nvmf_hostnqn_generate(NULL);
 
 	if (!hostnqn) {
 		nvme_show_error("hostnqn is not available -- use nvme gen-hostnqn");
-- 
2.18.2

[PATCH] libnvme: Introduce nvmf_hostid_generate function

[Israel Rukshin]

The function generates a machine specific host identifier. This is
useful when the host ID can't be derived from an NQN that doesn't
contain a UUID.
Also, add an host identifier parameter to nvmf_hostnqn_generate(),
to explicitly set UUID when it is not NULL.

Signed-off-by: Israel Rukshin <israelr@nvidia.com>
Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 src/libnvme.map    |  1 +
 src/nvme/fabrics.c | 24 +++++++++++++++++-------
 src/nvme/fabrics.h | 19 ++++++++++++++++---
 src/nvme/tree.c    |  2 +-
 4 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/src/libnvme.map b/src/libnvme.map
index 8710c41f..787cd6c0 100644
--- a/src/libnvme.map
+++ b/src/libnvme.map
@@ -10,6 +10,7 @@ LIBNVME_1.9 {
 		nvme_submit_passthru64;
 		nvme_update_key;
 		nvme_ctrl_get_cntlid;
+		nvmf_hostid_generate;
 };
 
 LIBNVME_1_8 {
diff --git a/src/nvme/fabrics.c b/src/nvme/fabrics.c
index 6738e9dc..684b2daf 100644
--- a/src/nvme/fabrics.c
+++ b/src/nvme/fabrics.c
@@ -1342,27 +1342,37 @@ static int uuid_from_dmi(char *system_uuid)
 	return ret;
 }
 
-char *nvmf_hostnqn_generate()
+char *nvmf_hostid_generate()
 {
-	char *hostnqn;
 	int ret;
 	char uuid_str[NVME_UUID_LEN_STRING];
 	unsigned char uuid[NVME_UUID_LEN];
 
 	ret = uuid_from_dmi(uuid_str);
-	if (ret < 0) {
+	if (ret < 0)
 		ret = uuid_from_device_tree(uuid_str);
-	}
 	if (ret < 0) {
 		if (nvme_uuid_random(uuid) < 0)
 			memset(uuid, 0, NVME_UUID_LEN);
 		nvme_uuid_to_string(uuid, uuid_str);
 	}
 
-	if (asprintf(&hostnqn, "nqn.2014-08.org.nvmexpress:uuid:%s", uuid_str) < 0)
-		return NULL;
+	return strdup(uuid_str);
+}
+
+char *nvmf_hostnqn_generate(char *hostid)
+{
+	char *hid = NULL;
+	char *hostnqn;
+	int ret;
+
+	if (!hostid)
+		hostid = hid = nvmf_hostid_generate();
+
+	ret = asprintf(&hostnqn, "nqn.2014-08.org.nvmexpress:uuid:%s", hostid);
+	free(hid);
 
-	return hostnqn;
+	return (ret < 0) ? NULL : hostnqn;
 }
 
 static char *nvmf_read_file(const char *f, int len)
diff --git a/src/nvme/fabrics.h b/src/nvme/fabrics.h
index 4ebeb35e..bedef9a4 100644
--- a/src/nvme/fabrics.h
+++ b/src/nvme/fabrics.h
@@ -252,10 +252,23 @@ struct nvmf_discovery_log *nvmf_get_discovery_wargs(struct nvme_get_discovery_ar
 
 /**
  * nvmf_hostnqn_generate() - Generate a machine specific host nqn
- * Returns: An nvm namespace qualified name string based on the machine
- * identifier, or NULL if not successful.
+ * @hostid:		Host identifier
+ *
+ * If @hostid is NULL, the function generates it based on the machine
+ * identifier.
+ *
+ * Return: On success, an NVMe Qualified Name for host identification. This
+ * name is based on the given host identifier. On failure, NULL.
+ */
+char *nvmf_hostnqn_generate(char *hostid);
+
+/**
+ * nvmf_hostid_generate() - Generate a machine specific host identifier
+ *
+ * Return: On success, an identifier string based on the machine identifier to
+ * be used as NVMe Host Identifier, or NULL on failure.
  */
-char *nvmf_hostnqn_generate();
+char *nvmf_hostid_generate();
 
 /**
  * nvmf_hostnqn_from_file() - Reads the host nvm qualified name from the config
diff --git a/src/nvme/tree.c b/src/nvme/tree.c
index 2218d5a8..b2b83bbb 100644
--- a/src/nvme/tree.c
+++ b/src/nvme/tree.c
@@ -125,7 +125,7 @@ nvme_host_t nvme_default_host(nvme_root_t r)
 
 	hostnqn = nvmf_hostnqn_from_file();
 	if (!hostnqn)
-		hostnqn = nvmf_hostnqn_generate();
+		hostnqn = nvmf_hostnqn_generate(NULL);
 	hostid = nvmf_hostid_from_file();
 
 	h = nvme_lookup_host(r, hostnqn, hostid);
-- 
2.18.2

Re: [PATCH] fabrics: Always pass hostid and hostnqn

[Daniel Wagner]

On Tue, Apr 16, 2024 at 06:32:55AM +0000, Israel Rukshin wrote:
> +void nvmf_set_hostid_and_hostnqn(char **hostid, char **hostnqn)
> +{
> +	if (!*hostid)
> +		*hostid = nvmf_hostid_from_file();
> +	if (!*hostnqn)
> +		*hostnqn = nvmf_hostnqn_from_file();

Please use a local pointer variable and do the final assign the
result argument pointer variable at the end of the function. This avoids
the dereferencing of hostid and hostnqn all the time.

Rest looks good.

Re: [PATCH] libnvme: Introduce nvmf_hostid_generate function

[Daniel Wagner]

On Tue, Apr 16, 2024 at 06:32:56AM +0000, Israel Rukshin wrote:
>  /**
>   * nvmf_hostnqn_generate() - Generate a machine specific host nqn
> - * Returns: An nvm namespace qualified name string based on the machine
> - * identifier, or NULL if not successful.
> + * @hostid:		Host identifier
> + *
> + * If @hostid is NULL, the function generates it based on the machine
> + * identifier.
> + *
> + * Return: On success, an NVMe Qualified Name for host identification. This
> + * name is based on the given host identifier. On failure, NULL.
> + */
> +char *nvmf_hostnqn_generate(char *hostid);

This breaks the API. The only way around this is to introduce a complete
new function as far I know.