[PATCH nvme-cli 0/4] Some useful fabrics patches

[PATCH nvme-cli 0/4] Some useful fabrics patches

[Sagi Grimberg]

Hey Keith,

So here are some useful fabrics patches I had in the pipe.
The first allows for ipv6 address resolution (resend) as
we will support it in fabrics for 4.9.

Patch 2 makes the discover output nice and pretty by outputing
human readable strings instead of numeric values.

Patch 3 just removes a redundant call.

Patch 4 is rfc, This was for my own convenience being too
lazy for manual parameterizing when discovering/connecting to nvmf
targets so this should allow to parameterize in a conf file.
Not sure if you are fond of the approach so I'd love to see some feedback.

Sagi Grimberg (4):
  fabrics: Allow ipv6 address resolution
  fabrics: stringify discover output.
  fabrics: Remove redundant build_options
  fabrics: Allow discover params to come from a conf file

 fabrics.c | 175 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 158 insertions(+), 17 deletions(-)

-- 
1.9.1

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

[Sagi Grimberg]

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 fabrics.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fabrics.c b/fabrics.c
index 4bf557b5e672..221e34e5e39b 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -390,7 +390,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 nvme-cli 1/4] fabrics: Allow ipv6 address resolution

[Christoph Hellwig]

On Sun, Aug 07, 2016@03:19:22PM +0300, Sagi Grimberg wrote:
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>

Looks fine, although if we only get one more I'll insist on a switch
statement :)

Reviewed-by: Christoph Hellwig <hch at lst.de>

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

[J Freyensee]

On Mon, 2016-08-08@08:55 +0200, Christoph Hellwig wrote:
> On Sun, Aug 07, 2016@03:19:22PM +0300, Sagi Grimberg wrote:
> > 
> > Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> 
> Looks fine, although if we only get one more I'll insist on a switch
> statement :)

dido :-)

Reviewed-by: Jay Freyensee <james_p_freyensee at linux.intel.com>

> 
> Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH 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>
---
 fabrics.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 107 insertions(+), 8 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index 221e34e5e39b..9f99e6175428 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -63,6 +63,106 @@ static const match_table_t opt_tokens = {
 	{ OPT_ERR,		NULL		},
 };
 
+#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
+
+static const char * const trtypes[] = {
+	[NVMF_TRTYPE_RDMA]	= "rdma",
+	[NVMF_TRTYPE_FC]	= "fibre-channel",
+	[NVMF_TRTYPE_LOOP]	= "loop",
+};
+
+static inline const char *trtype_str(__u8 trtype)
+{
+	size_t idx = trtype;
+
+	return (idx < ARRAY_SIZE(trtypes) && trtypes[idx]) ?
+			trtypes[idx] : "unrecognized";
+}
+
+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)
+{
+	size_t idx = adrfam;
+
+	return (idx < ARRAY_SIZE(adrfams) && adrfams[idx]) ?
+			adrfams[idx] : "unrecognized";
+}
+
+static const char * const nqntypes[] = {
+	[NVME_NQN_DISC]		= "discovery subsystem",
+	[NVME_NQN_NVME]		= "nvme subsystem",
+};
+
+static inline const char *nqntype_str(__u8 nqntype)
+{
+	size_t idx = nqntype;
+
+	return (idx < ARRAY_SIZE(nqntypes) && nqntypes[idx]) ?
+			nqntypes[idx] : "unrecognized";
+}
+
+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)
+{
+	size_t idx = treq;
+
+	return (idx < ARRAY_SIZE(treqs) && treqs[idx]) ?
+			treqs[idx] : "unrecognized";
+}
+
+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)
+{
+	size_t idx = prtype;
+
+	return (idx < ARRAY_SIZE(prtypes) && prtypes[idx]) ?
+			prtypes[idx] : "unrecognized";
+}
+
+static const char * const qptypes[] = {
+	[NVMF_RDMA_QPTYPE_CONNECTED]	= "connected",
+	[NVMF_RDMA_QPTYPE_DATAGRAM]	= "datagram",
+};
+
+static inline const char *qptype_str(__u8 qptype)
+{
+	size_t idx = qptype;
+
+	return (idx < ARRAY_SIZE(qptypes) && qptypes[idx]) ?
+			qptypes[idx] : "unrecognized";
+}
+
+static const char * const cms[] = {
+	[NVMF_RDMA_CMS_RDMA_CM]	= "rdma-cm",
+};
+
+static const char *cms_str(__u8 cm)
+{
+	size_t idx = cm;
+
+	return (idx < ARRAY_SIZE(cms) && cms[idx]) ?
+			cms[idx] : "unrecognized";
+}
+
 static int do_discover(char *argstr, bool connect);
 
 static int add_ctrl(const char *argstr)
