FUJITA Tomonori wrote:
> On Wed, 08 Oct 2008 08:45:32 +0200
> Doron Shoham <dorons@xxxxxxxxxxxx> wrote:
>
>> FUJITA Tomonori wrote:
>>> On Tue, 07 Oct 2008 18:01:31 +0200
>>> Doron Shoham <dorons@xxxxxxxxxxxx> wrote:
>>>
>>>> check for valid parameters when passing tgtadm commands
>>>>
>>>> Signed-off-by: Doron Shoham <dorons@xxxxxxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
