Re: [PATCH v2] blkzone: add open/close/finish commands

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On 2020/03/25 20:36, Karel Zak wrote:
> On Tue, Mar 24, 2020 at 07:02:17PM +0900, Damien Le Moal wrote:
>>  sys-utils/blkzone.8 | 27 +++++++++++++++--
>>  sys-utils/blkzone.c | 74 ++++++++++++++++++++++++++++++++++++++++-----
>>  2 files changed, 91 insertions(+), 10 deletions(-)
> 
> Applied, thanks.
> 
>>  static const struct blkzone_command commands[] = {
>>  	{ "report",	blkzone_report, N_("Report zone information about the given device") },
>> -	{ "reset",	blkzone_reset,  N_("Reset a range of zones.") }
>> +	{ "reset",	blkzone_action, N_("Reset a range of zones.") },
>> +	{ "open",	blkzone_action, N_("Open a range of zones.") },
>> +	{ "close",	blkzone_action, N_("Close a range of zones.") },
>> +	{ "finish",	blkzone_action, N_("Set a range of zones to Full.") }
>> +};
>> +
>> +/*
>> + * The action values must match the command index in the command array.
>> + */
>> +enum blkzone_action {
>> +	BLK_ZONE_NO_ACTION = 0,
>> +	BLK_ZONE_RESET,
>> +	BLK_ZONE_OPEN,
>> +	BLK_ZONE_CLOSE,
>> +	BLK_ZONE_FINISH,
>>  };
> 
> If you add ioctl_cmd and ioctl_name to the struct blkzone_command,
> and you define commands[] as:
> 
>  commands[] = {
>     { 
>         .name = "report",
>         .handler = blkzone_report,
>         .help = N_("Report zone information about the given device") 
>     },{
>         .name = "reset",
>         .handler = blkzone_action,
>         .help = N_("Reset a range of zones."),
>         .ioctl_cmd = BLKRESETZONE,
>         .ioctl_name = "BLKRESETZONE"
>     },
>     ...
>  };
> 
> than you do not need this switch() and command_action()
> 
>>  
>> +	switch (command_action(ctl->command)) {
>> +	case BLK_ZONE_RESET:
>> +		ioctl_cmd = BLKRESETZONE;
>> +		ioctl_name = "BLKRESETZONE";
>> +		break;
>> +	case BLK_ZONE_OPEN:
>> +		ioctl_cmd = BLKOPENZONE;
>> +		ioctl_name = "BLKOPENZONE";
>> +		break;
>> +	case BLK_ZONE_CLOSE:
>> +		ioctl_cmd = BLKCLOSEZONE;
>> +		ioctl_name = "BLKCLOSEZONE";
>> +		break;
>> +	case BLK_ZONE_FINISH:
>> +		ioctl_cmd = BLKFINISHZONE;
>> +		ioctl_name = "BLKFINISHZONE";
>> +		break;
>> +	case BLK_ZONE_NO_ACTION:
>> +		/* fallthrough */
>> +	default:
>> +		errx(EXIT_FAILURE, _("Invalid zone action"));
>   
> but you can use ctl->command.ioclt_cmd  etc.
> 
> Just idea :-)

I just sent a V3 of the full patch with this cleanup.
Did you prefer an incremental change ?
(I did not see that you said you applied v2 above... Thanks !)


> 
>  Karel
> 


-- 
Damien Le Moal
Western Digital Research