[PATCH 0/3] nvme-cli: fabric doc and tool fixes.

[PATCH 0/3] nvme-cli: fabric doc and tool fixes.

[Jay Freyensee]

This patchset tweaks discover/connect-all man pages according
to a previous thread, then last patch fixes the tool according
to the correct examples shown in the man pages.

Jay Freyensee (3):
  nvme-cli: follow-on discovery tweaks from thread
  nvme-cli: follow-on doc tweaks to nvme-connect-all
  nvme-cli: fix nvme-connect-all using hostnqn

 Documentation/nvme-connect-all.txt | 15 ++++++++-------
 Documentation/nvme-discover.txt    | 27 ++++++++++++++-------------
 fabrics.c                          |  6 ++++++
 3 files changed, 28 insertions(+), 20 deletions(-)

-- 
2.5.5

[PATCH 1/3] nvme-cli: follow-on discovery tweaks from thread

[Jay Freyensee]

Follow-on doc patch tweaks from discovery documentation discussion.

Signed-off-by: Jay Freyensee <james_p_freyensee at linux.intel.com>
---
 Documentation/nvme-discover.txt | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/Documentation/nvme-discover.txt b/Documentation/nvme-discover.txt
index d43f088..e9f04b2 100644
--- a/Documentation/nvme-discover.txt
+++ b/Documentation/nvme-discover.txt
@@ -3,7 +3,7 @@ nvme-discover(1)
 
 NAME
 ----
-nvme-discover - Send Discovery requests to Fabrics Discovery Controllers.
+nvme-discover - Send Get Log Page request to Discovery Controller.
 
 SYNOPSIS
 --------
@@ -17,7 +17,7 @@ SYNOPSIS
 
 DESCRIPTION
 -----------
-Send one or more Discovery requests to a NVMe over Fabrics Discovery
+Send one or more Get Log Page requests to a NVMe-over-Fabrics Discovery
 Controller.
 
 If no parameters are given, then 'nvme discover' will attempt to 
@@ -25,9 +25,9 @@ find a /etc/nvme/discovery.conf file to use to supply a list of
 Discovery commands to run.  If no /etc/nvme/discovery.conf file
 exists, the command will quit with an error.
 
-Otherwise a specific Discovery Controller should be specified using the
---transport, --traddr and if nessecary the --trsvcid and a Di?covery
-request will be sent to the specified Discovery Controller.
+Otherwise, a specific Discovery Controller should be specified using the
+--transport, --traddr, and if necessary the --trsvcid flags. A Di?covery
+request will then be sent to the specified Discovery Controller.
 
 BACKGROUND
 ----------
@@ -44,6 +44,9 @@ contained in that NVMe subsystem on the NVMe Target.
 Note that the base NVMe specfication defines the NQN (NVMe Qualified
 Name) format which an NVMe endpoint (device, subsystem, etc) must
 follow to guarantee a unique name under the NVMe standard.
+In particular, the Host NQN uniquely identifies the NVMe Host, and
+may be used by the the Discovery Controller to control what NVMe Target
+resources are allocated to the NVMe Host for a connection.
 
 A Discovery Controller has it's own NQN defined in the NVMe-over-Fabrics
 specification, *nqn.2014-08.org.nvmexpress.discovery*.  All Discovery
@@ -68,8 +71,8 @@ OPTIONS
 -a <traddr>::
 --traddr=<traddr>::
 	This field specifies the network address of the Discovery Controller.
-	For transports using IP addressing (e.g. rdma) this should be an IPv4
-	address.
+	For transports using IP addressing (e.g. rdma) this should be an
+	IP-based (ex. IPv4) address.
 
 -s <trsvcid>::
 --trsvcid=<trsvcid>::
@@ -79,12 +82,10 @@ OPTIONS
  
 -q <hostnqn>::
 --hostnqn=<hostnqn>::
-	Overrides the default host NQN that identifies the NVMe Host.  If this
-	option is not specified the default is read from /etc/nvme/hostnqn or
-	autogenerated by the kernel (in that order).
-	The Host NQN uniquely identifies the NVMe Host, and may be used by the
-	the Discovery Controller to control what NVMe Target resources are
-	allocated to the NVMe Host for a connection.
+	Overrides the default host NQN that identifies the NVMe Host.  
+	If this option is not specified, the default is read from
+	/etc/nvme/hostnqn first. If that does not exist, the autogenerated
+	NQN value from the NVMe Host kernel module is used next.
 
 -r <filename>::
 --raw=<filename>::
-- 
2.5.5

[PATCH 1/3] nvme-cli: follow-on discovery tweaks from thread

[Christoph Hellwig]

Hi Jay,

I'm not a huge fan with the explicit mention of the Get Log Page
command for th Discovery, but given that no one but us two seems to
care I'm not going to block the patch over it.

The rest of this patch and the connect-all version of it looks fine to
me:

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

[PATCH 2/3] nvme-cli: follow-on doc tweaks to nvme-connect-all

[Jay Freyensee]

Minor massaging and typo fixes.

Signed-off-by: Jay Freyensee <james_p_freyensee at linux.intel.com>
---
 Documentation/nvme-connect-all.txt | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/nvme-connect-all.txt b/Documentation/nvme-connect-all.txt
index f07ef93..1dc1782 100644
--- a/Documentation/nvme-connect-all.txt
+++ b/Documentation/nvme-connect-all.txt
@@ -22,11 +22,11 @@ Controller, and create controllers for the returned discovery records.
 
 If no parameters are given, then 'nvme connect-all' will attempt to
 find a /etc/nvme/discovery.conf file to use to supply a list of
