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

[FUJITA Tomonori]

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?


> -	if ((!name && value) || (name && !value)) {
> -		eprintf("'name' and 'value' options are necessary\n");
> -		exit(EINVAL);
> -	}

Ditto.

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


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

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