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

[tristan]

Joel,

I trust you a lot on the naming of options&short-options:-)


Joel Becker wrote:
> 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.
>
>   

So we made the deal, use -V here.

>> +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"?
>   

'-F' is ok for me.

>   
>> +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.
>   

How about '-B' for superBlock or '-D' for dump superblock or '-O' for 
output of volinfo?

It really makes the naming of this one a little bit tough if we aren't 
going to use '-V', '-S' or '-I' :)

>   
>> +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.
>   

Good catch, '--usage' was somewhat misleading to '--help', 
'--space-usage' could be much more straightforward.


>   
>> +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.
>   

Thanks for catching this.

>   
>> +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
>   

Joel,

You suggestion may make the usage of o2info look more straightforward 
and easy for end-users, It however will be losing flexibility somehow, 
How about the case when each operation needs a specific argument, such 
as, 'freefrag' need us to specify a chunksize. Besides, it also keeps an 
unification in favor to tunefs.ocfs2 which also uses long-options for 
operations. Even what's more, we also could be able to do combined 
operations by following:

'o2info --volinfo --fs-features /dev/sdx1', and we will not be forcing 
users to place device or filename at the end of cmd, following usage 
also is welcome:

'o2info /file/in/ocfs2/volume --volinfo --freefrage 32' OR

'o2info --volinfo /dev/sdb1 --fs-features'


How do you think about it?


Regards,
Tristan.


> device.
>
> Joel
>
>