Re: [Qemu-devel] [PATCH v2] scsi-block: Add qdev error properties

[Paolo Bonzini]

On 18/08/2017 05:38, Fam Zheng wrote:
> On Thu, 08/17 16:23, Paolo Bonzini wrote:
>>> +        if (scsi_sense_matches(r, SENSE_CODE(NO_MEDIUM))) {
>>> +            error = ENOMEDIUM;
>>> +        } else if (scsi_sense_matches(r, SENSE_CODE(TARGET_FAILURE))) {
>>> +            error = ENOMEM;
>>> +        } else if (scsi_sense_matches(r, SENSE_CODE(INVALID_FIELD))) {
>>> +            error = EINVAL;
>>> +        } else if (scsi_sense_matches(r, SENSE_CODE(SPACE_ALLOC_FAILED))) {
>>> +            error = ENOSPC;
>>> +        } else {
>>> +            error = EIO;
>>
>> Nice touch :) and in fact the ENOSPC case is needed for correctness.
>>
> 
> Offline you mentioned iscsi_translate_sense(). Makes sense to unify both. We 
> can
> create a util/scsi.c to move it there.
> 
> About the error/non-error condition, is it enough to add a check "status ==
> CHECK_CONDITION"?

No, CHECK_CONDITION is for almost every error.

Of those in iscsi_translate_sense:

- DATA_PROTECTION should be an error

- COMMAND_ABORTED should be passed down

- of the various ILLEGAL_REQUEST cases, only ENOMEDIUM and ENOACCES
should stop.  LBA out of range should never happen (see
scsi_disk_dma_command).

- NOT_READY should also look at asc/ascq individually, like
ILLEGAL_REQUEST, see below

Of those in your list above, these are missing in iscsi_translate_sense:

- SPACE_ALLOC_FAILED (which should stop the VM)

- NOT_READY should also look at asc/ascq individually, like
ILLEGAL_REQUEST.  NO_MEDIUM is returning -EBUSY instead of -ENOMEDIUM.

Among other sense keys, MEDIUM_ERROR and HARDWARE_ERROR probably should
stop the VM.

Paolo