@@ -270,10 +370,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:  %d\n", e->trtype);
-		printf("adrfam:  %d\n", e->adrfam);
-		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);
@@ -281,10 +381,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_pkey: 0x%04x\n", e->tsas.rdma.pkey);
+			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));
 			break;
 		}
 	}
-- 
1.9.1

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

[Christoph Hellwig]

On Sun, Aug 07, 2016@03:19:23PM +0300, Sagi Grimberg wrote:
> Just so we have a nice readable output.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>  fabrics.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 107 insertions(+), 8 deletions(-)
> 
> diff --git a/fabrics.c b/fabrics.c
> index 221e34e5e39b..9f99e6175428 100644
> --- a/fabrics.c
> +++ b/fabrics.c
> @@ -63,6 +63,106 @@ static const match_table_t opt_tokens = {
>  	{ OPT_ERR,		NULL		},
>  };
>  
> +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))

should go into a header so that all of the tool has it available.

> +static const char *cms_str(__u8 cm)
> +{
> +	size_t idx = cm;
> +
> +	return (idx < ARRAY_SIZE(cms) && cms[idx]) ?
> +			cms[idx] : "unrecognized";
> +}

How about a little helper instead of duplicating this pattern many
times, e.g

static const char *lookup_string(const char **strings, size_t array_size,
		size_t id)
{
	if (idx < array_size && strings[idx])
		return strings[idx];
	return "unrecognized";
}

also makes me wonder why we even bother with the inline annotation
for such slow path code.

Otherwise this looks nice to me.

