[PATCH v2 nvme-cli 0/4] Useful fabrics patches

[PATCH v2 nvme-cli 0/4] Useful fabrics patches

[Sagi Grimberg]

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

-- 
1.9.1

[PATCH v2 nvme-cli 1/4] fabrics: Allow ipv6 address resolution

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

diff --git a/fabrics.c b/fabrics.c
index 8a174d41b82b..961335d45bd2 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -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;
 		}
-- 
1.9.1

[PATCH v2 nvme-cli 2/4] fabrics: stringify discover output.

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

diff --git a/common.h b/common.h
index 639186d520b1..f0a94de57061 100644
--- a/common.h
+++ b/common.h
@@ -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
diff --git a/fabrics.c b/fabrics.c
index 961335d45bd2..aa18de9aac6d 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -69,6 +69,91 @@ static const match_table_t opt_tokens = {
 	{ OPT_ERR,		NULL		},
 };
 
+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";
+}
+
+static const char * const trtypes[] = {
+	[NVMF_TRTYPE_RDMA]	= "rdma",
+	[NVMF_TRTYPE_FC]	= "fibre-channel",
+	[NVMF_TRTYPE_LOOP]	= "loop",
+};
+
+static const char *trtype_str(__u8 trtype)
+{
+	return arg_str(trtypes, ARRAY_SIZE(trtypes), trtype);
+}
+
+static const char * const adrfams[] = {
+	[NVMF_ADDR_FAMILY_PCI]	= "pci",
+	[NVMF_ADDR_FAMILY_IP4]	= "ipv4",
+	[NVMF_ADDR_FAMILY_IP6]	= "ipv6",
+	[NVMF_ADDR_FAMILY_IB]	= "infiniband",
+	[NVMF_ADDR_FAMILY_FC]	= "fibre-channel",
+};
+
+static inline const char *adrfam_str(__u8 adrfam)
+{
+	return arg_str(adrfams, ARRAY_SIZE(adrfams), adrfam);
+}
+
+static const char * const nqntypes[] = {
+	[NVME_NQN_DISC]		= "discovery subsystem",
+	[NVME_NQN_NVME]		= "nvme subsystem",
+};
+
+static inline const char *nqntype_str(__u8 nqntype)
+{
+	return arg_str(nqntypes, ARRAY_SIZE(nqntypes), nqntype);
+}
+
+static const char * const treqs[] = {
+	[NVMF_TREQ_NOT_SPECIFIED]	= "unspecified transport requirements",
+	[NVMF_TREQ_REQUIRED]		= "required",
+	[NVMF_TREQ_NOT_REQUIRED]	= "not required",
+};
+
+static inline const char *treq_str(__u8 treq)
+{
+	return arg_str(treqs, ARRAY_SIZE(treqs), treq);
+}
+
+static const char * const prtypes[] = {
+	[NVMF_RDMA_PRTYPE_NOT_SPECIFIED]	= "not specified",
+	[NVMF_RDMA_PRTYPE_IB]			= "infiniband",
+	[NVMF_RDMA_PRTYPE_ROCE]			= "roce",
+	[NVMF_RDMA_PRTYPE_ROCEV2]		= "roce-v2",
+	[NVMF_RDMA_PRTYPE_IWARP]		= "iwarp",
+};
+
+static inline const char *prtype_str(__u8 prtype)
+{
+	return arg_str(prtypes, ARRAY_SIZE(prtypes), prtype);
+}
+
+static const char * const qptypes[] = {
+	[NVMF_RDMA_QPTYPE_CONNECTED]	= "connected",
+	[NVMF_RDMA_QPTYPE_DATAGRAM]	= "datagram",
+};
+
+static inline const char *qptype_str(__u8 qptype)
+{
+	return arg_str(qptypes, ARRAY_SIZE(qptypes), qptype);
+}
+
+static const char * const cms[] = {
+	[NVMF_RDMA_CMS_RDMA_CM]	= "rdma-cm",
+};
+
+static const char *cms_str(__u8 cm)
+{
+	return arg_str(cms, ARRAY_SIZE(cms), cm);
+}
+
 static int do_discover(char *argstr, bool connect);
 
 static int add_ctrl(const char *argstr)
@@ -276,44 +361,10 @@ static void print_discovery_log(struct nvmf_disc_rsp_page_hdr *log, int numrec)
 		struct nvmf_disc_rsp_page_entry *e = &log->entries[i];
 
 		printf("=====Discovery Log Entry %d======\n", i);
-
-		printf("trtype:  ");
-		switch(e->trtype) {
-		case NVMF_ADDR_FAMILY_IP4:
-			printf("ipv4\n");
-			break;
-		case NVMF_ADDR_FAMILY_IP6:
-			printf("ipv6\n");
-			break;
-		case NVMF_ADDR_FAMILY_IB:
-			printf("ib\n");
-			break;
-		case NVMF_ADDR_FAMILY_FC:
-			printf("fc\n");
-			break;
-		default:
-			printf("unknown\n");
-			break;
-		}
-
-		printf("adrfam:  ");
-		switch(e->adrfam) {
-		case NVMF_TRTYPE_RDMA:
-			printf("rdma\n");
-			break;
-		case NVMF_TRTYPE_FC:
-			printf("fc\n");
-			break;
-		case NVMF_TRTYPE_LOOP:
-			printf("loop\n");
-			break;
-		default:
-			printf("unknown\n");
-			break;
-		}
-
-		printf("nqntype: %d\n", e->nqntype);
-		printf("treq:    %d\n", e->treq);
+		printf("trtype:  %s\n", trtype_str(e->trtype));
+		printf("adrfam:  %s\n", adrfam_str(e->adrfam));
+		printf("nqntype: %s\n", nqntype_str(e->nqntype));
+		printf("treq:    %s\n", treq_str(e->treq));
 		printf("portid:  %d\n", e->portid);
 		printf("trsvcid: %s\n", e->trsvcid);
 		printf("subnqn:  %s\n", e->subnqn);
