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. > >> - 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. This checks only 'update' option. With your change, we can't catch a mistake like: tgtadm --op delete --mode target --name hoge --value xyz > >> 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? The null backing store code doesn't need the path option because it doesn't perform any I/O. It's just for performance measurements. Check out usr/bs_null.c > > > >> + 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. Yeah, documenting is also great. -- 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 |