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 |