possible problem with reporting correct error/sense code

[rosjat]

lets say we do something very simple commandwise but we know is going to fail since the device doesnt support this command.

here we have Report Timestamp, just building a cdb for now nothing else, this should return an illegal request sense code but it doesnt.

class ReportTimestamp(SCSICommand):
    """
    A class to hold information from a ReportTimestamp command to a scsi device
    """
    _cdb_bits =        {'opcode': [0xff, 0],
                        'service_action': [0x1f, 1],
                        'alloc_len': [0xffffffff, 6], }

    _data_bits =       {'tspdl': [0xffff, 0],
                        'ts_origin': [0x07, 2],
                        'ts': [0xffffffffffff, 4], }

    def __init__(self,
                 opcode,
                 alloclen=12):
        """
        initialize a new instance

        :param opcode: a OpCode instance
        :param alloclen: the max number of bytes allocated for the data_in buffer
        """
        SCSICommand.__init__(self,
                             opcode,
                             0,
                             alloclen)

        self.cdb = self.build_cdb(opcode=self.opcode.value,
                                  service_action=self.opcode.serviceaction.REPORT_TIMESTAMP,
                                  alloc_len=alloclen)

calling this in example brings the following

Traceback (most recent call last):
  File "examples/report_timestamp.py", line 28, in <module>
    main(sys.argv[1])
  File "examples/report_timestamp.py", line 23, in main
    d.execute(cmd)
  File "/usr/local/lib/python3.7/site-packages/pyscsi/pyscsi/scsi_device.py", line 132, in execute
    cmd.datain)
  File "src/linux_sgio.pyx", line 144, in sgio.execute
sgio.UnspecifiedError

this would refer to this code

result = ioctl(fid.fileno(), SG_IO, &io_hdr)

    if result < 0:
        raise OSError(errno, 'ioctl failed')

    if io_hdr.info & SG_INFO_OK_MASK != SG_INFO_OK:
        if io_hdr.sb_len_wr > 0:
            raise CheckConditionError(sense)
        else:
            raise UnspecifiedError()

as far as i figured we get a 0 from the ioctl and this would result in that, i might be wrong with my cdb but again i know this command is not supported by the blockdevice i am issue this cdb at. So lets keep an eye on this.

[rosjat]

question about this enum

    cdef enum:
        SCSI_STATUS_CHECK_CONDITION

does this translate to 0 , if so this is wrong this hast to translate to 2!

[Flameeyes]

question about this enum

    cdef enum:
        SCSI_STATUS_CHECK_CONDITION

does this translate to 0 , if so this is wrong this hast to translate to 2!

I just sent a pull request to remove that altogether, since the Cython implementation does not use it. In the pure C version, you were returning that as a status code (https://github.com/rosjat/linux_sgio/blob/d9a94480af187d61774d92a5c36e39d0a04a8321/linux_sgio/sgiomodule.c#L255) but Cython raises an exception directly in both cases.

The logic behind the type of exception is the same between the two versions — just in this case it's inlined in Cython rather than delegated to the Python caller. It seems like the ioctl succeeded (no OSError) but it didn't write a sense buffer (sb_len_wr == 0). This makes sense, unless mx_sb_len was set to zero, but that doesn't make sense to me, as sense is allocated in the function itself, and if it failed allocation it should have raised a MemoryError() already.

Any chance you can run this through strace and see what the ioctl structure is reported as?

P.S.: the Cython cdef enum does not define any values — it just tells Cython that the name is defined in C (in the headers already included), which is why none of the enums have an explicit value next to them.

[Flameeyes]

This should be fixed now, and released in 1.1.1 just to be on the safe side.

[rosjat]

well at least no error but i dont get any usefull data back from excecute so i dont really know what to make of it. Again it should give me a sense code but it returns an empty array

[Flameeyes]

Okay, let me see if I can write a test using qemu…

[rosjat]

sorry i am so used to simply reinstall changes and run it again, but like i said i simply build the cdb and pass it to the execute method and the block device i test (one of my hdd) doesnt support this command so it should give me an illegal request. And if something is wrong with the cdb it would fail even before the sense code

[rosjat]

about handle stuff sensewise in the cython module, this is not a good approach! Since we already got sense code in python-scsi we should python-scsi handle this! If you really want a to have some sense handling in this module we should make sure it still hands over stuff for python-scsi to figure out so we dont end up with situations like in this scenario.

so this means something useful needs to be returned from CheckConditionError, if we got that it will work with the sense code in python-scsi here

 sgio.execute(
                self._file,
                cmd.cdb,
                cmd.dataout,
                cmd.datain)
        except sgio.CheckConditionError as error:
            self.CheckCondition(error.sense)

so error.sense need to be a bytearray that we can handle with SCSICheckCondition in scsi_sense.py

[rosjat]

Another thing that might be a problem is, and i really dont know this, the types we use for datain here.
In the c extention datain and dataout are Py_buffer datastructutes and here data in is some kind of unsigned char array? I did some research and you can use py_buffer with cython so we might need to consider this?

[Flameeyes]

about handle stuff sensewise in the cython module, this is not a good approach! Since we already got sense code in python-scsi we should python-scsi handle this!

Sorry I wasn't clear before — what cython-sgio does now (and rather, with the pull request I just sent, since it was half-broken) is handing off the (unparsed) sense data as a CheckConditionError exception. cython-sgio does not do any parsing, or take any decision, based on the content.

so error.sense need to be a bytearray that we can handle with SCSICheckCondition in scsi_sense.py

Indeed it was supposed to be that — but I made a mistake when copying it over. I have fixed that in #4, and now it behaves as it should have, and as you would have expected. Well, nearly, it's returning bytes rather than bytearray, which is just fine, as we don't really need to change its content.

I'm still trying to find a way to have actual tests that don't require root to run :( Not easy.

[rosjat]

sorry but i still dont get some of the things this code is doing or better not doing...

where does it allocated memory for a data_out buffer at all?

what is going on with these line, it doesnt really make sense to me ...

cdef unsigned char[:] input_view = data_in

io_hdr.dxferp = &input_view[0]

again in the c implementation both values are py_buffer stucts and i dont think we should cast this to a char array here. If we take a look at py_buffer it does more then simply holding a char array, i might be wrong but again if i issue simple CDBs that should produce datain content and they dont it's confusing. When a command comes back with a GOOD status and it should produce data in any way.

io_hdr.dxferp is by definition a void pointer so assuming it will always be unsigned char array might be wrong?