[PATCH nvme-cli rfc] fabrics: support default connect/discover args

[PATCH nvme-cli rfc] fabrics: support default connect/discover args

[Sagi Grimberg]

Introduce /etc/nvme/defargs.conf where we allow the user to
specify connect/discover parameters once and not for every
controller. The cli will always ingest the content of this
file before parsing cmdline args such that the user can
override them.

The format is simply writing the arguments into the file as
if they were appended to the execution command.

Also, properly install this file with some minimal documentation.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
This was raised before in the past that we don't have some place
to store default connect args.

For example, when handling automatic discovery change log events
this can be a way for the user to set global default arguments.

Feedback is welcome.

 Makefile            |  3 +++
 etc/defargs.conf.in |  5 ++++
 fabrics.c           | 63 +++++++++++++++++++++++++++++++++++++++++++++
 nvme.spec.in        |  1 +
 4 files changed, 72 insertions(+)
 create mode 100644 etc/defargs.conf.in

diff --git a/Makefile b/Makefile
index 4bfbebbd156a..6f11941b5d3e 100644
--- a/Makefile
+++ b/Makefile
@@ -105,6 +105,9 @@ install-etc:
 	if [ ! -f $(DESTDIR)$(SBINDIR)/nvme/discovery.conf ]; then \
 		$(INSTALL) -m 644 -T ./etc/discovery.conf.in $(DESTDIR)$(SYSCONFDIR)/nvme/discovery.conf; \
 	fi
+	if [ ! -f $(DESTDIR)$(SBINDIR)/nvme/defargs.conf ]; then \
+		$(INSTALL) -m 644 -T ./etc/defargs.conf.in $(DESTDIR)$(SYSCONFDIR)/nvme/defargs.conf; \
+	fi
 
 install-spec: install-bin install-man install-bash-completion install-zsh-completion install-etc
 install: install-spec install-hostparams
diff --git a/etc/defargs.conf.in b/etc/defargs.conf.in
new file mode 100644
index 000000000000..e91106bf5bbf
--- /dev/null
+++ b/etc/defargs.conf.in
@@ -0,0 +1,5 @@
+# Used for extracting default controller connect parameters
+#
+# Example:
+# --keep-alive-tmo=<x> --reconnect-delay=<y> --ctrl-loss-tmo=<z> --nr-io-queues=<u>
+# --queue-size=<v>
diff --git a/fabrics.c b/fabrics.c
index 511de06aec97..e609e679c832 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -72,8 +72,10 @@ static struct config {
 #define PATH_NVMF_DISC		"/etc/nvme/discovery.conf"
 #define PATH_NVMF_HOSTNQN	"/etc/nvme/hostnqn"
 #define PATH_NVMF_HOSTID	"/etc/nvme/hostid"
+#define PATH_NVMF_DEFARGS	"/etc/nvme/defargs.conf"
 #define SYS_NVME		"/sys/class/nvme"
 #define MAX_DISC_ARGS		10
+#define MAX_DEF_ARGS		10
 #define MAX_DISC_RETRIES	10
 
 enum {
@@ -894,6 +896,59 @@ static int do_discover(char *argstr, bool connect)
 	return ret;
 }
 
+static int nvmf_parse_defargs(const char *desc,
+		const struct argconfig_commandline_options *opts)
+{
+	FILE *f;
+	char line[256], *ptr, *args, **argv;
+	int argc = 0, ret = 0;
+
+	f = fopen(PATH_NVMF_DEFARGS, "r");
+	if (f == NULL)
+		return 0;
+
+	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");
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		argv = calloc(MAX_DEF_ARGS, BUF_SIZE);
+		if (!argv) {
+			fprintf(stderr, "failed to allocate argv vector\n");
+			free(args);
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		argc = 0;
+		argv[argc++] = "dummy";
+		while ((ptr = strsep(&args, " =\n")) != NULL)
+			argv[argc++] = ptr;
+
+		ret = argconfig_parse(argc, argv, desc, opts, &cfg, sizeof(cfg));
+		free(args);
+		free(argv);
+		if (ret)
+			goto out;
+
+		if (cfg.transport || cfg.traddr || cfg.trsvcid || cfg.nqn) {
+			fprintf(stderr, "args transport, traddr, trsvcid, nqn "
+				"cannot have a default\n");
+			ret = -EINVAL;
+			goto out;
+		}
+	}
+out:
+	fclose(f);
+	return ret;
+}
+
 static int discover_from_conf_file(const char *desc, char *argstr,
 		const struct argconfig_commandline_options *opts, bool connect)
 {
@@ -981,6 +1036,10 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 		{NULL},
 	};
 
+	ret = nvmf_parse_defargs(desc, command_line_options);
+	if (ret)
+		return ret;
+
 	ret = argconfig_parse(argc, argv, desc, command_line_options, &cfg,
 			sizeof(cfg));
 	if (ret)
@@ -1026,6 +1085,10 @@ int connect(const char *desc, int argc, char **argv)
 		{NULL},
 	};
 