@@ -321,9 +372,9 @@ static void print_discovery_log(struct nvmf_disc_rsp_page_hdr *log, int numrec)
 
 		switch (e->trtype) {
 		case NVMF_TRTYPE_RDMA:
-			printf("rdma_prtype: %d\n", e->tsas.rdma.prtype);
-			printf("rdma_qptype: %d\n", e->tsas.rdma.qptype);
-			printf("rdma_cms:    %d\n", e->tsas.rdma.cms);
+			printf("rdma_prtype: %s\n", prtype_str(e->tsas.rdma.prtype));
+			printf("rdma_qptype: %s\n", qptype_str(e->tsas.rdma.qptype));
+			printf("rdma_cms:    %s\n", cms_str(e->tsas.rdma.cms));
 			printf("rdma_pkey: 0x%04x\n", e->tsas.rdma.pkey);
 			break;
 		}
-- 
1.9.1

[PATCH v2 nvme-cli 3/4] fabrics: Allow discover params to come from a conf file

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

diff --git a/fabrics.c b/fabrics.c
index aa18de9aac6d..54ed8e17b527 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -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)
-- 
1.9.1

[PATCH v2 nvme-cli 4/4] fabrics: Take the hostnqn parameter from a conf file if not given

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

diff --git a/fabrics.c b/fabrics.c
index 54ed8e17b527..90308f07d0fb 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -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;
-- 
1.9.1

[PATCH v2 nvme-cli 2/4] fabrics: stringify discover output.

[Christoph Hellwig]

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

[PATCH v2 nvme-cli 3/4] fabrics: Allow discover params to come from a conf file

[Christoph Hellwig]

On Mon, Aug 08, 2016@02:57:59PM +0300, Sagi Grimberg wrote:
> 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?

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

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

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

[PATCH v2 nvme-cli 4/4] fabrics: Take the hostnqn parameter from a conf file if not given

[Christoph Hellwig]

On Mon, Aug 08, 2016@02:58:00PM +0300, Sagi Grimberg wrote:
> 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(-)
> 
> diff --git a/fabrics.c b/fabrics.c
> index 54ed8e17b527..90308f07d0fb 100644
> --- a/fabrics.c
> +++ b/fabrics.c
> @@ -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;

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

[PATCH v2 nvme-cli 0/4] Useful fabrics patches

[Keith Busch]

On Mon, Aug 08, 2016@02:57:56PM +0300, Sagi Grimberg wrote:
> 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.

[PATCH v2 nvme-cli 1/4] fabrics: Allow ipv6 address resolution

[J Freyensee]

On Mon, 2016-08-08@14:57 +0300, Sagi Grimberg wrote:
> 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(-)
> 
> diff --git a/fabrics.c b/fabrics.c
> index 8a174d41b82b..961335d45bd2 100644
> --- a/fabrics.c
> +++ b/fabrics.c
> @@ -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) {

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.

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

[PATCH v2 nvme-cli 3/4] fabrics: Allow discover params to come from a conf file

[J Freyensee]

On Mon, 2016-08-08@15:35 +0200, Christoph Hellwig wrote:
> On Mon, Aug 08, 2016@02:57:59PM +0300, Sagi Grimberg wrote:
> > 
> > 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.

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


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

[PATCH v2 nvme-cli 4/4] fabrics: Take the hostnqn parameter from a conf file if not given

[J Freyensee]

On Mon, 2016-08-08@15:36 +0200, Christoph Hellwig wrote:
> On Mon, Aug 08, 2016@02:58:00PM +0300, Sagi Grimberg wrote:
> > 
> > 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(-)
> > 
> > diff --git a/fabrics.c b/fabrics.c
> > index 54ed8e17b527..90308f07d0fb 100644
> > --- a/fabrics.c
> > +++ b/fabrics.c
> > @@ -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;
> 
> 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).

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

> 

[PATCH v2 nvme-cli 3/4] fabrics: Allow discover params to come from a conf file

[J Freyensee]

I missed some things on this patch.

> ?
> ?#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;

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

> +	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");

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

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


> +			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;

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

> +			continue;
> +		}
> +
> +		err = do_discover(argstr, connect);
> +		if (err) {
> +			ret = err;

Same here.

> +			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)

same here.

> +			return ret;
> ?
> -	return do_discover(argstr, connect);
> +		return do_discover(argstr, connect);
> +	}
> ?}
> ?
> ?int connect(const char *desc, int argc, char **argv)

[PATCH v2 nvme-cli 3/4] fabrics: Allow discover params to come from a conf file

[Sagi Grimberg]

On 08/08/16 16:35, Christoph Hellwig wrote:
> On Mon, Aug 08, 2016@02:57:59PM +0300, Sagi Grimberg wrote:
>> 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

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

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

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

[PATCH v2 nvme-cli 3/4] fabrics: Allow discover params to come from a conf file

[Sagi Grimberg]

On 09/08/16 00:51, J Freyensee wrote:
> I missed some things on this patch.
>
>>
>>  #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;
>
> Why is line 255 and not 256?  Maybe want it a #define?

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

>
>> +	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");
>
> Be useful to print out the line so you know where you failed.

Its pretty explicit to know where this failed.

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

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

changed to argv.

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