Re: [PATCH] check for valid parameters when passing tgtadm commands

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



FUJITA Tomonori wrote:
> On Wed, 08 Oct 2008 08:45:32 +0200
> Doron Shoham <dorons@xxxxxxxxxxxx> wrote:
> 
>> FUJITA Tomonori wrote:
>>> On Tue, 07 Oct 2008 18:01:31 +0200
>>> Doron Shoham <dorons@xxxxxxxxxxxx> wrote:
>>>
>>>> check for valid parameters when passing tgtadm commands
>>>>
>>>> Signed-off-by: Doron Shoham <dorons@xxxxxxxxxxxx>
>>>> ---
>>>>  usr/tgtadm.c |   65 +++++++++++++++++++++++++++++++++++++++++-----------------
>>>>  1 files changed, 46 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/usr/tgtadm.c b/usr/tgtadm.c
>>>> index 9e90b40..21bb245 100644
>>>> --- a/usr/tgtadm.c
>>>> +++ b/usr/tgtadm.c
>>>> @@ -484,35 +484,38 @@ int main(int argc, char **argv)
>>>>  /* 		exit(EINVAL); */
>>>>  	}
>>>>  
>>>> -	if ((name || value) && op != OP_UPDATE) {
>>>> -		eprintf("only 'update' operation takes"
>>>> -			" 'name' and 'value' options\n");
>>>> -		exit(EINVAL);
>>>> -	}
>>> Hm, I think that this is a valid checking. Why did you remove it?
>> My idea was to check that every option
>> receives its own valid parameters.
> 
> But if you do, the code would have tons of duplications:
> 
> 
> case OP_SHOW:
>      if (name || value)
>      	eprintf("only 'update' operation takes"
> 		" 'name' and 'value' options\n");
> 	exit(EINVAL);
> 	break;
> case OP_DELETE:
>      if (name || value)
>      	eprintf("only 'update' operation takes"
> 		" 'name' and 'value' options\n");
> 	exit(EINVAL);
> 	break;
> case OP_BIND:
>      if (name || value)
>      	eprintf("only 'update' operation takes"
> 		" 'name' and 'value' options\n");
> 	exit(EINVAL);
> 	break;
> case OP_UNBIND:
>      if (name || value)
>      	eprintf("only 'update' operation takes"
> 		" 'name' and 'value' options\n");
> 	exit(EINVAL);
> 	break;
> case OP_UPDATE:
> 
> 
> Or
> 
> void check_name_value(void)
> {
>      if (name || value)
>      	eprintf("only 'update' operation takes"
> 		" 'name' and 'value' options\n");
> 	exit(EINVAL);
> }
> 
> case OP_SHOW:
> 	check_name_value();
> 	break;
> case OP_BIND:
> 	check_name_value();
> 	break;
> 
> 
> Neither looks good.
> 
> 
>>>> -	if ((!name && value) || (name && !value)) {
>>>> -		eprintf("'name' and 'value' options are necessary\n");
>>>> -		exit(EINVAL);
>>>> -	}
>>> Ditto.
>> This check moved into the proper location -
>> in the switch under MODE_TARGET
>>
>>>>  	if (mode == MODE_TARGET) {
>>>> +		if ((tid < 0 && (op != OP_SHOW))) {
>>>> +			eprintf("'tid' option is necessary\n");
>>>> +			exit(EINVAL);
>>>> +		}
>>>>  		switch (op) {
>>>>  		case OP_NEW:
>>>> +			if (!targetname) {
>>>> +				eprintf("creating a new target needs --targetname\n");
>>>> +				exit(EINVAL);
>>>> +			}
>>>> +			break;
>>>> +		case OP_SHOW:
>>>> +			break;
>>>>  		case OP_DELETE:
>>>> +			break;
>>>>  		case OP_BIND:
>>>>  		case OP_UNBIND:
>>>> -		case OP_UPDATE:
>>>> -			if (op == OP_NEW && !targetname) {
>>>> -				eprintf("creating a new target needs the name\n");
>>>> +			if (!address) {
>>>> +				eprintf("%s operation requires initiator-address\n", op==OP_BIND?"bind":"unbind");
>>>>  				exit(EINVAL);
>>>>  			}
>>>> -
>>>> -			if (tid < 0) {
>>>> -				eprintf("'tid' option is necessary\n");
>>>> +			break;
>>>> +		case OP_UPDATE:
>>>> +			if ((!name || !value)) {
>>>> +				eprintf("update operation requires 'name' and 'value' options\n");
>>>>  				exit(EINVAL);
>>>>  			}
>>>>  			break;
>> This is where the check move into.
> 
> This checks only 'update' option. With your change, we can't catch a
> mistake like:
> 
> tgtadm --op delete --mode target --name hoge --value xyz
> 
> 

this specific mistake will be caught because --tid isn't provided.
following your method we'll need to check many more combinations.

For example, why not checking:
tgtadm --mode target --op show --tid=1 --initiator-address=xyz

In my opinion, as long as the user has provided all of the valid options
the command is a valid command.
so it won't be a mistake to write 
tgtadm --op delete --mode target --tid=1 --name hoge --value xyz
it will just delete tid 1.

> 
>>>>  		default:
>>>> +			eprintf("option %d not supported in target mode\n", op);
>>>> +			exit(EINVAL);
>>>>  			break;
>>>>  		}
>>>>  	}
>>>> @@ -541,8 +544,32 @@ int main(int argc, char **argv)
>>>>  			}
>>>>  			break;
>>>>  		default:
>>>> -			eprintf("the update operation can't"
>>>> -				" handle accounts\n");
>>>> +			eprintf("option %d not supported in account mode\n", op);
>>>> +			exit(EINVAL);
>>>> +			break;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	if (mode == MODE_DEVICE) {
>>>> +		if (tid < 0) {
>>>> +			eprintf("'tid' option is necessary\n");
>>>> +			exit(EINVAL);
>>>> +		}
>>>> +		if (!lun) {
>>>> +			eprintf("'lun' option is necessary\n");
>>>> +			exit(EINVAL);
>>>> +		}
>>>> +		switch (op) {
>>>> +		case OP_NEW:
>>>> +			if (!path) {
>>>> +				eprintf("'backing-store' option is necessary\n");
>>>> +				exit(EINVAL);
>>>> +			}
>>>> +			break;
>>> The null backing store code doesn't the patch option. We can add a
>>> trick here. Feel free to do it in a different patch later.
>> I didn't understand what is the problem,
>> can you please explain?
> 
> The null backing store code doesn't need the path option because it
> doesn't perform any I/O. It's just for performance measurements.
> 
> Check out usr/bs_null.c

As far as I understand you have to use -b (path) option
in order to create a null backing store.
If I'll try to do something like this:

tgtadm --mode logicalunit --op new --tid=1 --lun=1 -E null
tgtadm: invalid request

and

tgtadm --mode logicalunit --op new --tid=1 --lun=1 -b xyz -E null
tgtadm --mode target --op show
Target 1: doron
    System information:
        Driver: iscsi
        State: ready
    I_T nexus information:
    LUN information:
        LUN: 0
            Type: controller
            SCSI ID: deadbeaf1:0
            SCSI SN: beaf10
            Size: 0 MB
            Online: Yes
            Removable media: No
            Backing store: No backing store
        LUN: 1
            Type: disk
            SCSI ID: deadbeaf1:1
            SCSI SN: beaf11
            Size: 1099512 MB
            Online: Yes
            Removable media: No
            Backing store: xyz
    Account information:
    ACL information:

> 
> 
>>>> +		case OP_DELETE:
>>>> +			break;
>>>> +		default:
>>>> +			eprintf("option %d not supported in logicalunit mode\n", op);
>>>>  			exit(EINVAL);
>>>>  			break;
>>>>  		}
>>>> -- 
>>>> 1.5.3.8
>>>>
>>>>
>>>> I know that there many more commands which I didn't checked.
>>>> We still need to add some more testing for the different modes (SYSTEM,SESSION and CONNECTION).
>>>> And to document them as well.
>>> tgtadm is in a mess (the error check, a way to communicate with tgtd,
>>> etc). Any effort to clean up it are really welcome.
>> The first thing to do is to document all the "hidden" options -
>> that way it will be more clear what each option does and
>> what parameters it receives.
> 
> Yeah, documenting is also great.

--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux SCSI]     [Linux RAID]     [Linux Clusters]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]

  Powered by Linux