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.
>
>
>> - 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.
>> 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?
>
>
>> + 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.
>
> Thanks!
--
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
