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

[Paolo Bonzini]

On 17/08/2017 16:11, Fam Zheng wrote:
> This makes the werror/rerror options available on the scsi-block device,
> to allow user specify error handling policy in the same way as scsi-hd
> etc.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> 
> ---
> 
> Take care of status, error, sense and QMP as well. [Paolo]
> ---
>  hw/scsi/scsi-disk.c | 81 
> ++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 59 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 5f1e5e8070..124d77c159 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -106,7 +106,8 @@ typedef struct SCSIDiskState
>      bool tray_locked;
>  } SCSIDiskState;
>  
> -static int scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed);
> +static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int status,
> +                                bool update_sense, bool acct_failed);
>  
>  static void scsi_free_request(SCSIRequest *req)
>  {
> @@ -179,21 +180,23 @@ static void scsi_disk_load_request(QEMUFile *f, 
> SCSIRequest *req)
>  
>  static bool scsi_disk_req_check_error(SCSIDiskReq *r, int ret, bool 
> acct_failed)
>  {
> +    int status = r->status ? *r->status : 0;
> +
>      if (r->req.io_canceled) {
>          scsi_req_cancel_complete(&r->req);
>          return true;
>      }
>  
> -    if (ret < 0) {
> -        return scsi_handle_rw_error(r, -ret, acct_failed);
> +    if (ret < 0 || status) {
> +        return scsi_handle_rw_error(r, -ret, status, !r->status, 
> acct_failed);

Do we need to pass both ret and !r->status?  We should update the sense
whenever error != 0, meaning the command hasn't run, so you can just add a

        case 0:
            /* The command has run, no need to fake sense.  */
            assert(r->status && *r->status);
            scsi_req_complete(&r->req, *r->status);

in scsi_handle_rw_error's switch statement.

>      }
>  
> -    if (r->status && *r->status) {
> +    if (status) {
>          if (acct_failed) {
>              SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>              block_acct_failed(blk_get_stats(s->qdev.conf.blk), &r->acct);
>          }
> -        scsi_req_complete(&r->req, *r->status);
> +        scsi_req_complete(&r->req, status);
>          return true;

This "if" is now dead code (yay!).

>      }
>  
> @@ -428,7 +447,8 @@ static void scsi_read_data(SCSIRequest *req)
>   * scsi_handle_rw_error always manages its reference counts, independent
>   * of the return value.
>   */
> -static int scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
> +static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int status,
> +                                bool update_sense, bool acct_failed)
>  {
>      bool is_read = (r->req.cmd.mode == SCSI_XFER_FROM_DEV);
>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> @@ -439,22 +459,38 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int 
> error, bool acct_failed)
>          if (acct_failed) {
>              block_acct_failed(blk_get_stats(s->qdev.conf.blk), &r->acct);
>          }
> -        switch (error) {
> -        case ENOMEDIUM:
> -            scsi_check_condition(r, SENSE_CODE(NO_MEDIUM));
> -            break;
> -        case ENOMEM:
> -            scsi_check_condition(r, SENSE_CODE(TARGET_FAILURE));
> -            break;
> -        case EINVAL:
> -            scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
> -            break;
> -        case ENOSPC:
> -            scsi_check_condition(r, SENSE_CODE(SPACE_ALLOC_FAILED));
> -            break;
> -        default:
> -            scsi_check_condition(r, SENSE_CODE(IO_ERROR));
> -            break;
> +        if (update_sense) {
> +            switch (error) {
> +            case ENOMEDIUM:
> +                scsi_check_condition(r, SENSE_CODE(NO_MEDIUM));
> +                break;
> +            case ENOMEM:
> +                scsi_check_condition(r, SENSE_CODE(TARGET_FAILURE));
> +                break;
> +            case EINVAL:
> +                scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
> +                break;
> +            case ENOSPC:
> +                scsi_check_condition(r, SENSE_CODE(SPACE_ALLOC_FAILED));
> +                break;
> +            default:
> +                scsi_check_condition(r, SENSE_CODE(IO_ERROR));
> +                break;
> +            }
> +        }

Because of the dead code above, you're now not running scsi_req_complete.

> +    }
> +    if (!error) {
> +        assert(status);

Since you are using the request object, you might as well
"assert(r->status && *r->status)" and skip passing the argument.

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

Later we could also add a blk_error_action version that takes a char*,
and pass a string that describes the sense data (see Linux
drivers/scsi/sense_codes.h).

Paolo

>          }
>      }
>      blk_error_action(s->qdev.conf.blk, action, is_read, error);
> @@ -2972,6 +3008,7 @@ static const TypeInfo scsi_cd_info = {
>  
>  #ifdef __linux__
>  static Property scsi_block_properties[] = {
> +    DEFINE_BLOCK_ERROR_PROPERTIES(SCSIDiskState, qdev.conf),         \
>      DEFINE_PROP_DRIVE("drive", SCSIDiskState, qdev.conf.blk),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>