From mboxrd@z Thu Jan 1 00:00:00 1970 From: hch@lst.de (Christoph Hellwig) Date: Thu, 13 Oct 2016 12:26:32 +0200 Subject: [PATCH 2/3] fine tune the nvme-discover manpage In-Reply-To: <1476292685.5410.67.camel@linux.intel.com> References: <1476261953-23119-1-git-send-email-hch@lst.de> <1476261953-23119-3-git-send-email-hch@lst.de> <1476292685.5410.67.camel@linux.intel.com> Message-ID: <20161013102632.GA14543@lst.de> On Wed, Oct 12, 2016@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=:: > > ? 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.