test for success of windows scsi command looks incorrect

[ZakDanger]

In the file smartie/scsi/windows.py at line 100, you are testing for success of a scsi command by accepting a scsi_status value of 0 or 2 as succeeded. However I think this should just check for a scsi_status value of 0 for success.

A value of 2 usually means "something went wrong, check the sense data for more info". So in these cases the sense data is usually populated.

I found this webpage that lists the various values for scsi_status. It's annoying that these values are not documated clearly on microsoft API related websites :P https://dosbox-x.com/doxygen/html/scsidefs_8h_source.html

[ZakDanger]

I also notice that errors seem to have two ways of being handled. 1) After a scsi command is executed the "sense data" is parsed by SCSIDevice.parse_sense(), with some sense data values resulting in a SenseError exception being thrown. 2) If the sense data parsing didnt throw an exception then you return a value in SCSIResponse() for "succeeded", along with the sense data.

Perhaps I am missing something here but why is there sometimes an exception and sometimes a success flag returned?

Sense data is basically an "error code" that should be looked at if an error occured. The caller needs to be able to look at the sense data values to work out where/what went wrong. So the user would check the "succeeded" value and if false then they would look at the sense data. It seems the exception is not needed here and actually sdtops the user from being able to look at the SCSIResponse() data because they won't ever receive it if the exception occurs.

[ZakDanger]

One small thing, you are calling self.parse_sense(bytearray(header_with_buffer.sense)) twice after sending a scsi comand in WindowsSCSIDevice.issue_command(). Not sure if you meant to do this.

[ZakDanger]

One other small thing, out of the full sense data, there are 3 bytes that make up the main "error code" value that users will check. Usually these byte are provided together as a 3 byte array for the user to check. Perhaps you could include a small function that returns these 3 bytes grouped together when passed in the full sense data, or have your code extract them and include them in the SCSIResponse().

These 3 bytes are: sense[2] = sense key (aka key) sense[12] = sense code (aka asc) sense[13] = sense qualifier (aka ascq)

These bytes are usually combined together into an array of 3 bytes to give error codes such as the ones shown here: https://en.wikipedia.org/wiki/Key_Code_Qualifier

an example of a function that might extract them:

def kcq(sense):
    return bytes((sense[2], sense[12], sense[13]))

[jackeichen]

In the file smartie/scsi/windows.py at line 100, you are testing for success of a scsi command by accepting a scsi_status value of 0 or 2 as succeeded. However I think this should just check for a scsi_status value of 0 for success.

A value of 2 usually means "something went wrong, check the sense data for more info". So in these cases the sense data is usually populated.

I found this webpage that lists the various values for scsi_status. It's annoying that these values are not documated clearly on microsoft API related websites :P https://dosbox-x.com/doxygen/html/scsidefs_8h_source.html

scsi status value 2 means "Check Condition", i think here is to be compatible with ata passthrough"? @TkTech We may check it Combined with command OP Code，or do something else to make difference between scsi command set and ata command set.

[TkTech]

Haven't really had a chance to look at this yet, been a bit busy.

Sense data is basically an "error code" that should be looked at if an error occured. The caller needs to be able to look at the sense data values to work out where/what went wrong. So the user would check the "succeeded" value and if false then they would look at the sense data. It seems the exception is not needed here and actually sdtops the user from being able to look at the SCSIResponse() data because they won't ever receive it if the exception occurs.

This isn't quite right, sense data can be returned for successful commands not just errors. The presence of sense data isn't related to the error state, but failing to parse the Sense data is definitely an exception.

[jackeichen]

One other small thing, out of the full sense data, there are 3 bytes that make up the main "error code" value that users will check. Usually these byte are provided together as a 3 byte array for the user to check. Perhaps you could include a small function that returns these 3 bytes grouped together when passed in the full sense data, or have your code extract them and include them in the SCSIResponse().

These 3 bytes are: sense[2] = sense key (aka key) sense[12] = sense code (aka asc) sense[13] = sense qualifier (aka ascq)

These bytes are usually combined together into an array of 3 bytes to give error codes such as the ones shown here: https://en.wikipedia.org/wiki/Key_Code_Qualifier

an example of a function that might extract them:

def kcq(sense):
  return bytes((sense[2], sense[12], sense[13]))

What you need is easily to get, it should be:

def get_error_code(scsi_response):
    if scsi_response.sense:
        return bytes((scsi_response.sense.sense_key,
                      scsi_response.sense.additional_sense_code,
                      scsi_response.sense.additional_sense_code_qualifier,
                      ))