[PATCH 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>
>> ---
>>  fabrics.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 107 insertions(+), 8 deletions(-)
>>
>> diff --git a/fabrics.c b/fabrics.c
>> index 221e34e5e39b..9f99e6175428 100644
>> --- a/fabrics.c
>> +++ b/fabrics.c
>> @@ -63,6 +63,106 @@ static const match_table_t opt_tokens = {
>>  	{ OPT_ERR,		NULL		},
>>  };
>>
>> +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
>
> should go into a header so that all of the tool has it available.

You're right, Moved to common.h

>> +static const char *cms_str(__u8 cm)
>> +{
>> +	size_t idx = cm;
>> +
>> +	return (idx < ARRAY_SIZE(cms) && cms[idx]) ?
>> +			cms[idx] : "unrecognized";
>> +}
>
> How about a little helper instead of duplicating this pattern many
> times, e.g
>
> static const char *lookup_string(const char **strings, size_t array_size,
> 		size_t id)
> {
> 	if (idx < array_size && strings[idx])
> 		return strings[idx];
> 	return "unrecognized";
> }

It is nicer, thanks!

Added, just with a different name: arg_str()

>
> also makes me wonder why we even bother with the inline annotation
> for such slow path code.

Because it's really short and simply returns a string. I can lose it...

[PATCH nvme-cli 3/4] fabrics: Remove redundant build_options

[Sagi Grimberg]

We call it again in do_discover.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 fabrics.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index 9f99e6175428..8c1a8331cc45 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -584,7 +584,6 @@ static int do_discover(char *argstr, bool connect)
 int discover(const char *desc, int argc, char **argv, bool connect)
 {
 	char argstr[BUF_SIZE];
-	int ret;
 	const struct argconfig_commandline_options command_line_options[] = {
 		{"transport", 't', "LIST", CFG_STRING, &cfg.transport, required_argument,
 			"transport type" },
@@ -599,12 +598,6 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 
 	argconfig_parse(argc, argv, desc, command_line_options, &cfg, sizeof(cfg));
 
-	cfg.nqn = NVME_DISC_SUBSYS_NAME;
-
-	ret = build_options(argstr, BUF_SIZE);
-	if (ret)
-		return ret;
-
 	return do_discover(argstr, connect);
 }
 
-- 
1.9.1

[PATCH nvme-cli 3/4] fabrics: Remove redundant build_options

[Christoph Hellwig]

On Sun, Aug 07, 2016@03:19:24PM +0300, Sagi Grimberg wrote:
> We call it again in do_discover.

Do we?  At least in my copy of the tree we don't, and given that
connect_ctrl builds the argstr differently that would be a bad idea..

[PATCH nvme-cli 3/4] fabrics: Remove redundant build_options

[Sagi Grimberg]

>> We call it again in do_discover.
>
> Do we?  At least in my copy of the tree we don't, and given that
> connect_ctrl builds the argstr differently that would be a bad idea..

I wasn't rebased... you can discard this one...

[PATCH RFC nvme-cli 4/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>
---
Not sure if anyone will find this useful, but worth a
shot.

 fabrics.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/fabrics.c b/fabrics.c
index 8c1a8331cc45..53af68d66c57 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -50,6 +50,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,
@@ -581,6 +583,48 @@ 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, 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)
+			continue;
+
+		argv = calloc(MAX_DISC_ARGS, BUF_SIZE);
+		if (!argv)
+			return -EINVAL;
+
+		argc = 0;
+		argv[argc++] = "discover";
+		while ((ptr = strsep(&args, " =")) != NULL)
+			argv[argc++] = ptr;
+
+		argconfig_parse(argc, argv, desc, opts, &cfg, sizeof(cfg));
+
+		ret |= do_discover(argstr, connect);
+
+		free(args);
+		free(argv);
+	}
+
+	fclose(f);
+	return ret;
+}
+
 int discover(const char *desc, int argc, char **argv, bool connect)
 {
 	char argstr[BUF_SIZE];
@@ -598,7 +642,11 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 
 	argconfig_parse(argc, argv, desc, command_line_options, &cfg, sizeof(cfg));
 
-	return do_discover(argstr, connect);
+	if (!cfg.transport && !cfg.traddr)
+		return discover_from_conf_file(desc, argstr,
+				command_line_options, connect);
+	else
+		return do_discover(argstr, connect);
 }
 
 int connect(const char *desc, int argc, char **argv)
-- 
1.9.1

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

[Christoph Hellwig]

On Sun, Aug 07, 2016@03:19:25PM +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.
> 
> 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>
> ---
> Not sure if anyone will find this useful, but worth a
> shot.

Looks ok to me.  While the options are an odd config file format
having them the same as the command line at least makes it easy
to document both in one spot.

Oh, yeah - documentation.  Reminds me that I still need to deliver
the man pages for the fabrics command..

Anyway, thanks for looking into this, we really need the config
file!

[PATCH RFC nvme-cli 4/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>
>> ---
>> Not sure if anyone will find this useful, but worth a
>> shot.
>
> Looks ok to me.

I'll settle for ok :)

> While the options are an odd config file format
> having them the same as the command line at least makes it easy
> to document both in one spot.

It was the easiest for me in terms of parsing (or non-parsing to
be exact).

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

[J Freyensee]

On Sun, 2016-08-07@15:19 +0300, Sagi Grimberg wrote:

I think it's a good idea! ?A few minor things.

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

1. I think the "official trademark" of NVMe-over-Fabrics is NVMeo-F.
?So maybe tweak the conf file to /etc/nvme/nvmeof_disc (the dash
between the o and F is weird and would look dumb here).

2. Be nice to see a quick documentation example of contents of the conf
file in this patch as we don't have any documentation yet (this way
someone, not necessarily the author of this patch, can more easily
write the documentation).

Jay