nvme: Remove duplicated variables

[sc108-lee]

already defined as static const char variable

[igaw]

Hmm, it is a bit odd that we have some of the variables as globals and some not. Can't we come up with something more sane in the long run. Any ideas?

[sc108-lee]

I agree.
Also I think we need manage short-option for ARGS as well.
same short option for same argument.

To avoid confilct, do we need manage all argments set in global?

rough idea
ARG_ADD_NSID = OPT_UINT("namespace-id", 'n', namespace_id, namespace)
ARG_ADD_RAW_BINARY = OPT_FLAG("raw-binary", 'b', raw_binary, raw_output)

use
NVME_ARGS(opts,
ARG_ADD_NSID,
ARG_ADD_RAW_BINARY);

[igaw]

The commands in nvme.c do not have a common pattern for the arguments similar to fabrics.c as far I can tell.

While I like to reduce duplication, I think here it would be contra productive as it introduces even more abstractions from what is happening. E.g. the cfg is command specific and the commands is using it later.

	struct config {
		char	*file_name;
		__u32	host_gen;
		bool	ctrl_init;
		int	data_area;
		bool	rae;
		__u8	mcda;
	};
	struct config cfg = {
		.file_name	= NULL,
		.host_gen	= 1,
		.ctrl_init	= false,
		.data_area	= 3,
		.rae		= false,
		.mcda		= 0xff,
	};

	NVME_ARGS(opts,
		  OPT_FILE("output-file",     'O', &cfg.file_name, fname),
		  OPT_UINT("host-generate",   'g', &cfg.host_gen,  hgen),
		  OPT_FLAG("controller-init", 'c', &cfg.ctrl_init, cgen),
		  OPT_UINT("data-area",       'd', &cfg.data_area, dgen),
		  OPT_FLAG("rae",             'r', &cfg.rae,       rae),
		  OPT_BYTE("mcda",            'm', &cfg.mcda,      mcda));


	err = parse_and_open(&ctx, &hdl, argc, argv, desc, opts);
	if (err)
		return err;

	err = validate_output_format(nvme_cfg.output_format, &flags);
	if (err < 0) {
		nvme_show_error("Invalid output format");
		return err;
	}

I think for any casual reader, it's pretty clear what's happening and it's easy to extend. Having more abstraction is making this more difficult. The main objective is always making the code maintainable in the log run.

Anyway, the only thing which is annoying is the help string. So a bunch of function use the global defined strings and some have local defined strings. Maybe we should just move all those strings to the global section?

If we do this, then I would suggest to introduce a common prefix for these variables, so it's also clear what they are.
fname -> desc_fname or so (naming is hard!).

[igaw]

I've done some cleanups in the command parser area. It should be a bit more consistent. So I think it's time to look at the help text strings. So the command itself use desc. If we follow the pattern we should call those variables desc_$foo.

e.g. these here would be prefixed with desc_ and moved out of the function context.

	const char *fname = "File name to save raw binary, includes header";
	const char *hgen = "Have the host tell the controller to generate the report";
	const char *cgen = "Gather report generated by the controller.";
	const char *dgen = "Pick which telemetry data area to report. Default is 3 to fetch areas 1-3. Valid options are 1, 2, 3, 4.";
	const char *mcda = "Host-init Maximum Created Data Area. Valid options are 0 ~ 4 "
		"If given, This option will override dgen. 0 : controller determines data area";

WDYT?