On Fri, 10 Oct 2008 11:38:39 +0200
"Eli Dorfman" <dorfman.eli@xxxxxxxxx> wrote:
> On Wed, Oct 8, 2008 at 9:02 AM, FUJITA Tomonori
> <fujita.tomonori@xxxxxxxxxxxxx> wrote:
> > On Wed, 08 Oct 2008 08:45:32 +0200
> > Doron Shoham <dorons@xxxxxxxxxxxx> wrote:
> >
> >> 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.
> >
> > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
