Re: iscsi-test vs iscsi-test-cu

[ronnie sahlberg]

They should be the same.

I will take a look at the tests you mention tonight.


On Tue, Mar 19, 2013 at 3:18 AM, <sit...@gmail.com> wrote:
> Hello,
>
> There are a few tests in iscsi-test-cu (TestRead10.testRead10ZeroBlocks,
> TestWriteSame10.testWriteSame10Unmap) that return different results to their
> equivalent tests (T0102_read10_0blocks, T0180_writesame10_unmap) in the old
> iscsi-test tool. On the system I'm testing the new tests pass whereas the
> old tests fail. Are the new tests correct in their behaviour?
>
> --
> You received this message because you are subscribed to the Google Groups
> "libiscsi" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to libiscsi+u...@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[ronnie sahlberg]

Sitsofe,

Many thanks!

I have just pushed a change to the old testsuite so it tests and
reports the same thing.

This particular part is a bit ambigous in the SBC standard.
Strictly speaking, by following the SBC standard to the extreme, the
old test were right and the new test suite was more forgiving.
(see the commit message for more data)
but this is an area that is ambigous so I bailed out and dont test
"check 0 block access at the exact end of the device" and changed the
old test suite
to be more like the new one and instead test "check 0 block access one
full block past the end" instead.

I think the old testsuite was just too "follow the spec to the letter
eventhough it is not important and it is really not that reasonably to
enforce this."


Again, thanks for pointing this delta out to me.


May I ask what you use the testsuite for ?
Maybe I can convince you to help out creating more tests and make it
more complete ?
It would be really nice with tests for the full SSC and MMC commandsets too :-)

best regards
ronnie sahlberbg


On Tue, Mar 19, 2013 at 8:37 AM, <sit...@gmail.com> wrote:
> Here is the output that the tests produce on my system:
>
> ./iscsi-test --dataloss --test=T0102_read10_0blocks iscsi://host/iqn/0
>
> Actual result:
> 0102_read10_0blocks:
> ====================
> READ10 0blocks at LBA:0 ... [OK]
> READ10 0blocks at one block beyond <end-of-LUN> ... [FAILED]
> READ10 command: Should fail when reading 0blocks beyond end
> TEST T0102_read10_0blocks [FAILED]
>
>
> ./iscsi-test-cu -d -t TestRead10.testRead10ZeroBlocks
> iscsi://host/iqn/0 -V
>
>
> Test READ10 0-blocks at LBA==0
> Send READ10 LBA:0 blocks:0 rdprotect:0 dpo:0 fua:0 fua_nv:0 group:0
> [OK] READ10 returned SUCCESS.
> Test READ10 0-blocks one block past end-of-LUN
> Send READ10 (Expecting LBA_OUT_OF_RANGE) LBA:204801 blocks:0 rdprotect:0
> dpo:0 fua:0 fua_nv:0 group:0
> [OK] READ10 returned ILLEGAL_REQUEST/LBA_OUT_OF_RANGE.
> Test READ10 0-blocks at LBA==2^31
> Send READ10 (Expecting LBA_OUT_OF_RANGE) LBA:-2147483648 blocks:0
> rdprotect:0 dpo:0 fua:0 fua_nv:0 group:0
> [OK] READ10 returned ILLEGAL_REQUEST/LBA_OUT_OF_RANGE.
> Test READ10 0-blocks at LBA==-1
> Send READ10 (Expecting LBA_OUT_OF_RANGE) LBA:-1 blocks:0 rdprotect:0
> dpo:0
> fua:0 fua_nv:0 group:0
> [OK] READ10 returned ILLEGAL_REQUEST/LBA_OUT_OF_RANGE.
>
>
>
> ./iscsi-test --test=T0180_writesame10_unmap --dataloss iscsi://host/iqn/0
> 0180_writesame10_unmap:
> =======================
> Logical unit is fully provisioned. All commands should fail with check
> condition.
> Logical Block Provisioning is available. Check that VPD page 0xB2 exists ...
> [FAILED]
> Inquiry command failed : SENSE KEY:ILLEGAL_REQUEST(5)
> ASCQ:INVALID_FIELD_IN_CDB(0x2400)
> TEST T0180_writesame10_unmap [FAILED]
>
>
> ./iscsi-test-cu -d -t TestWriteSame10.testWriteSame10Unmap
> iscsi://host/iqn/0
>
> Tests completed with return value: 0

[Lee Duncan]

On Mar 21, 2013, at 9:52 AM, sit...@gmail.com wrote:

