Skip to content

Conversation

@sc108-lee
Copy link
Contributor

already defined as static const char variable

already defined as static const char variable

Signed-off-by: Steven Seungcheol Lee <sc108.lee@samsung.com>
@igaw
Copy link
Collaborator

igaw commented Feb 10, 2026

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
Copy link
Contributor Author

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
Copy link
Collaborator

igaw commented Feb 11, 2026

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!).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants