Re: [Qemu-devel] about qemu crash in scsi_handle_rw_error

[Paolo Bonzini]

On 12/10/2018 10:05, Wangguang wrote:
> Hi
> 
> qemu had a assert when we use scsi-3 reservation。
> 
> This happen when scsi sence is recoverd error。
> 
> And which lead scsi_req_complete twice.
> 
>  
> 
>  
> 
> static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool
> acct_failed)
> 
> {
> 
>     bool is_read = (r->req.cmd.mode == SCSI_XFER_FROM_DEV);
> 
>     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> 
>     BlockErrorAction action = blk_get_error_action(s->qdev.conf.blk,
> 
>                                                    is_read, error);
> 
>  
> 
>     if (action == BLOCK_ERROR_ACTION_REPORT) {
> 
>         if (acct_failed) {
> 
>             block_acct_failed(blk_get_stats(s->qdev.conf.blk), &r->acct);
> 
>         }
> 
>         switch (error) {
> 
>         case 0:
> 
>             /* The command has run, no need to fake sense.  */
> 
>             assert(r->status && *r->status);
> 
>             scsi_req_complete(&r->req, *r->status);                     
> the  first complete
> 
>             break;
> 
>         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 (!error) {
> 
>         assert(r->status && *r->status);
> 
>         error = scsi_sense_buf_to_errno(r->req.sense, sizeof(r->req.sense));
> 
>  
> 
>         if (error == ECANCELED || error == EAGAIN || error == ENOTCONN ||
> 
>             error == 0)  {
> 
>             /* These errors are handled by guest. */
> 
>            scsi_req_complete(&r->req, *r->status);                     
> complete again when error==0;
> 
>             return true;
> 
>         }
> 
>     }
>  
> 
> Shall we delete error==0  at the second scsi_req_complete ？？

No, the first call must go away.  In fact, even better, you can move the
entire contents of "if (!error)" inside the "case 0:".

Paolo