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

[J Freyensee]

On Mon, 2016-08-08 at 15:35 +0200, Christoph Hellwig wrote:
> On Mon, Aug 08, 2016 at 02:57:59PM +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.
> 
> Hmm, can just call this /etc/nvme/discovery.conf or something else
> that rolls easier off the hand?

Yah, similar to my last nitpick comment, I like this a tad better
because we don't even have to mention nvmf/nvmeof.

I'd still like to see example documentation in the header patch of what
the resulting .conf file contents could look like.

> 
> > 
> > +static int discover_from_conf_file(const char *desc, char *argstr,
> > +	const struct argconfig_commandline_options *opts, bool
> > connect)
> 
> Second tab for the argument indent please.
> 
> > 
> > +		err = build_options(argstr, BUF_SIZE);
> > +		if (err) {
> > +			ret = err;
> > +			continue;
> > +		}
> > +
> > +		err = do_discover(argstr, connect);
> > +		if (err) {
> > +			ret = err;
> > +			continue;
> > +		}
> 
> How will diagnostics look like here if the file has incorrect
> syntax?

Dido.  Be nice to know what argstr/parameter broke.


> 
> > 
> > +	if (!cfg.transport && !cfg.traddr) {
> > +		return discover_from_conf_file(desc, argstr,
> > +				command_line_options, connect);
> 
> Maybe we want a separate options that says the option needs to be
> read from a config file, e.g. --file with an optional argument
> for the file name?

Are you thinking that there could be a single system that is acting
like multiple hosts, therefore each "host/hostnqn" could use a separate
.conf file for connection? 

If that is the case, then it may be a little confusing if there was a
single /etc/nvme/hostnqn filename that existed.