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. > > >> - 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 at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html |