Re: [PATCH v2 2/7] nvme: consolidate some nvme-cli utility functions

[Sagi Grimberg <sagi@grimberg.me>]

On 8/6/20 7:53 PM, Chaitanya Kulkarni wrote:
> On 8/6/20 12:15, Sagi Grimberg wrote:
>>    
>> @@ -97,6 +97,33 @@ _setup_nvmet() {
>>    	modprobe nvme-loop
>>    }
>>    
>> +_nvme_disconnect_ctrl() {
>> +	local ctrl="$1"
>> +
>> +	nvme disconnect -d ${ctrl}
>> +}
>> +
>> +_nvme_disconnect_subsys() {
>> +	local subsysnqn="$1"
>> +
>> +	nvme disconnect -n ${subsysnqn}
>> +}
>> +
>> +_nvme_connect_subsys() {
>> +	local trtype="$1"
>> +	local subsysnqn="$2"
>> +
>> +	cmd="nvme connect -t ${trtype} -n ${subsysnqn}"
>> +	eval $cmd
>> +}
>> +
>> +_nvme_discover() {
>> +	local trtype="$1"
>> +
>> +	cmd="nvme discover -t ${trtype}"
>> +	eval $cmd
>> +}
>> +
>>    _create_nvmet_port() {
>>    	local trtype="$1"
>>    
>> @@ -206,6 +233,6 @@ _filter_discovery() {
>>    }
>>    
>>    _discovery_genctr() {
>> -	nvme discover -t loop |
>> +	_nvme_discover "loop" |
>>    		sed -n -e 's/^.*Generation counter \([0-9]\+\).*$/\1/p'
>>    }
>> -- 2.25.1
> 
> I'm okay with having a wrapper for disconnect but for connect and
> discover command it can have many arguments having a call in the
> test-case might loose the readability.

That's why these has default values.

> The downside is it will need argument count handling in the future
> and makes things not easier when user want to skip certain
> parameters, closest example would be _create_nvmet_ns().
> 
> Also if we are adding wrappers why not move $FULL 2>&1 to avoid
> duplication ?

Not exactly sure what you meant