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

[FUJITA Tomonori]

On Fri, 10 Oct 2008 11:38:39 +0200
"Eli Dorfman" <dorfman.eli at gmail.com> wrote:

> 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 if we go with this way, the code would be like:

case OP_DELETE:
     check_name_value();
     check_is_initiator_address_null();
     check_is_backing_store_null();
     ...
     break;
case OP_SHOW:
     check_name_value();
     check_is_initiator_address_null();
     check_is_backing_store_null();
     ...
     break;


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

I know, as I said in the previous mail, surely we need to fix tgtadm.
--
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