[Ocfs2-tools-devel] [PATCH 08/11] Ocfs2-tools: Add main.c to o2info.

[Joel Becker]

On Fri, Jan 08, 2010 at 04:50:58PM +0800, Tristan Ye wrote:
> +static struct o2info_option version_option = {
> +	.opt_option	= {
> +		.name		= "version",
> +		.val		= 'v',
> +		.has_arg 	= 0,
> +		.flag		= NULL,
> +	},
> +	.opt_help	= NULL,
> +	.opt_handler	= version_handler,
> +	.opt_op		= NULL,
> +	.opt_private = NULL,
> +};

	Oh, no, do not use 'v' as the short option for "--version".
'-v' usually means "--verbose", including in other ocfs2 tools.  We use
'-V' for "--version" elsewhere.  Of course, that means '-V' cannot be
"--volinfo".  More below.

> +static struct o2info_option coherency_option = {
> +	.opt_option	= {
> +		.name		= "no-coherency",
> +		.val		= 'c',
> +		.has_arg 	= 0,
> +		.flag		= NULL,
> +	},
> +	.opt_help	=
> +		"-c|--no-coherency",
> +	.opt_handler	= coherency_handler,
> +	.opt_op		= NULL,
> +	.opt_private = NULL,
> +};

	I don't like '-c'.  It would imply "coherency", not "_no_
coherency".  '-n' doesn't work, because it ususally means "--dry-run".
How about '-F', which is our usual idiom for "ignore the cluster"?

> +static struct o2info_option volinfo_option = {
> +	.opt_option	= {
> +		.name		= "volinfo",
> +		.val		= 'V',
> +		.has_arg	= 0,
> +		.flag		= NULL,
> +	},
> +	.opt_help	=
> +		"-V|--volinfo",
> +	.opt_handler	= NULL,
> +	.opt_op		= &volinfo_op,
> +	.opt_private	= NULL,
> +};

	As I said above, '-V' is '--version'.  I'd think '-I' for
"info" or '-S' for "superblock info", but both are taken below.  We need
to think about a good thing here.

> +static struct o2info_option usage_option = {
> +	.opt_option	= {
> +		.name		= "usage",
> +		.val		= 'U',
> +		.has_arg	= 0,
> +		.flag		= NULL,
> +	},
> +	.opt_help	= "-U|--usage",
> +	.opt_handler	= NULL,
> +	.opt_op		= &usage_op,
> +};

	"--usage" made me think "--help" at first.  How about
"--space-usage".  '-U' is a fine short option.

> +static struct o2info_option filestat_option = {
> +	.opt_option	= {
> +		.name		= "filestat",
> +		.val		= 'S',
> +		.has_arg	= 0,
> +		.flag		= NULL,
> +	},
> +	.opt_help	= "-S|--filestat",
> +	.opt_handler	= NULL,
> +	.opt_op		= &filestat_op,
> +};

	"--file-stats"?  "--file-statistics"?  Long options should not
be cryptic.

> +static struct o2info_option freefrag_option = {
> +	.opt_option	= {
> +		.name		= "freefrag",
> +		.val		= 'F',
> +		.has_arg	= 1,
> +		.flag		= NULL,
> +	},
> +	.opt_help	= "-F|--freefrag <chunksize in KB>",
> +	.opt_handler	= NULL,
> +	.opt_op		= &freefrag_op,
> +};

	Argh!  I considered using '-F' above.
	I think we're going to have to give up on option names for this.
They aren't options - they are _operations_.  Forget getopt for these.
Instead, the usage should be 'o2info [options] <operation> [..
<operation>] <device>'.  eg, "o2info -F volinfo freefrag freeinodes
/dev/sdb1".  You only use getopt to gather actual options.  Things like
"no coherencye".  Then at the end of the option list, you walk the
remaining arguments for operation names.  The last argument is the
device.

Joel

-- 

"Nearly all men can stand adversity, but if you really want to
 test a man's character, give him power."
	- Abraham Lincoln

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127