[PATCH 2/3] fine tune the nvme-discover manpage

[Christoph Hellwig]

On Wed, Oct 12, 2016 at 10:18:05AM -0700, J Freyensee wrote:
> > -nvme-discover - Send Get Log Page request to Discovery Controller.
> > +nvme-discover - Send Discovery requests to Fabrics Discovery
> > Controllers.
> 
> I preferred mentioning "Get Log Page" because that is really what is
> happening and reflects the NVMe specification, which I was aiming to
> push the envelope a bit and help educate and accelerate the learning
> curve for people new to these features (map more accurately what is
> being described here to the NVMe specification(s).

While Get Log Page is technicaly correct I don't really think it
serves the user much - it's the way the discovery is implemented,
the important part is that we perform discovery.

> >  SYNOPSIS
> >  --------
> >  [verse]
> > -'nvme discover'	[device]
> > +'nvme discover'
> 
> It was there because nvme-cli works regardless if a device parameter is
> there or not (such as /dev/nvme-fabrics/).  Granted it seems a bit
> pointless to include it if the same correct behavior works without it,
> but it made it common with the rest of the nvme-cli command man pages.

Well, the point is that we hardcode the /dev/nvme-fabrics path in
fabrics.c.  So while nvme-cli accept as a device argument it is ignored.

We'll need to fix the option parse to properly reject it, I think.

> This is unnecessary to be mentioning the flags here when they have
> already been mentioned and shown in "SYNOPSIS" as well as described in
> detail below with examples provided after that.  This isn't adding much
> additional value and making this slightly harder to maintain if flags
> are added/adjusted.
> 
> (necessary is misspelled btw).

The important addition I wanted is something to balance the description
of what happens if no arguments are specified. 

That being said I really, really hate the having the read config file
behavior the default without option.  I already complained about it
when it went in, and having to read the manpage it's even more confusing.

I'd much prefer making it a --config option that points to a config
file, as the prime user of it is the system startup script anyway.

> >  |=================
> >  
> >  -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.
> 
> We shouldn't be mentioning that this can be specifically a IPv4 address
> when there has already been work to support an IPv6 address (for
> example) and then this will have to be tweaked again.  I think the
> explanation was fine the way it is and should be explained generically
> that leaves out specific mentions of IPv4 (plus the examples provided
> afterword show using an IPv4 address).

I was tempted to add IPv6, but given that it's not supported I skipped it.
We'll need some guidance on what the valid address format are.  I'm not
sure what the best way is, but I don't think example are sufficient.

Maybe a table in a separate section or even man page.

> Good start, but I can see an interpretation that this can make it sound
> like the kernel will autogenerate hostnqn if this parameter is not
> supplied, which would be incorrect because it's already generated upon
> NVMe Host module load to be used as a 'catch-all default'.  I think a
> further tweak to this could be:

True.

> 
> "If this option is not specified, the default is read from
> /etc/nvme/hostnqn first and if that does not exist, the autogenerated
> NQN value from the NVMe Host kernel module is used next."

Sounds fine.

> > +	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.
> 
> This sounds like it could go in the BACKGROUND section I wrote, which I
> admit I didn't not state this as clearly as here, but then again it's
> probably ok to keep this blurb here to re-emphasize this important
> feature of NVMe devices.

I'm fine with moving this around.