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

[Fam Zheng]

On Fri, 08/18 08:41, Paolo Bonzini wrote:
> 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

And the same to DATA PROTECT.

> 
> 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.

Thanks, I'll work on a new version.

Fam