[Qemu-devel] [PATCH] scsi-disk: Fix crash if underlying host file or disk returns error.

[Qemu-devel] [PATCH] scsi-disk: Fix crash if underlying host file or disk returns error.

[Richard W.M. Jones]

Commit 40dce4ee6 "scsi-disk: fix rerror/werror=ignore" introduced a
bug which causes qemu to crash with the assertion error below if the
host file or disk returns an error:

  qemu-system-x86_64: hw/scsi/scsi-bus.c:1374: scsi_req_complete:
  Assertion `req->status == -1' failed.

Kevin Wolf suggested this fix:

  < kwolf> Hm, should the final return false; in that patch
           actually be a return true?
  < kwolf> Because I think he didn't intend to change anything
           except BLOCK_ERROR_ACTION_IGNORE

Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
Buglink: https://bugs.launchpad.net/qemu/+bug/1804323
---
 hw/scsi/scsi-disk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 6eb258d3f3..0e9027c8f3 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -482,7 +482,7 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
     if (action == BLOCK_ERROR_ACTION_STOP) {
         scsi_req_retry(&r->req);
     }
-    return false;
+    return true;
 }
 
 static void scsi_write_complete_noio(SCSIDiskReq *r, int ret)
-- 
2.19.0.rc0

Re: [Qemu-devel] [PATCH] scsi-disk: Fix crash if underlying host file or disk returns error.

[Kevin Wolf]

Am 21.11.2018 um 13:47 hat Richard W.M. Jones geschrieben:
> Commit 40dce4ee6 "scsi-disk: fix rerror/werror=ignore" introduced a
> bug which causes qemu to crash with the assertion error below if the
> host file or disk returns an error:
> 
>   qemu-system-x86_64: hw/scsi/scsi-bus.c:1374: scsi_req_complete:
>   Assertion `req->status == -1' failed.
> 
> Kevin Wolf suggested this fix:
> 
>   < kwolf> Hm, should the final return false; in that patch
>            actually be a return true?
>   < kwolf> Because I think he didn't intend to change anything
>            except BLOCK_ERROR_ACTION_IGNORE
> 
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1804323

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH] scsi-disk: Fix crash if underlying host file or disk returns error.

[Paolo Bonzini]

On 21/11/18 13:47, Richard W.M. Jones wrote:
> Commit 40dce4ee6 "scsi-disk: fix rerror/werror=ignore" introduced a
> bug which causes qemu to crash with the assertion error below if the
> host file or disk returns an error:
> 
>   qemu-system-x86_64: hw/scsi/scsi-bus.c:1374: scsi_req_complete:
>   Assertion `req->status == -1' failed.
> 
> Kevin Wolf suggested this fix:
> 
>   < kwolf> Hm, should the final return false; in that patch
>            actually be a return true?
>   < kwolf> Because I think he didn't intend to change anything
>            except BLOCK_ERROR_ACTION_IGNORE
> 
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1804323
> ---
>  hw/scsi/scsi-disk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 6eb258d3f3..0e9027c8f3 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -482,7 +482,7 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
>      if (action == BLOCK_ERROR_ACTION_STOP) {
>          scsi_req_retry(&r->req);
>      }
> -    return false;
> +    return true;
>  }
>  
>  static void scsi_write_complete_noio(SCSIDiskReq *r, int ret)
> 

Looks good.  I was confused because now the function always returns
true.  "If an arm was returning true, the other must be returning false...".

Paolo

Re: [Qemu-devel] [PATCH] scsi-disk: Fix crash if underlying host file or disk returns error.

[Kevin Wolf]

Am 21.11.2018 um 19:31 hat Paolo Bonzini geschrieben:
> On 21/11/18 13:47, Richard W.M. Jones wrote:
> > Commit 40dce4ee6 "scsi-disk: fix rerror/werror=ignore" introduced a
> > bug which causes qemu to crash with the assertion error below if the
> > host file or disk returns an error:
> > 
> >   qemu-system-x86_64: hw/scsi/scsi-bus.c:1374: scsi_req_complete:
> >   Assertion `req->status == -1' failed.
> > 
> > Kevin Wolf suggested this fix:
> > 
> >   < kwolf> Hm, should the final return false; in that patch
> >            actually be a return true?
> >   < kwolf> Because I think he didn't intend to change anything
> >            except BLOCK_ERROR_ACTION_IGNORE
> > 
> > Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1804323
> > ---
> >  hw/scsi/scsi-disk.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> > index 6eb258d3f3..0e9027c8f3 100644
> > --- a/hw/scsi/scsi-disk.c
> > +++ b/hw/scsi/scsi-disk.c
> > @@ -482,7 +482,7 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
> >      if (action == BLOCK_ERROR_ACTION_STOP) {
> >          scsi_req_retry(&r->req);
> >      }
> > -    return false;
> > +    return true;
> >  }
> >  
> >  static void scsi_write_complete_noio(SCSIDiskReq *r, int ret)
> > 
> 
> Looks good.  I was confused because now the function always returns
> true.  "If an arm was returning true, the other must be returning false...".

Yes. And we should probably simplify the function by removing the return
value, but the minimal fix is better for 3.1.

Kevin