> Fix reservation key/service action reservation key confusion.
> ---
> diff --git a/test-tool/iscsi-support.c b/test-tool/iscsi-support.c
> index 04911d7..14c15f5 100644
> --- a/test-tool/iscsi-support.c
> +++ b/test-tool/iscsi-support.c
> @@ -663,7 +663,7 @@ prin_verify_key_presence(struct iscsi_context *iscsi, int lun,
>
> int
> prout_reregister_key_fails(struct iscsi_context *iscsi, int lun,
> - unsigned long long sark)
> + unsigned long long rk)
> {
> struct scsi_persistent_reserve_out_basic poc;
> struct scsi_task *task;
> @@ -680,7 +680,7 @@ prout_reregister_key_fails(struct iscsi_context *iscsi, int lun,
> }
>
> memset(&poc, 0, sizeof (poc));
> - poc.service_action_reservation_key = sark;
> + poc.reservation_key = rk;
> task = iscsi_persistent_reserve_out_sync(iscsi, lun,
> SCSI_PERSISTENT_RESERVE_REGISTER,
> SCSI_PERSISTENT_RESERVE_SCOPE_LU,
>


No, this patch is incorrect.

Here's a PGR refresher:

- SARK -- the service action reservation key is the one you pass in when
wanting to change it, via a service action. For example, if you want
to set the reservation you, you pass it in via SARK

- RK -- the reservation key is supposed to match the current reservation for
the IT Nexus making the call (i.e. the current client)

And, of course, zero is a special key meaning "no key".

So, for example, to register a reservation key, you pass in the service action of
REGISTER KEY, and you pass in the SARK of the new key, and the RK of zero
(meaning there is no current key).

I believe your patch has changed this function to try to reset the key to "none" (zero),
but it is passing in a random key for "RK", which of course does not match the
existing key.

The test is supposed to try to set the new key while not knowing the existing current
key, and that of course is supposed to fail.

You have not convinced me the existing test is incorrect.

May I ask why you (or the developer) thinks it's incorrect? Did your target not pass
this test?
--
Lee Duncan

"Man is the center of a circle of infinite circumference." -- John Lilly

[Lee Duncan]

Also Nacked ... (see previous email)

On Mar 21, 2013, at 9:53 AM, sit...@gmail.com wrote:

diff --git a/test-tool/iscsi-support.c b/test-tool/iscsi-support.c

index 4a3f88e..14c15f5 100644

--- a/test-tool/iscsi-support.c

+++ b/test-tool/iscsi-support.c

@@ -693,11 +693,9 @@ prout_reregister_key_fails(struct iscsi_context *iscsi, int lun,

  return -1;

  }

 

- if (task->status != SCSI_STATUS_CHECK_CONDITION ||

-    task->sense.key != SCSI_SENSE_ILLEGAL_REQUEST ||

-    task->sense.ascq != SCSI_SENSE_ASCQ_INVALID_OPERATION_CODE) {

+ if ( task->status != SCSI_STATUS_RESERVATION_CONFLICT ) {

  logging(LOG_NORMAL,

-    "[FAILED] PROUT/REGISTER when already registered should fail");

+    "[FAILED] PROUT/REGISTER when already registered should fail with reservation conflict");

  ret = -1;

  goto dun;

[Lee Duncan]

On Mar 22, 2013, at 3:53 AM, farey...@googlemail.com wrote:

>
> I'm the developer - here's why I think its wrong.
>
> The test PROUT Register Simple does the following:
> - Registers a Key using Register and Ignore
> - Reads the Key
> - Tries to re-register a key 1 greater than the original key using Register.

Correct.


> This is expected to fail with a Reservation Conflict.

Here's where I believe you've found an error. I _should_ generate a RESERVATION
CONFLICT, but the test instead checks for INVALID OPERATION.


> It passes 0 as the Key, and the original key + 1 as the SA Key

Correct.

>
> I'm looking at section 5.6.6 of SPC3-r23. There is a table there for the REGISTER Service Action, which lists the parameters expected when the command is received on a registered I_T nexus.

That is Table 34 in Version 3 rev 23, by the way.

I am using SPC 4, rev 33, where this is section 5.9.7, Table 49. And the table
has evolved quite a bit. It has a new (second) column, which is the supplied
reservation key (RK).


> That states that the expected key is in the Reservation Key parameter, not the SA reservation Key. Setting RK = original key and SARK = new key changes the key. Setting RK to original key and SARK to 0 unregisters the original key. Setting Key not equal to original key results in a reservation conflict, which seems to be what the test is trying to check.

I am quite familiar with this table. Way too familiar, I'm afraid. :)

I'll describe what I've done based on the updated version of the spec, for
those that don't have a copy handy.

That table has two main sections, based on the first column:
either the command is received on a registered I_T nexus, or it is not. And,
for this test, it is received on a registered I_T nexus, i.e. the bottom half of
the table.

The part of the table we now have left is further divided into two halves, again,
this time based on the Reservation Key (rk) passed in from the registered
I_T Nexus. This is the part of the table that is not present in your version.
For this version of the spec, either we pass in a key that is equal to the existing reservation
key, or we do not. For my test case, it passes in zero for the reservation
key, which means it is not equal to the I_T nexus reservation key. Table
49 says this should return a RESERVATION CONFLICT.

This is precisely the case I am trying to test, i.e. trying to register a key other
than the key when I do not know the current key.

Actually, it shouldn't matter what the SARK is, which is the value we are passing
in for the test, since it should be ignored.

[For this test to be more robust, it could check that all sorts of values for SARK
cause the same result, i.e. the SARK is ignored. But it does not currently do
that. Feel free to add tests. This was supposed to be the "simple" test, which
is why it doesn't do much.]

But I think you truly found a problem: the Check Condition returned should be
RESERVATION CONFLICT, not INVALID OPERATION, as my test checks for.

To be honest, I think I was lazy when I wrote that test, and I just set up the test to pass
for the targets I was using, and I didn't read this table in the spec to make
sure the check condition was correct.

>
> So I still maintain this test has got the RK and the SARK the wrong way round.
>

I'll supply a patch that changes the expected check condition soon.
--
Lee Duncan
SUSE Labs

[Chris Farey]

Another one which looks wrong, is the function prin_verify_not_reserved(), in iscsi-support.c

Its using the Read Reservation action to ensure that there is no reservation held. SPC3 states that if no reservation is held, then 8 bytes should be returned, with the additional length field set to 0. However, this function appears to assume that additonal data is returned.

The unmarshall function checks for this, and if there is no additonal data only allocates up to the reserved field of the structure. However, prin_verify_not_reserved() checks the reserved field, and I think is picking up random data.

I think the unmarshall function should be doing an offsetof() on reservation_key rather than reserved.