FUJITA Tomonori 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. > > >>>> - 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 > > this specific mistake will be caught because --tid isn't provided. following your method we'll need to check many more combinations. For example, why not checking: tgtadm --mode target --op show --tid=1 --initiator-address=xyz In my opinion, as long as the user has provided all of the valid options the command is a valid command. so it won't be a mistake to write tgtadm --op delete --mode target --tid=1 --name hoge --value xyz it will just delete tid 1. > >>>> 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 As far as I understand you have to use -b (path) option in order to create a null backing store. If I'll try to do something like this: tgtadm --mode logicalunit --op new --tid=1 --lun=1 -E null tgtadm: invalid request and tgtadm --mode logicalunit --op new --tid=1 --lun=1 -b xyz -E null tgtadm --mode target --op show Target 1: doron System information: Driver: iscsi State: ready I_T nexus information: LUN information: LUN: 0 Type: controller SCSI ID: deadbeaf1:0 SCSI SN: beaf10 Size: 0 MB Online: Yes Removable media: No Backing store: No backing store LUN: 1 Type: disk SCSI ID: deadbeaf1:1 SCSI SN: beaf11 Size: 1099512 MB Online: Yes Removable media: No Backing store: xyz Account information: ACL information: > > >>>> + 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 |