+	ret = nvmf_parse_defargs(desc, command_line_options);
+	if (ret)
+		return ret;
+
 	ret = argconfig_parse(argc, argv, desc, command_line_options, &cfg,
 			sizeof(cfg));
 	if (ret)
diff --git a/nvme.spec.in b/nvme.spec.in
index 6934f8fd605b..e2240b61d79e 100644
--- a/nvme.spec.in
+++ b/nvme.spec.in
@@ -35,6 +35,7 @@ make install-spec DESTDIR=%{buildroot} PREFIX=/usr
 %{_sysconfdir}/nvme/hostnqn
 %{_sysconfdir}/nvme/hostid
 %{_sysconfdir}/nvme/discovery.conf
+%{_sysconfdir}/nvme/defargs.conf
 
 %clean
 rm -rf $RPM_BUILD_ROOT
-- 
2.17.1

[PATCH nvme-cli rfc] fabrics: support default connect/discover args

[Minwoo Im]

Hi, Sagi.

> -----Original Message-----
> From: Linux-nvme [mailto:linux-nvme-bounces at lists.infradead.org] On
> Behalf Of Sagi Grimberg
> Sent: Tuesday, April 30, 2019 7:54 AM
> To: linux-nvme at lists.infradead.org
> Cc: Keith Busch; Hannes Reinecke; Johannes Thumshirn; Christoph Hellwig;
> James Smart
> Subject: [PATCH nvme-cli rfc] fabrics: support default connect/discover args
> 
> Introduce /etc/nvme/defargs.conf where we allow the user to
> specify connect/discover parameters once and not for every
> controller. The cli will always ingest the content of this
> file before parsing cmdline args such that the user can
> override them.
> 
> The format is simply writing the arguments into the file as
> if they were appended to the execution command.
> 
> Also, properly install this file with some minimal documentation.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
> This was raised before in the past that we don't have some place
> to store default connect args.
> 
> For example, when handling automatic discovery change log events
> this can be a way for the user to set global default arguments.
> 
> Feedback is welcome.
> 
>  Makefile            |  3 +++
>  etc/defargs.conf.in |  5 ++++
>  fabrics.c           | 63
> +++++++++++++++++++++++++++++++++++++++++++++
>  nvme.spec.in        |  1 +
>  4 files changed, 72 insertions(+)
>  create mode 100644 etc/defargs.conf.in
> 
> diff --git a/Makefile b/Makefile
> index 4bfbebbd156a..6f11941b5d3e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -105,6 +105,9 @@ install-etc:
>  	if [ ! -f $(DESTDIR)$(SBINDIR)/nvme/discovery.conf ]; then \
>  		$(INSTALL) -m 644 -T ./etc/discovery.conf.in
> $(DESTDIR)$(SYSCONFDIR)/nvme/discovery.conf; \
>  	fi
> +	if [ ! -f $(DESTDIR)$(SBINDIR)/nvme/defargs.conf ]; then \
> +		$(INSTALL) -m 644 -T ./etc/defargs.conf.in
> $(DESTDIR)$(SYSCONFDIR)/nvme/defargs.conf; \
> +	fi
> 
>  install-spec: install-bin install-man install-bash-completion install-zsh-
> completion install-etc
>  install: install-spec install-hostparams
> diff --git a/etc/defargs.conf.in b/etc/defargs.conf.in
> new file mode 100644
> index 000000000000..e91106bf5bbf
> --- /dev/null
> +++ b/etc/defargs.conf.in
> @@ -0,0 +1,5 @@
> +# Used for extracting default controller connect parameters
> +#
> +# Example:
> +# --keep-alive-tmo=<x> --reconnect-delay=<y> --ctrl-loss-tmo=<z> --nr-io-
> queues=<u>
> +# --queue-size=<v>
> diff --git a/fabrics.c b/fabrics.c
> index 511de06aec97..e609e679c832 100644
> --- a/fabrics.c
> +++ b/fabrics.c
> @@ -72,8 +72,10 @@ static struct config {
>  #define PATH_NVMF_DISC		"/etc/nvme/discovery.conf"
>  #define PATH_NVMF_HOSTNQN	"/etc/nvme/hostnqn"
>  #define PATH_NVMF_HOSTID	"/etc/nvme/hostid"
> +#define PATH_NVMF_DEFARGS	"/etc/nvme/defargs.conf"
>  #define SYS_NVME		"/sys/class/nvme"
>  #define MAX_DISC_ARGS		10
> +#define MAX_DEF_ARGS		10
>  #define MAX_DISC_RETRIES	10
> 
>  enum {
> @@ -894,6 +896,59 @@ static int do_discover(char *argstr, bool connect)
>  	return ret;
>  }
> 
> +static int nvmf_parse_defargs(const char *desc,
> +		const struct argconfig_commandline_options *opts)
> +{
> +	FILE *f;
> +	char line[256], *ptr, *args, **argv;
> +	int argc = 0, ret = 0;

Nit here: we don't need initialize 'argc' here because it will be zero before
getting used below.

This looks good to me, by the way.
Reviewed-by: Minwoo Im <minwoo.im at samsung.com>

Thanks,

> +
> +	f = fopen(PATH_NVMF_DEFARGS, "r");
> +	if (f == NULL)
> +		return 0;
> +
> +	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");
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +
> +		argv = calloc(MAX_DEF_ARGS, BUF_SIZE);
> +		if (!argv) {
> +			fprintf(stderr, "failed to allocate argv vector\n");
> +			free(args);
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +
> +		argc = 0;
> +		argv[argc++] = "dummy";
> +		while ((ptr = strsep(&args, " =\n")) != NULL)
> +			argv[argc++] = ptr;
> +
> +		ret = argconfig_parse(argc, argv, desc, opts, &cfg, sizeof(cfg));
> +		free(args);
> +		free(argv);
> +		if (ret)
> +			goto out;
> +
> +		if (cfg.transport || cfg.traddr || cfg.trsvcid || cfg.nqn) {
> +			fprintf(stderr, "args transport, traddr, trsvcid, nqn "
> +				"cannot have a default\n");
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +	}
> +out:
> +	fclose(f);
> +	return ret;
> +}
> +
>  static int discover_from_conf_file(const char *desc, char *argstr,
>  		const struct argconfig_commandline_options *opts, bool
> connect)
>  {
> @@ -981,6 +1036,10 @@ int discover(const char *desc, int argc, char **argv,
> bool connect)
>  		{NULL},
>  	};
> 
> +	ret = nvmf_parse_defargs(desc, command_line_options);
> +	if (ret)
> +		return ret;
> +
>  	ret = argconfig_parse(argc, argv, desc, command_line_options, &cfg,
>  			sizeof(cfg));
>  	if (ret)
> @@ -1026,6 +1085,10 @@ int connect(const char *desc, int argc, char **argv)
>  		{NULL},
>  	};
> 
> +	ret = nvmf_parse_defargs(desc, command_line_options);
> +	if (ret)
> +		return ret;
> +
>  	ret = argconfig_parse(argc, argv, desc, command_line_options, &cfg,
>  			sizeof(cfg));
>  	if (ret)
> diff --git a/nvme.spec.in b/nvme.spec.in
> index 6934f8fd605b..e2240b61d79e 100644
> --- a/nvme.spec.in
> +++ b/nvme.spec.in
> @@ -35,6 +35,7 @@ make install-spec DESTDIR=%{buildroot} PREFIX=/usr
>  %{_sysconfdir}/nvme/hostnqn
>  %{_sysconfdir}/nvme/hostid
>  %{_sysconfdir}/nvme/discovery.conf
> +%{_sysconfdir}/nvme/defargs.conf
> 
>  %clean
>  rm -rf $RPM_BUILD_ROOT
> --
> 2.17.1
> 
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH nvme-cli rfc] fabrics: support default connect/discover args

