Skip to contentSkip to conversation

Useful fabrics patches

PATCH Superseded Sagi Grimberg 4 participants 15 messages Aug 8, 2016 – Aug 9, 2016 NVMe

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Sagi Grimberg

author maintainer
#0 lore raw

v2 cover Useful fabrics patches

Hey Keith,

Changes from v1:
- Fixed review comments from Christoph
- Added a patch to take hostnqn from a default conf file if it exists (and a hostnqn param wasn't given, patches by Jay and Roy)

Sagi Grimberg (4):
fabrics: Allow ipv6 address resolution
fabrics: stringify discover output.
fabrics: Allow discover params to come from a conf file
fabrics: Take the hostnqn parameter from a conf file if not given

 common.h  |   2 +
 fabrics.c | 229 +++++++++++++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 184 insertions(+), 47 deletions(-)
Signature · 1 line
1.9.1
author maintainer
#1 lore raw

PATCH v2 1/4 fabrics: Allow ipv6 address resolution

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
Reviewed-by Christoph Hellwig <hch at lst.de>
---
 fabrics.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
fabrics.c +2 -1
index 8a174d41b82b..961335d45bd2 100644
@@ -438,7 +438,8 @@ static int connect_ctrl(struct nvmf_disc_rsp_page_entry *e)
 		/* we can safely ignore the rest of the entries */
 		break;
 	case NVMF_TRTYPE_RDMA:
-		if (e->adrfam != NVMF_ADDR_FAMILY_IP4) {
+		if (e->adrfam != NVMF_ADDR_FAMILY_IP4 &&
+		    e->adrfam != NVMF_ADDR_FAMILY_IP6) {
 			fprintf(stderr, "skipping unsupported adrfam\n");
 			return -EINVAL;
 		}
Signature · 1 line
1.9.1
#2 lore raw

PATCH v2 1/4 fabrics: Allow ipv6 address resolution

Quoted 15 lines · Sagi Grimberg
Signed-off-by Sagi Grimberg <sagi at grimberg.me>
Reviewed-by Christoph Hellwig <hch at lst.de>

---
?fabrics.c | 3 ++-
?1 file changed, 2 insertions(+), 1 deletion(-)

fabrics.c +0 -0
index 8a174d41b82b..961335d45bd2 100644
@@ -438,7 +438,8 @@ static int connect_ctrl(struct
 nvmf_disc_rsp_page_entry *e)

? /* we can safely ignore the rest of the entries */
? break;
? case NVMF_TRTYPE_RDMA:

fabrics.c static int connect_ctrl(struct nvmf_disc_rsp_page_entry *e)

Quoted 3 lines
fabrics.c +2 -1
-		if (e->adrfam != NVMF_ADDR_FAMILY_IP4) {
+		if (e->adrfam != NVMF_ADDR_FAMILY_IP4 &&
+		????e->adrfam != NVMF_ADDR_FAMILY_IP6) {

If we are going to roll this patch series again, maybe better to just go ahead and do the case()?

I'm indifferent either way.

Quoted 4 lines

? fprintf(stderr, "skipping unsupported
adrfam\n");
? return -EINVAL;
? }

author maintainer
#3 lore raw

PATCH v2 2/4 fabrics: stringify discover output.

In reply to Sagi Grimberg

Just so we have a nice readable output.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
Reviewed-by Christoph Hellwig <hch at lst.de>
---
 common.h  |   2 +
 fabrics.c | 133 +++++++++++++++++++++++++++++++++++++++++++-------------------
 2 files changed, 94 insertions(+), 41 deletions(-)
common.h +2 -0
index 639186d520b1..f0a94de57061 100644
@@ -6,4 +6,6 @@
 #define __round_mask(x, y) ((__typeof__(x))((y)-1))
 #define round_up(x, y) ((((x)-1) | __round_mask(x, y))+1)
 
+#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
+
 #endif
fabrics.c +92 -41

151 lines not on this page · raw message

Signature · 1 line
1.9.1
maintainer
#4 lore raw

PATCH v2 2/4 fabrics: stringify discover output.

fabrics.c static const match_table_t opt_tokens = { · line 78

Quoted 8 lines
fabrics.c +7 -0
+static const char *arg_str(const char * const *strings,
+		size_t array_size, size_t idx)
+{
+		if (idx < array_size && strings[idx])
+			return strings[idx];
+		return "unrecognized";
+}

The indentation of the function body looks b0rked.

author maintainer
#5 lore raw

PATCH v2 3/4 fabrics: Allow discover params to come from a conf file

In reply to Sagi Grimberg

Allow the user to just run "nvme discover" or "nvme connect-all" in case it finds a default /etc/nvme/nvmf_disc conf file.

We allow multiple discovery addresses by iterating over the lines of the file and executing a discover (with or without connect) for each line. We allow newlines and '#' prefixed comments.

The return value is or'ed on all discover attempts.

In order to minimize some parsing code, I just convert the file line into an (argc, argv) pair and feed it to argconfig_parse() which dictates that the file lines are identical to what one would pass nvme discover <params>. I'm open to better ideas.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 fabrics.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 67 insertions(+), 4 deletions(-)
fabrics.c +67 -4
index aa18de9aac6d..54ed8e17b527 100644
@@ -56,6 +56,8 @@ struct config {
 
 #define BUF_SIZE		4096
 #define PATH_NVME_FABRICS	"/dev/nvme-fabrics"
+#define PATH_NVMF_DISC		"/etc/nvme/nvmf_disc"
+#define MAX_DISC_ARGS		10
 
 enum {
 	OPT_INSTANCE,

@@ -575,6 +577,62 @@ static int do_discover(char *argstr, bool connect)
 	return ret;
 }
 
+static int discover_from_conf_file(const char *desc, char *argstr,
+	const struct argconfig_commandline_options *opts, bool connect)
+{
+	FILE *f;
+	char line[255], *ptr, *args, **argv;
+	int argc, err, ret = 0;
+
+	f = fopen(PATH_NVMF_DISC, "r");
+	if (f == NULL) {
+		fprintf(stderr, "No discover params given and no %s conf\n", PATH_NVMF_DISC);
+		return -EINVAL;
+	}
+
+	while (fgets(line, sizeof(line), f) != NULL) {
+		if (line[0] == '#' || line[0] == '\n')
+			continue;
+
+		args = strdup(line);
+		if (!args) {
+			fprintf(stderr, "failed to strdup args\n");
+			return -ENOMEM;
+		}
+
+		argv = calloc(MAX_DISC_ARGS, BUF_SIZE);
+		if (!argv) {
+			fprintf(stderr, "failed to allocate args vector\n");
+			return -ENOMEM;
+		}
+
+		argc = 0;
+		argv[argc++] = "discover";
+		while ((ptr = strsep(&args, " =\n")) != NULL)
+			argv[argc++] = ptr;
+
+		argconfig_parse(argc, argv, desc, opts, &cfg, sizeof(cfg));
+
+		err = build_options(argstr, BUF_SIZE);
+		if (err) {
+			ret = err;
+			continue;
+		}
+
+		err = do_discover(argstr, connect);
+		if (err) {
+			ret = err;
+			continue;
+		}
+
+		free(args);
+		free(argv);
+	}
+
+	fclose(f);
+	return ret;
+}
+
 int discover(const char *desc, int argc, char **argv, bool connect)
 {
 	char argstr[BUF_SIZE];

@@ -597,11 +655,16 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 
 	cfg.nqn = NVME_DISC_SUBSYS_NAME;
 
-	ret = build_options(argstr, BUF_SIZE);
-	if (ret)
-		return ret;
+	if (!cfg.transport && !cfg.traddr) {
+		return discover_from_conf_file(desc, argstr,
+				command_line_options, connect);
+	} else {
+		ret = build_options(argstr, BUF_SIZE);
+		if (ret)
+			return ret;
 
-	return do_discover(argstr, connect);
+		return do_discover(argstr, connect);
+	}
 }
 
 int connect(const char *desc, int argc, char **argv)
Signature · 1 line
1.9.1
maintainer
#6 lore raw

PATCH v2 3/4 fabrics: Allow discover params to come from a conf file

Quoted 2 lines · Sagi Grimberg

Allow the user to just run "nvme discover" or "nvme connect-all" in case it finds a default /etc/nvme/nvmf_disc conf file.

Hmm, can just call this /etc/nvme/discovery.conf or something else that rolls easier off the hand?

fabrics.c static int do_discover(char *argstr, bool connect) · line 581

Quoted 2 lines
fabrics.c +2 -0
+static int discover_from_conf_file(const char *desc, char *argstr,
+	const struct argconfig_commandline_options *opts, bool connect)

Second tab for the argument indent please.

fabrics.c static int do_discover(char *argstr, bool connect) · line 626

Quoted 11 lines
fabrics.c +11 -0
+		err = build_options(argstr, BUF_SIZE);
+		if (err) {
+			ret = err;
+			continue;
+		}
+
+		err = do_discover(argstr, connect);
+		if (err) {
+			ret = err;
+			continue;
+		}

How will diagnostics look like here if the file has incorrect syntax?

fabrics.c int discover(const char *desc, int argc, char **argv, bool connect) · line 660

Quoted 3 lines
fabrics.c +3 -0
+	if (!cfg.transport && !cfg.traddr) {
+		return discover_from_conf_file(desc, argstr,
+				command_line_options, connect);

Maybe we want a separate options that says the option needs to be read from a config file, e.g. --file with an optional argument for the file name?

#7 lore raw

PATCH v2 3/4 fabrics: Allow discover params to come from a conf file

Quoted 7 lines · Christoph Hellwig
Quoted 3 lines · Sagi Grimberg

Allow the user to just run "nvme discover" or "nvme connect-all" in case it finds a default /etc/nvme/nvmf_disc conf file.

Hmm, can just call this /etc/nvme/discovery.conf or something else that rolls easier off the hand?

Yah, similar to my last nitpick comment, I like this a tad better because we don't even have to mention nvmf/nvmeof.

I'd still like to see example documentation in the header patch of what the resulting .conf file contents could look like.

Quoted 9 lines
Quoted 5 lines
(quoted diff) +2 -0
+static int discover_from_conf_file(const char *desc, char *argstr,
+	const struct argconfig_commandline_options *opts, bool

connect)

Second tab for the argument indent please.

Quoted 1 line
Quoted 14 lines

fabrics.c static int do_discover(char *argstr, bool connect) · line 626

Quoted 11 lines
fabrics.c +11 -0
+		err = build_options(argstr, BUF_SIZE);
+		if (err) {
+			ret = err;
+			continue;
+		}
+
+		err = do_discover(argstr, connect);
+		if (err) {
+			ret = err;
+			continue;
+		}

How will diagnostics look like here if the file has incorrect syntax?

Dido. ?Be nice to know what argstr/parameter broke.

Quoted 9 lines

fabrics.c int discover(const char *desc, int argc, char **argv, bool connect) · line 660

Quoted 5 lines
fabrics.c +3 -0
+	if (!cfg.transport && !cfg.traddr) {
+		return discover_from_conf_file(desc, argstr,
+				command_line_options, connect);

Maybe we want a separate options that says the option needs to be read from a config file, e.g. --file with an optional argument for the file name?

Are you thinking that there could be a single system that is acting like multiple hosts, therefore each "host/hostnqn" could use a separate .conf file for connection??

If that is the case, then it may be a little confusing if there was a single /etc/nvme/hostnqn filename that existed.

author maintainer
#8 lore raw

PATCH v2 3/4 fabrics: Allow discover params to come from a conf file

In reply to Christoph Hellwig
Quoted 6 lines · Christoph Hellwig
Quoted 2 lines · Sagi Grimberg

Allow the user to just run "nvme discover" or "nvme connect-all" in case it finds a default /etc/nvme/nvmf_disc conf file.

Hmm, can just call this /etc/nvme/discovery.conf or something else that rolls easier off the hand?

ok

Quoted 5 lines

fabrics.c static int do_discover(char *argstr, bool connect) · line 581

Quoted 3 lines
fabrics.c +2 -0
+static int discover_from_conf_file(const char *desc, char *argstr,
+	const struct argconfig_commandline_options *opts, bool connect)

Second tab for the argument indent please.

done

Quoted 15 lines

fabrics.c static int do_discover(char *argstr, bool connect) · line 626

Quoted 12 lines
fabrics.c +11 -0
+		err = build_options(argstr, BUF_SIZE);
+		if (err) {
+			ret = err;
+			continue;
+		}
+
+		err = do_discover(argstr, connect);
+		if (err) {
+			ret = err;
+			continue;
+		}

How will diagnostics look like here if the file has incorrect syntax?

the same way that discovery from user args, it'll just happen for each incorrect line. Would you prefer that we print the args line we're using?

Quoted 7 lines

fabrics.c int discover(const char *desc, int argc, char **argv, bool connect) · line 660

Quoted 3 lines
fabrics.c +3 -0
+	if (!cfg.transport && !cfg.traddr) {
+		return discover_from_conf_file(desc, argstr,
+				command_line_options, connect);

Maybe we want a separate options that says the option needs to be read from a config file, e.g. --file with an optional argument for the file name?

I kinda like the fact that you can just run discover/connect-all without providing any arguments. We could add the conf file argument if it's located in a non-default path maybe? what do others think?

#9 lore raw

PATCH v2 3/4 fabrics: Allow discover params to come from a conf file

In reply to Sagi Grimberg

I missed some things on this patch.

Quoted 16 lines

?
?#define BUF_SIZE 4096
?#define PATH_NVME_FABRICS "/dev/nvme-fabrics"

fabrics.c +2 -0
+#define PATH_NVMF_DISC		"/etc/nvme/nvmf_disc"
+#define MAX_DISC_ARGS		10

?
?enum {
? OPT_INSTANCE,

(quoted diff) +0 -0
@@ -575,6 +577,62 @@ static int do_discover(char *argstr, bool
 connect)
 ?	return ret;
 ?}

?

(quoted diff) +2 -0
+static int discover_from_conf_file(const char *desc, char *argstr,
+	const struct argconfig_commandline_options *opts, bool

connect)

fabrics.c static int do_discover(char *argstr, bool connect) · line 584

Quoted 3 lines
fabrics.c +3 -0
+{
+	FILE *f;
+	char line[255], *ptr, *args, **argv;

Why is line 255 and not 256? ?Maybe want it a #define?

Quoted 6 lines
fabrics.c +5 -0
+	int argc, err, ret = 0;
+
+	f = fopen(PATH_NVMF_DISC, "r");
+	if (f == NULL) {
+		fprintf(stderr, "No discover params given and no %s

conf\n", PATH_NVMF_DISC);

fabrics.c static int do_discover(char *argstr, bool connect) · line 599

Quoted 10 lines
fabrics.c +10 -0
+		return -EINVAL;
+	}
+
+	while (fgets(line, sizeof(line), f) != NULL) {
+		if (line[0] == '#' || line[0] == '\n')
+			continue;
+
+		args = strdup(line);
+		if (!args) {
+			fprintf(stderr, "failed to strdup args\n");

Be useful to print out the line so you know where you failed.

Quoted 7 lines
fabrics.c +6 -0
+			return -ENOMEM;
+		}
+
+		argv = calloc(MAX_DISC_ARGS, BUF_SIZE);
+		if (!argv) {
+			fprintf(stderr, "failed to allocate args

vector\n");

memory leak from not free'ing args in this case???

Also fprintf() says 'args' not 'argv'. ?And again may be useful to print out the line of where you failed.

Quoted 10 lines
fabrics.c +9 -0
+			return -ENOMEM;
+		}
+
+		argc = 0;
+		argv[argc++] = "discover";
+		while ((ptr = strsep(&args, " =\n")) != NULL)
+			argv[argc++] = ptr;
+
+		argconfig_parse(argc, argv, desc, opts, &cfg,

sizeof(cfg));

fabrics.c static int do_discover(char *argstr, bool connect) · line 618

Quoted 4 lines
fabrics.c +4 -0
+
+		err = build_options(argstr, BUF_SIZE);
+		if (err) {
+			ret = err;

Again mentioned by Christoph, probably need some diagnostics on why err didn't get set to 0.

fabrics.c static int do_discover(char *argstr, bool connect) · line 624

Quoted 6 lines
fabrics.c +6 -0
+			continue;
+		}
+
+		err = do_discover(argstr, connect);
+		if (err) {
+			ret = err;

Same here.

Quoted 19 lines
fabrics.c +11 -0
+			continue;
+		}
+
+		free(args);
+		free(argv);
+	}
+
+	fclose(f);
+	return ret;
+}
+

?int discover(const char *desc, int argc, char **argv, bool connect) ?{
? char argstr[BUF_SIZE];

(quoted diff) +0 -0
@@ -597,11 +655,16 @@ int discover(const char *desc, int argc, char
 **argv, bool connect)

?
? cfg.nqn = NVME_DISC_SUBSYS_NAME;
?

fabrics.c int discover(const char *desc, int argc, char **argv, bool connect) · line 663

Quoted 9 lines
fabrics.c +6 -3
-	ret = build_options(argstr, BUF_SIZE);
-	if (ret)
-		return ret;
+	if (!cfg.transport && !cfg.traddr) {
+		return discover_from_conf_file(desc, argstr,
+				command_line_options, connect);
+	} else {
+		ret = build_options(argstr, BUF_SIZE);
+		if (ret)

same here.

Quoted 8 lines
(quoted diff) +1 -0
+			return ret;

?

fabrics.c +2 -1
-	return do_discover(argstr, connect);
+		return do_discover(argstr, connect);
+	}

?}
?
?int connect(const char *desc, int argc, char **argv)

author maintainer
#10 lore raw

PATCH v2 3/4 fabrics: Allow discover params to come from a conf file

Quoted 15 lines · J Freyensee

I missed some things on this patch.

Quoted 13 lines

#define BUF_SIZE 4096
#define PATH_NVME_FABRICS "/dev/nvme-fabrics"

fabrics.c +2 -0
+#define PATH_NVMF_DISC		"/etc/nvme/nvmf_disc"
+#define MAX_DISC_ARGS		10
 
 enum {
 	OPT_INSTANCE,

@@ -575,6 +577,62 @@ static int do_discover(char *argstr, bool
 connect)
 	return ret;
 }
Quoted 8 lines

fabrics.c static int do_discover(char *argstr, bool connect) · line 584

Quoted 6 lines
fabrics.c +5 -0
+static int discover_from_conf_file(const char *desc, char *argstr,
+	const struct argconfig_commandline_options *opts, bool
 connect)
+{
+	FILE *f;
+	char line[255], *ptr, *args, **argv;

Why is line 255 and not 256? Maybe want it a #define?

I can change to 256. Not sure if we need a define...

Quoted 11 lines
Quoted 11 lines
fabrics.c +9 -0
+	int argc, err, ret = 0;
+
+	f = fopen(PATH_NVMF_DISC, "r");
+	if (f == NULL) {
+		fprintf(stderr, "No discover params given and no %s
 conf\n", PATH_NVMF_DISC);
+		return -EINVAL;
+	}
+
+	while (fgets(line, sizeof(line), f) != NULL) {
Quoted 8 lines

fabrics.c static int do_discover(char *argstr, bool connect) · line 599

Quoted 6 lines
fabrics.c +6 -0
+		if (line[0] == '#' || line[0] == '\n')
+			continue;
+
+		args = strdup(line);
+		if (!args) {
+			fprintf(stderr, "failed to strdup args\n");

Be useful to print out the line so you know where you failed.

Its pretty explicit to know where this failed.

Quoted 10 lines
Quoted 8 lines
fabrics.c +6 -0
+			return -ENOMEM;
+		}
+
+		argv = calloc(MAX_DISC_ARGS, BUF_SIZE);
+		if (!argv) {
+			fprintf(stderr, "failed to allocate args

vector\n");

memory leak from not free'ing args in this case?

Right, fixed.

Quoted 3 lines

Also fprintf() says 'args' not 'argv'. And again may be useful to print out the line of where you failed.

changed to argv.

Quoted 11 lines
Quoted 11 lines
fabrics.c +9 -0
+			return -ENOMEM;
+		}
+
+		argc = 0;
+		argv[argc++] = "discover";
+		while ((ptr = strsep(&args, " =\n")) != NULL)
+			argv[argc++] = ptr;
+
+		argconfig_parse(argc, argv, desc, opts, &cfg,
Quoted 8 lines

fabrics.c static int do_discover(char *argstr, bool connect) · line 618

Quoted 5 lines

sizeof(cfg));

fabrics.c +4 -0
+
+		err = build_options(argstr, BUF_SIZE);
+		if (err) {
+			ret = err;

Again mentioned by Christoph, probably need some diagnostics on why err didn't get set to 0.

build_options will print out the failure reason.

author maintainer
#11 lore raw

PATCH v2 4/4 fabrics: Take the hostnqn parameter from a conf file if not given

In reply to Sagi Grimberg

In order to allow persistent hostnqns, take the hostnqn parameter for /etc/nvme/hostnqn if exists.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 fabrics.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)
fabrics.c +21 -1
index 54ed8e17b527..90308f07d0fb 100644
@@ -57,6 +57,7 @@ struct config {
 #define BUF_SIZE		4096
 #define PATH_NVME_FABRICS	"/dev/nvme-fabrics"
 #define PATH_NVMF_DISC		"/etc/nvme/nvmf_disc"
+#define PATH_NVMF_HOSTNQN	"/etc/nvme/hostnqn"
 #define MAX_DISC_ARGS		10
 
 enum {

@@ -405,6 +406,25 @@ static void save_discovery_log(struct nvmf_disc_rsp_page_hdr *log, int numrec)
 	close(fd);
 }
 
+static int nvmf_hostnqn_file(void)
+{
+	FILE *f;
+	char hostnqn[255];
+
+	f = fopen(PATH_NVMF_HOSTNQN, "r");
+	if (f == NULL)
+		return false;
+
+	if (fgets(hostnqn, sizeof(hostnqn), f) == NULL)
+		return false;
+
+	cfg.hostnqn = strdup(hostnqn);
+	if (!cfg.hostnqn)
+		return false;
+
+	return true;
+}
+
 static int build_options(char *argstr, int max_len)
 {
 	int len;

@@ -449,7 +469,7 @@ static int build_options(char *argstr, int max_len)
 		max_len -= len;
 	}
 
-	if (cfg.hostnqn) {
+	if (cfg.hostnqn || nvmf_hostnqn_file()) {
 		len = snprintf(argstr, max_len, ",hostnqn=%s", cfg.hostnqn);
 		if (len < 0)
 			return -EINVAL;
Signature · 1 line
1.9.1
maintainer
#12 lore raw

PATCH v2 4/4 fabrics: Take the hostnqn parameter from a conf file if not given

Quoted 31 lines · Sagi Grimberg

In order to allow persistent hostnqns, take the hostnqn parameter for /etc/nvme/hostnqn if exists.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 fabrics.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)
fabrics.c +8 -0
index 54ed8e17b527..90308f07d0fb 100644
@@ -57,6 +57,7 @@ struct config {
 #define BUF_SIZE		4096
 #define PATH_NVME_FABRICS	"/dev/nvme-fabrics"
 #define PATH_NVMF_DISC		"/etc/nvme/nvmf_disc"
+#define PATH_NVMF_HOSTNQN	"/etc/nvme/hostnqn"
 #define MAX_DISC_ARGS		10
 
 enum {

@@ -405,6 +406,25 @@ static void save_discovery_log(struct nvmf_disc_rsp_page_hdr *log, int numrec)
 	close(fd);
 }
 
+static int nvmf_hostnqn_file(void)
+{
+	FILE *f;
+	char hostnqn[255];
+
+	f = fopen(PATH_NVMF_HOSTNQN, "r");
+	if (f == NULL)

fabrics.c static void save_discovery_log(struct nvmf_disc_rsp_page_hdr *log, int numrec) · line 425

Quoted 10 lines
fabrics.c +10 -0
+		return false;
+
+	if (fgets(hostnqn, sizeof(hostnqn), f) == NULL)
+		return false;
+
+	cfg.hostnqn = strdup(hostnqn);
+	if (!cfg.hostnqn)
+		return false;
+
+	return true;

No fclose? The leak probably isn't bad for a short running program, but sooner or later someone is going to turn it into a library.

Also shouldn't we just read the 223 bytes of the actual NQN value instead of the on the wire field length? (which should be 256 anyway IIRC).

#13 lore raw

PATCH v2 4/4 fabrics: Take the hostnqn parameter from a conf file if not given

Quoted 48 lines · Christoph Hellwig
Quoted 33 lines · Sagi Grimberg

In order to allow persistent hostnqns, take the hostnqn parameter for /etc/nvme/hostnqn if exists.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
?fabrics.c | 22 +++++++++++++++++++++-
?1 file changed, 21 insertions(+), 1 deletion(-)

fabrics.c +0 -0
index 54ed8e17b527..90308f07d0fb 100644
@@ -57,6 +57,7 @@ struct config {

?#define BUF_SIZE 4096
?#define PATH_NVME_FABRICS "/dev/nvme-fabrics"
?#define PATH_NVMF_DISC "/etc/nvme/nvmf_disc"
+#define PATH_NVMF_HOSTNQN "/etc/nvme/hostnqn"
?#define MAX_DISC_ARGS 10
?
?enum {

fabrics.c +0 -0
@@ -405,6 +406,25 @@ static void save_discovery_log(struct
 nvmf_disc_rsp_page_hdr *log, int numrec)
 ?	close(fd);
 ?}

?

fabrics.c +7 -0
+static int nvmf_hostnqn_file(void)
+{
+	FILE *f;
+	char hostnqn[255];
+
+	f = fopen(PATH_NVMF_HOSTNQN, "r");
+	if (f == NULL)

fabrics.c static void save_discovery_log(struct nvmf_disc_rsp_page_hdr *log, int numrec) · line 425

Quoted 10 lines
fabrics.c +10 -0
+		return false;
+
+	if (fgets(hostnqn, sizeof(hostnqn), f) == NULL)
+		return false;
+
+	cfg.hostnqn = strdup(hostnqn);
+	if (!cfg.hostnqn)
+		return false;
+
+	return true;

No fclose???The leak probably isn't bad for a short running program, but sooner or later someone is going to turn it into a library.

Quoted 3 lines

Also shouldn't we just read the 223 bytes of the actual NQN value instead
of the on the wire field length? (which should be 256 anyway IIRC).

That's true, nqn names are 223, unless you have to store the name and keep it byte-aligned, then having a variable with length 256 is more appropriate, but then you'll need #define's, 1 for the NQN length, one to define the array (reminder to null-terminate the string too).

Quoted 1 line
#14 lore raw

v2 cover Useful fabrics patches

In reply to Sagi Grimberg
Quoted 1 line · Sagi Grimberg

Hey Keith,

Thanks, Sagi. I applied 1/4 with hch's review. The remaining sound like they have additional comments to consider, so holding off for the moment.