On Thu, Apr 10, 2025 at 4:25 PM Paolo Bonzini <pbonz...@redhat.com> wrote:
> You should set ret = 0 here to avoid going down the
> scsi_sense_from_errno() path.
>
> Otherwise,
>
> Reviewed-by: Paolo Bonzini <pbonz...@redhat.com>
Okay, going down the scsi_sense_from_errno() path is more or less
harmless because status and sense end up unused; even though ENODEV is
not something that the function handles, that can be added as a
cleanup in 10.1.
Paolo
> > + }
> > +
> > if (ret < 0) {
> > status = scsi_sense_from_errno(-ret, &sense);
> > error = -ret;
> > @@ -289,6 +303,10 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int
> > ret, bool acct_failed)
> > if (acct_failed) {
> > block_acct_failed(blk_get_stats(s->qdev.conf.blk), &r->acct);
> > }
> > + if (host_status != -1) {
> > + scsi_req_complete_failed(&r->req, host_status);
> > + return true;
> > + }
> > if (req_has_sense) {
> > sdc->update_sense(&r->req);
> > } else if (status == CHECK_CONDITION) {
> > @@ -409,7 +427,6 @@ done:
> > scsi_req_unref(&r->req);
> > }
> >
> > -/* May not be called in all error cases, don't rely on cleanup here */
> > static void scsi_dma_complete(void *opaque, int ret)
> > {
> > SCSIDiskReq *r = (SCSIDiskReq *)opaque;
> > @@ -448,7 +465,6 @@ done:
> > scsi_req_unref(&r->req);
> > }
> >
> > -/* May not be called in all error cases, don't rely on cleanup here */
> > static void scsi_read_complete(void *opaque, int ret)
> > {
> > SCSIDiskReq *r = (SCSIDiskReq *)opaque;
> > @@ -585,7 +601,6 @@ done:
> > scsi_req_unref(&r->req);
> > }
> >
> > -/* May not be called in all error cases, don't rely on cleanup here */
> > static void scsi_write_complete(void * opaque, int ret)
> > {
> > SCSIDiskReq *r = (SCSIDiskReq *)opaque;
> > @@ -2846,14 +2861,10 @@ static void scsi_block_sgio_complete(void *opaque,
> > int ret)
> > sg_io_hdr_t *io_hdr = &req->io_header;
> >
> > if (ret == 0) {
> > - /* FIXME This skips calling req->cb() and any cleanup in it */
> > if (io_hdr->host_status != SCSI_HOST_OK) {
> > - scsi_req_complete_failed(&r->req, io_hdr->host_status);
> > - scsi_req_unref(&r->req);
> > - return;
> > - }
> > -
> > - if (io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT) {
> > + r->req.host_status = io_hdr->host_status;
> > + ret = -ENODEV;
> > + } else if (io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT) {
> > ret = BUSY;
> > } else {
> > ret = io_hdr->status;
>
