[stgt] [PATCH] check for valid parameters when passing tgtadm commands

[FUJITA Tomonori]

On Wed, 08 Oct 2008 08:45:32 +0200
Doron Shoham <dorons at voltaire.com> wrote:

> FUJITA Tomonori wrote:
> > On Tue, 07 Oct 2008 18:01:31 +0200
> > Doron Shoham <dorons at Voltaire.COM> wrote:
> > 
> >> check for valid parameters when passing tgtadm commands
> >>
> >> Signed-off-by: Doron Shoham <dorons at voltaire.com>
> >> ---
> >>  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



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


> > 
> >> +		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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html