-Connect-all commands to run. If no /etc/nvme/discovery.conf file exists,
+connect-all commands to run. If no /etc/nvme/discovery.conf file exists,
 the command will quit with an error.
 
 Otherwise a specific Discovery Controller should be specified using the
---transport, --traddr and if nessecary the --trsvcid and a Di?covery
+--transport, --traddr and if necessary the --trsvcid and a Di?covery
 request will be sent to the specified Discovery Controller.
 
 See the documentation for the nvme-discover(1) command for further
@@ -50,8 +50,8 @@ OPTIONS
 -a <traddr>::
 --traddr=<traddr>::
 	This field specifies the network address of the Discovery Controller.
-	For transports using IP addressing (e.g. rdma) this should be an IPv4
-	address.
+	For transports using IP addressing (e.g. rdma) this should be an
+	IP-based address (ex. IPv4).
 
 -s <trsvcid>::
 --trsvcid=<trsvcid>::
@@ -61,9 +61,10 @@ OPTIONS
  
 -q <hostnqn>::
 --hostnqn=<hostnqn>::
-	Overrides the default host NQN that identifies the NVMe Host.  If this
-	option is not specified the default is read from /etc/nvme/hostnqn or
-	autogenerated by the kernel (in that order).
+	Overrides the default Host NQN that identifies the NVMe Host.
+	If this option is not specified, the default is read from
+	/etc/nvme/hostnqn first. If that does not exist, the autogenerated
+	NQN value from the NVMe Host kernel module is used next.
 	The Host NQN uniquely identifies the NVMe Host, and may be used by the
 	the Discovery Controller to control what NVMe Target resources are
 	allocated to the NVMe Host for a connection.
-- 
2.5.5

[PATCH 3/3] nvme-cli: fix nvme-connect-all using hostnqn

[Jay Freyensee]

The example in the man pages:

nvme connect-all --transport=rdma --traddr=192.168.1.3 \
--hostnqn=host1-rogue-nqn

fails because nvme-cli fails to actually use hostnqn upon
connect.  This patch fixes that.

Signed-off-by: Jay Freyensee <james_p_freyensee at linux.intel.com>
---
 fabrics.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fabrics.c b/fabrics.c
index 51e424e..801fe4f 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -540,6 +540,12 @@ static int connect_ctrl(struct nvmf_disc_rsp_page_entry *e)
 		return -EINVAL;
 	p += len;
 
+	if (cfg.hostnqn)
+	len = sprintf(p, ",hostnqn=%s", cfg.hostnqn);
+	if (len < 0)
+		return -EINVAL;
+	p += len;
+
 	switch (e->trtype) {
 	case NVMF_TRTYPE_LOOP: /* loop */
 		len = sprintf(p, ",transport=loop");
-- 
2.5.5

[PATCH 3/3] nvme-cli: fix nvme-connect-all using hostnqn

[Christoph Hellwig]

On Wed, Oct 19, 2016@12:43:07PM -0700, Jay Freyensee wrote:
> The example in the man pages:
> 
> nvme connect-all --transport=rdma --traddr=192.168.1.3 \
> --hostnqn=host1-rogue-nqn
> 
> fails because nvme-cli fails to actually use hostnqn upon
> connect.  This patch fixes that.
> 
> Signed-off-by: Jay Freyensee <james_p_freyensee at linux.intel.com>
> ---
>  fabrics.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fabrics.c b/fabrics.c
> index 51e424e..801fe4f 100644
> --- a/fabrics.c
> +++ b/fabrics.c
> @@ -540,6 +540,12 @@ static int connect_ctrl(struct nvmf_disc_rsp_page_entry *e)
>  		return -EINVAL;
>  	p += len;
>  
> +	if (cfg.hostnqn)
> +	len = sprintf(p, ",hostnqn=%s", cfg.hostnqn);
> +	if (len < 0)
> +		return -EINVAL;
> +	p += len;

The indentation here looks odd, I'd expect the line conditional
on the if to be indented.  Also while the code should work fine as-is
I'd move the len check and increment into the conditional, as they are
pointless without doing the sprintf.

[PATCH 3/3] nvme-cli: fix nvme-connect-all using hostnqn

[J Freyensee]

On Fri, 2016-10-21@06:22 -0700, Christoph Hellwig wrote:
> On Wed, Oct 19, 2016@12:43:07PM -0700, Jay Freyensee wrote:
> > 
> > The example in the man pages:
> > 
> > nvme connect-all --transport=rdma --traddr=192.168.1.3 \
> > --hostnqn=host1-rogue-nqn
> > 
> > fails because nvme-cli fails to actually use hostnqn upon
> > connect.??This patch fixes that.
> > 
> > Signed-off-by: Jay Freyensee <james_p_freyensee at linux.intel.com>
> > ---
> > ?fabrics.c | 6 ++++++
> > ?1 file changed, 6 insertions(+)
> > 
> > diff --git a/fabrics.c b/fabrics.c
> > index 51e424e..801fe4f 100644
> > --- a/fabrics.c
> > +++ b/fabrics.c
> > @@ -540,6 +540,12 @@ static int connect_ctrl(struct
> > nvmf_disc_rsp_page_entry *e)
> > ?		return -EINVAL;
> > ?	p += len;
> > ?
> > +	if (cfg.hostnqn)
> > +	len = sprintf(p, ",hostnqn=%s", cfg.hostnqn);
> > +	if (len < 0)
> > +		return -EINVAL;
> > +	p += len;
> 
> The indentation here looks odd, I'd expect the line conditional
> on the if to be indented.??

Yes, I didn't do that indentation right, I'll adjust the code block per
comments.

Jay

> Also while the code should work fine as-is
> I'd move the len check and increment into the conditional, as they
> are
> pointless without doing the sprintf.

> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme