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? > - if ((!name && value) || (name && !value)) { > - eprintf("'name' and 'value' options are necessary\n"); > - exit(EINVAL); > - } Ditto. > 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; > 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. > + 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. 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 |