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

[Eli Dorfman]

On Wed, Oct 8, 2008 at 9:02 AM, FUJITA Tomonori
<fujita.tomonori at lab.ntt.co.jp> wrote:
> 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.

The second option looks much better, but in general i agree with
doron's approach that we should check values per option unless there
are common values (e.g. tid) that can be checked before the switch for
all options.
The current code of tgtadm does not handle properly any wrong input
from the user and simply returns sigfault.
--
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