[Heitke, Kenneth]

On 4/29/2019 4:53 PM, Sagi Grimberg wrote:
> Introduce /etc/nvme/defargs.conf where we allow the user to
> specify connect/discover parameters once and not for every
> controller. The cli will always ingest the content of this
> file before parsing cmdline args such that the user can
> override them.
> 
> The format is simply writing the arguments into the file as
> if they were appended to the execution command.
> 
> Also, properly install this file with some minimal documentation.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
> This was raised before in the past that we don't have some place
> to store default connect args.
> 
> For example, when handling automatic discovery change log events
> this can be a way for the user to set global default arguments.
> 
> Feedback is welcome.
> 
>   Makefile            |  3 +++
>   etc/defargs.conf.in |  5 ++++
>   fabrics.c           | 63 +++++++++++++++++++++++++++++++++++++++++++++
>   nvme.spec.in        |  1 +
>   4 files changed, 72 insertions(+)
>   create mode 100644 etc/defargs.conf.in
> 
> diff --git a/Makefile b/Makefile
> index 4bfbebbd156a..6f11941b5d3e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -105,6 +105,9 @@ install-etc:
>   	if [ ! -f $(DESTDIR)$(SBINDIR)/nvme/discovery.conf ]; then \
>   		$(INSTALL) -m 644 -T ./etc/discovery.conf.in $(DESTDIR)$(SYSCONFDIR)/nvme/discovery.conf; \
>   	fi
> +	if [ ! -f $(DESTDIR)$(SBINDIR)/nvme/defargs.conf ]; then \
> +		$(INSTALL) -m 644 -T ./etc/defargs.conf.in $(DESTDIR)$(SYSCONFDIR)/nvme/defargs.conf; \
> +	fi
>   
>   install-spec: install-bin install-man install-bash-completion install-zsh-completion install-etc
>   install: install-spec install-hostparams
> diff --git a/etc/defargs.conf.in b/etc/defargs.conf.in
> new file mode 100644
> index 000000000000..e91106bf5bbf
> --- /dev/null
> +++ b/etc/defargs.conf.in
> @@ -0,0 +1,5 @@
> +# Used for extracting default controller connect parameters
> +#
> +# Example:
> +# --keep-alive-tmo=<x> --reconnect-delay=<y> --ctrl-loss-tmo=<z> --nr-io-queues=<u>
> +# --queue-size=<v>
> diff --git a/fabrics.c b/fabrics.c
> index 511de06aec97..e609e679c832 100644
> --- a/fabrics.c
> +++ b/fabrics.c
> @@ -72,8 +72,10 @@ static struct config {
>   #define PATH_NVMF_DISC		"/etc/nvme/discovery.conf"
>   #define PATH_NVMF_HOSTNQN	"/etc/nvme/hostnqn"
>   #define PATH_NVMF_HOSTID	"/etc/nvme/hostid"
> +#define PATH_NVMF_DEFARGS	"/etc/nvme/defargs.conf"
>   #define SYS_NVME		"/sys/class/nvme"
>   #define MAX_DISC_ARGS		10
> +#define MAX_DEF_ARGS		10
>   #define MAX_DISC_RETRIES	10
>   
>   enum {
> @@ -894,6 +896,59 @@ static int do_discover(char *argstr, bool connect)
>   	return ret;
>   }
>   
> +static int nvmf_parse_defargs(const char *desc,
> +		const struct argconfig_commandline_options *opts)
> +{
> +	FILE *f;
> +	char line[256], *ptr, *args, **argv;
> +	int argc = 0, ret = 0;
> +
> +	f = fopen(PATH_NVMF_DEFARGS, "r");
> +	if (f == NULL)
> +		return 0;
> +
> +	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");
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +
> +		argv = calloc(MAX_DEF_ARGS, BUF_SIZE);
> +		if (!argv) {
> +			fprintf(stderr, "failed to allocate argv vector\n");
> +			free(args);
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +
> +		argc = 0;
> +		argv[argc++] = "dummy";
> +		while ((ptr = strsep(&args, " =\n")) != NULL)

There might be an issue here because args will be updated each time to
point to the next position in the string after the current token. When
you go to 'free' args, the pointer may no longer be valid.

> +			argv[argc++] = ptr;
> +
> +		ret = argconfig_parse(argc, argv, desc, opts, &cfg, sizeof(cfg));
> +		free(args);
> +		free(argv);
> +		if (ret)
> +			goto out;
> +
> +		if (cfg.transport || cfg.traddr || cfg.trsvcid || cfg.nqn) {
> +			fprintf(stderr, "args transport, traddr, trsvcid, nqn "
> +				"cannot have a default\n");
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +	}
> +out:
> +	fclose(f);
> +	return ret;
> +}
> +
>   static int discover_from_conf_file(const char *desc, char *argstr,
>   		const struct argconfig_commandline_options *opts, bool connect)
>   {
> @@ -981,6 +1036,10 @@ int discover(const char *desc, int argc, char **argv, bool connect)
>   		{NULL},
>   	};
>   
> +	ret = nvmf_parse_defargs(desc, command_line_options);
> +	if (ret)
> +		return ret;
> +
>   	ret = argconfig_parse(argc, argv, desc, command_line_options, &cfg,
>   			sizeof(cfg));
>   	if (ret)
> @@ -1026,6 +1085,10 @@ int connect(const char *desc, int argc, char **argv)
>   		{NULL},
>   	};
>   
> +	ret = nvmf_parse_defargs(desc, command_line_options);
> +	if (ret)
> +		return ret;
> +
>   	ret = argconfig_parse(argc, argv, desc, command_line_options, &cfg,
>   			sizeof(cfg));
>   	if (ret)
> diff --git a/nvme.spec.in b/nvme.spec.in
> index 6934f8fd605b..e2240b61d79e 100644
> --- a/nvme.spec.in
> +++ b/nvme.spec.in
> @@ -35,6 +35,7 @@ make install-spec DESTDIR=%{buildroot} PREFIX=/usr
>   %{_sysconfdir}/nvme/hostnqn
>   %{_sysconfdir}/nvme/hostid
>   %{_sysconfdir}/nvme/discovery.conf
> +%{_sysconfdir}/nvme/defargs.conf
>   
>   %clean
>   rm -rf $RPM_BUILD_ROOT
> 

[PATCH nvme-cli rfc] fabrics: support default connect/discover args

[Sagi Grimberg]

>> +??????? argc = 0;
>> +??????? argv[argc++] = "dummy";
>> +??????? while ((ptr = strsep(&args, " =\n")) != NULL)
> 
> There might be an issue here because args will be updated each time to
> point to the next position in the string after the current token. When
> you go to 'free' args, the pointer may no longer be valid.

Yea... We need another pointer for that...

[PATCH nvme-cli rfc] fabrics: support default connect/discover args

[Hannes Reinecke]

On 4/30/19 12:53 AM, Sagi Grimberg wrote:
> Introduce /etc/nvme/defargs.conf where we allow the user to
> specify connect/discover parameters once and not for every
> controller. The cli will always ingest the content of this
> file before parsing cmdline args such that the user can
> override them.
> 
> The format is simply writing the arguments into the file as
> if they were appended to the execution command.
> 
> Also, properly install this file with some minimal documentation.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
> This was raised before in the past that we don't have some place
> to store default connect args.
> 
> For example, when handling automatic discovery change log events
> this can be a way for the user to set global default arguments.
> 
> Feedback is welcome.
> 
>   Makefile            |  3 +++
>   etc/defargs.conf.in |  5 ++++
>   fabrics.c           | 63 +++++++++++++++++++++++++++++++++++++++++++++
>   nvme.spec.in        |  1 +
>   4 files changed, 72 insertions(+)
>   create mode 100644 etc/defargs.conf.in
> 

I do like the idea, but not necessarily the naming.
Can't we just call it 'default.conf' ?

What about precedence?
I would have expected that we should have

defargs.conf
discovery.conf
<cmdline>

Is that the case?
And shouldn't we document that somewhere?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG N?rnberg)

[PATCH nvme-cli rfc] fabrics: support default connect/discover args

[Sagi Grimberg]

> I do like the idea, but not necessarily the naming.
> Can't we just call it 'default.conf' ?

It's not as self-explanatory, but I'm fine with default.conf...

> What about precedence?
> I would have expected that we should have
> 
> defargs.conf
> discovery.conf
> <cmdline>
> 
> Is that the case?

Effectively yes, but defargs does not allow transport,traddr,trsvcid,nqn
parameters (which don't make sense), and discovery.conf is designed to
hold exactly those. So in practice they should not overlap.

Also, we only go to discovery.conf if we did not get a transport+traddr
(which are mandatory). So its not exactly the order you mentioned, more
like:

defargs.conf
<cmdline>
if transport+traddr not given
discovery.conf

> And shouldn't we document that somewhere?
Where would be the place to update this?

[PATCH nvme-cli rfc] fabrics: support default connect/discover args

[Hannes Reinecke]

On 4/30/19 7:54 AM, Sagi Grimberg wrote:
> 
>> I do like the idea, but not necessarily the naming.
>> Can't we just call it 'default.conf' ?
> 
> It's not as self-explanatory, but I'm fine with default.conf...
> 
>> What about precedence?
>> I would have expected that we should have
>>
>> defargs.conf
>> discovery.conf
>> <cmdline>
>>
>> Is that the case?
> 
> Effectively yes, but defargs does not allow transport,traddr,trsvcid,nqn
> parameters (which don't make sense), and discovery.conf is designed to
> hold exactly those. So in practice they should not overlap.
> 
Uh-oh. I _do_ know of cases where discovery.conf is used for precisely 
this purpose, ie holding _all_ arguments.
And in the absense of any other way existing implementations will be 
using a very similar thing.

> Also, we only go to discovery.conf if we did not get a transport+traddr
> (which are mandatory). So its not exactly the order you mentioned, more
> like:
> 
> defargs.conf
> <cmdline>
> if transport+traddr not given
> discovery.conf
> 
How very curious ...
I would have expected that any command-line args would overwrite any 
arguments given in the configuration files.
But given that discovery.conf can contain several lines I can see the 
problem.

However, to clean things up we should be updating the parser to ignore 
or even call an error if we find a line in discovery.conf which does not 
specify transport+traddr.
Just to avoid confusion.

>> And shouldn't we document that somewhere?
> Where would be the place to update this?
manpage?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG N?rnberg)

[PATCH nvme-cli rfc] fabrics: support default connect/discover args

[Sagi Grimberg]

>> Effectively yes, but defargs does not allow transport,traddr,trsvcid,nqn
>> parameters (which don't make sense), and discovery.conf is designed to
>> hold exactly those. So in practice they should not overlap.
>>
> Uh-oh. I _do_ know of cases where discovery.conf is used for precisely 
> this purpose, ie holding _all_ arguments.

That's fine.. it was never designed for this purpose though...

> And in the absense of any other way existing implementations will be 
> using a very similar thing.

Again, this would work.

>> Also, we only go to discovery.conf if we did not get a transport+traddr
>> (which are mandatory). So its not exactly the order you mentioned, more
>> like:
>>
>> defargs.conf
>> <cmdline>
>> if transport+traddr not given
>> discovery.conf
>>
> How very curious ...
> I would have expected that any command-line args would overwrite any 
> arguments given in the configuration files.
> But given that discovery.conf can contain several lines I can see the 
> problem.

Exactly. discovery.conf was simply added such that you can run nvme
connect-all without explicitly passing parameters so you can build
auto-connect scripts on top of.

> However, to clean things up we should be updating the parser to ignore 
> or even call an error if we find a line in discovery.conf which does not 
> specify transport+traddr.
> Just to avoid confusion.

It will fail, the command will fail for lacking a mandatory parameter.

>>> And shouldn't we document that somewhere?
>> Where would be the place to update this?
> manpage?

Well discovery.conf is quite a generic name...
also don't you think a full blown man page is an overkill?

both files have some documentation in them, we could enhance those
a bit...

[PATCH nvme-cli rfc] fabrics: support default connect/discover args

[James Smart]

On 4/29/2019 3:53 PM, Sagi Grimberg wrote:
> Introduce /etc/nvme/defargs.conf where we allow the user to
> specify connect/discover parameters once and not for every
> controller. The cli will always ingest the content of this
> file before parsing cmdline args such that the user can
> override them.
>
> The format is simply writing the arguments into the file as
> if they were appended to the execution command.
>
> Also, properly install this file with some minimal documentation.
>
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
> This was raised before in the past that we don't have some place
> to store default connect args.
>
> For example, when handling automatic discovery change log events
> this can be a way for the user to set global default arguments.
>
> Feedback is welcome.
>
>   Makefile            |  3 +++
>   etc/defargs.conf.in |  5 ++++
>   fabrics.c           | 63 +++++++++++++++++++++++++++++++++++++++++++++
>   nvme.spec.in        |  1 +
>   4 files changed, 72 insertions(+)
>   create mode 100644 etc/defargs.conf.in
>
>
> diff --git a/etc/defargs.conf.in b/etc/defargs.conf.in
> new file mode 100644
> index 000000000000..e91106bf5bbf
> --- /dev/null
> +++ b/etc/defargs.conf.in
> @@ -0,0 +1,5 @@
> +# Used for extracting default controller connect parameters
> +#
> +# Example:
> +# --keep-alive-tmo=<x> --reconnect-delay=<y> --ctrl-loss-tmo=<z> --nr-io-queues=<u>
> +# --queue-size=<v>
>

What I'd like to see is a file? (maybe "connectargs.conf") that has a 
syntax more like:
<subnqn>? <trtype>? <traddr>? <trsvcid> <host-traddr>? :? <arg list>

where the first 5 things, all addressing components for the connect, can 
specify a matching policy, and if matched, "arg list" is then applied. 
An addressing component could be wildcarded, thus match anything.

When a connect or connect-all request is made,? the cli would try to 
match each line, and if matched, apply the arg list. If an argument was 
set prior, it would be overridden. Then, command line args can override 
the arg list built up by the match list.

Thus, for global defaults, the opening line in the file can be?? " * * * 
* * --keep-alive-tmo=<x> --reconnect-delay=<y>" and all connection 
attempts would receive those args.

We would probably do some special syntaxes over time:
Rather than the long string for a discovery controller have a shorthand 
string that means the same.
Devise a subnqn syntax that allows a <prefix>* - so that all nqns from 
vendor x that has the prefix could be applied, and so on.

-- james

[PATCH nvme-cli rfc] fabrics: support default connect/discover args

[James Smart]

On 5/1/2019 2:23 PM, James Smart wrote:
>
>
> On 4/29/2019 3:53 PM, Sagi Grimberg wrote:
>> Introduce /etc/nvme/defargs.conf where we allow the user to
>> specify connect/discover parameters once and not for every
>> controller. The cli will always ingest the content of this
>> file before parsing cmdline args such that the user can
>> override them.
>>
>> The format is simply writing the arguments into the file as
>> if they were appended to the execution command.
>>
>> Also, properly install this file with some minimal documentation.
>>
>> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
>> ---
>> This was raised before in the past that we don't have some place
>> to store default connect args.
>>
>> For example, when handling automatic discovery change log events
>> this can be a way for the user to set global default arguments.
>>
>> Feedback is welcome.
>>
>> ? Makefile??????????? |? 3 +++
>> ? etc/defargs.conf.in |? 5 ++++
>> ? fabrics.c?????????? | 63 +++++++++++++++++++++++++++++++++++++++++++++
>> ? nvme.spec.in??????? |? 1 +
>> ? 4 files changed, 72 insertions(+)
>> ? create mode 100644 etc/defargs.conf.in
>>
>>
>> diff --git a/etc/defargs.conf.in b/etc/defargs.conf.in
>> new file mode 100644
>> index 000000000000..e91106bf5bbf
>> --- /dev/null
>> +++ b/etc/defargs.conf.in
>> @@ -0,0 +1,5 @@
>> +# Used for extracting default controller connect parameters
>> +#
>> +# Example:
>> +# --keep-alive-tmo=<x> --reconnect-delay=<y> --ctrl-loss-tmo=<z> 
>> --nr-io-queues=<u>
>> +# --queue-size=<v>
>>
>
> What I'd like to see is a file? (maybe "connectargs.conf") that has a 
> syntax more like:
> <subnqn>? <trtype>? <traddr>? <trsvcid> <host-traddr>? :? <arg list>
>
> where the first 5 things, all addressing components for the connect, 
> can specify a matching policy, and if matched, "arg list" is then 
> applied. An addressing component could be wildcarded, thus match 
> anything.
>
> When a connect or connect-all request is made,? the cli would try to 
> match each line, and if matched, apply the arg list. If an argument 
> was set prior, it would be overridden. Then, command line args can 
> override the arg list built up by the match list.
>
> Thus, for global defaults, the opening line in the file can be?? " * * 
> * * * --keep-alive-tmo=<x> --reconnect-delay=<y>" and all connection 
> attempts would receive those args.
>
> We would probably do some special syntaxes over time:
> Rather than the long string for a discovery controller have a 
> shorthand string that means the same.
> Devise a subnqn syntax that allows a <prefix>* - so that all nqns from 
> vendor x that has the prefix could be applied, and so on.
>
> -- james
>

I do realize this would make us do a first pass through the cmd line 
args to get the addressing components.

-- james

[PATCH nvme-cli rfc] fabrics: support default connect/discover args

[James Smart]

On 4/30/2019 12:34 AM, Sagi Grimberg wrote:
>
>>> Effectively yes, but defargs does not allow 
>>> transport,traddr,trsvcid,nqn
>>> parameters (which don't make sense), and discovery.conf is designed to
>>> hold exactly those. So in practice they should not overlap.
>>>
>> Uh-oh. I _do_ know of cases where discovery.conf is used for 
>> precisely this purpose, ie holding _all_ arguments.
>
> That's fine.. it was never designed for this purpose though...
>
>> And in the absense of any other way existing implementations will be 
>> using a very similar thing.
>
> Again, this would work.
>
>>> Also, we only go to discovery.conf if we did not get a transport+traddr
>>> (which are mandatory). So its not exactly the order you mentioned, more
>>> like:
>>>
>>> defargs.conf
>>> <cmdline>
>>> if transport+traddr not given
>>> discovery.conf
>>>
>> How very curious ...
>> I would have expected that any command-line args would overwrite any 
>> arguments given in the configuration files.
>> But given that discovery.conf can contain several lines I can see the 
>> problem.
>
> Exactly. discovery.conf was simply added such that you can run nvme
> connect-all without explicitly passing parameters so you can build
> auto-connect scripts on top of.

with my suggestion it would be:
 ?? <parse cmdline for connect addressing args>
 ?? if transport+traddr not given:? pull them from discovery.conf note: 
if non-addressing args present, remember them
 ?? parse addressing args vs connectargs.conf? (aka defargs.conf)
 ? <apply non-addressing args from discovery.conf>
 ? <apply cmdline non-addressing args>

-- james

[PATCH nvme-cli rfc] fabrics: support default connect/discover args

[Arun Easi]

On Mon, 29 Apr 2019, 3:53pm, Sagi Grimberg wrote:

> Introduce /etc/nvme/defargs.conf where we allow the user to
> specify connect/discover parameters once and not for every
> controller. The cli will always ingest the content of this
> file before parsing cmdline args such that the user can
> override them.
> 
> The format is simply writing the arguments into the file as
> if they were appended to the execution command.
> 
> Also, properly install this file with some minimal documentation.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
> This was raised before in the past that we don't have some place
> to store default connect args.
> 
> For example, when handling automatic discovery change log events
> this can be a way for the user to set global default arguments.
> 
> Feedback is welcome.
> 
>  Makefile            |  3 +++
>  etc/defargs.conf.in |  5 ++++
>  fabrics.c           | 63 +++++++++++++++++++++++++++++++++++++++++++++
>  nvme.spec.in        |  1 +
>  4 files changed, 72 insertions(+)
>  create mode 100644 etc/defargs.conf.in
> 
> diff --git a/Makefile b/Makefile
> index 4bfbebbd156a..6f11941b5d3e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -105,6 +105,9 @@ install-etc:
>  	if [ ! -f $(DESTDIR)$(SBINDIR)/nvme/discovery.conf ]; then \
>  		$(INSTALL) -m 644 -T ./etc/discovery.conf.in $(DESTDIR)$(SYSCONFDIR)/nvme/discovery.conf; \
>  	fi
> +	if [ ! -f $(DESTDIR)$(SBINDIR)/nvme/defargs.conf ]; then \
> +		$(INSTALL) -m 644 -T ./etc/defargs.conf.in $(DESTDIR)$(SYSCONFDIR)/nvme/defargs.conf; \
> +	fi
>  
>  install-spec: install-bin install-man install-bash-completion install-zsh-completion install-etc
>  install: install-spec install-hostparams
> diff --git a/etc/defargs.conf.in b/etc/defargs.conf.in
> new file mode 100644
> index 000000000000..e91106bf5bbf
> --- /dev/null
> +++ b/etc/defargs.conf.in
> @@ -0,0 +1,5 @@
> +# Used for extracting default controller connect parameters
> +#
> +# Example:
> +# --keep-alive-tmo=<x> --reconnect-delay=<y> --ctrl-loss-tmo=<z> --nr-io-queues=<u>
> +# --queue-size=<v>
> diff --git a/fabrics.c b/fabrics.c
> index 511de06aec97..e609e679c832 100644
> --- a/fabrics.c
> +++ b/fabrics.c
> @@ -72,8 +72,10 @@ static struct config {
>  #define PATH_NVMF_DISC		"/etc/nvme/discovery.conf"
>  #define PATH_NVMF_HOSTNQN	"/etc/nvme/hostnqn"
>  #define PATH_NVMF_HOSTID	"/etc/nvme/hostid"
> +#define PATH_NVMF_DEFARGS	"/etc/nvme/defargs.conf"
>  #define SYS_NVME		"/sys/class/nvme"
>  #define MAX_DISC_ARGS		10
> +#define MAX_DEF_ARGS		10
>  #define MAX_DISC_RETRIES	10
>  
>  enum {
> @@ -894,6 +896,59 @@ static int do_discover(char *argstr, bool connect)
>  	return ret;
>  }
>  
> +static int nvmf_parse_defargs(const char *desc,
> +		const struct argconfig_commandline_options *opts)
> +{
> +	FILE *f;
> +	char line[256], *ptr, *args, **argv;
> +	int argc = 0, ret = 0;
> +
> +	f = fopen(PATH_NVMF_DEFARGS, "r");
> +	if (f == NULL)
> +		return 0;
> +
> +	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");
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +
> +		argv = calloc(MAX_DEF_ARGS, BUF_SIZE);
> +		if (!argv) {
> +			fprintf(stderr, "failed to allocate argv vector\n");
> +			free(args);
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +
> +		argc = 0;
> +		argv[argc++] = "dummy";
> +		while ((ptr = strsep(&args, " =\n")) != NULL)
> +			argv[argc++] = ptr;
> +
> +		ret = argconfig_parse(argc, argv, desc, opts, &cfg, sizeof(cfg));
> +		free(args);
> +		free(argv);
> +		if (ret)
> +			goto out;
> +
> +		if (cfg.transport || cfg.traddr || cfg.trsvcid || cfg.nqn) {
> +			fprintf(stderr, "args transport, traddr, trsvcid, nqn "
> +				"cannot have a default\n");
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +	}
> +out:
> +	fclose(f);
> +	return ret;
> +}
> +
>  static int discover_from_conf_file(const char *desc, char *argstr,
>  		const struct argconfig_commandline_options *opts, bool connect)
>  {
> @@ -981,6 +1036,10 @@ int discover(const char *desc, int argc, char **argv, bool connect)
>  		{NULL},
>  	};
>  
> +	ret = nvmf_parse_defargs(desc, command_line_options);
> +	if (ret)
> +		return ret;
> +
>  	ret = argconfig_parse(argc, argv, desc, command_line_options, &cfg,
>  			sizeof(cfg));
>  	if (ret)
> @@ -1026,6 +1085,10 @@ int connect(const char *desc, int argc, char **argv)
>  		{NULL},
>  	};
>  
> +	ret = nvmf_parse_defargs(desc, command_line_options);
> +	if (ret)
> +		return ret;
> +
>  	ret = argconfig_parse(argc, argv, desc, command_line_options, &cfg,
>  			sizeof(cfg));
>  	if (ret)
> diff --git a/nvme.spec.in b/nvme.spec.in
> index 6934f8fd605b..e2240b61d79e 100644
> --- a/nvme.spec.in
> +++ b/nvme.spec.in
> @@ -35,6 +35,7 @@ make install-spec DESTDIR=%{buildroot} PREFIX=/usr
>  %{_sysconfdir}/nvme/hostnqn
>  %{_sysconfdir}/nvme/hostid
>  %{_sysconfdir}/nvme/discovery.conf
> +%{_sysconfdir}/nvme/defargs.conf
>  
>  %clean
>  rm -rf $RPM_BUILD_ROOT
> 

Does it make sense to have:

- A message printed when arguments are picked up from *.conf, perhaps not 
  always but when, say, a --verbose/-v is supplied to make it clear to
  the user.

- An option like, "--no-conf=<discovery/defargs/all>" so that none of the
  options/values are read from those files. Perhaps useful in scripting
  environments where finer controls are desired; thus avoiding a 
  default argument addition later by an admin to *.conf affecting
  script driven invocations.

Regards,
-Arun