From eebe5fe8cbc854a6365e7c1adbb701079b137bcb Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 31 Jul 2024 14:32:06 +0200 Subject: [PATCH 095/100] scsi-disk: Add warning comments that host_status errors take a shortcut RH-Author: Kevin Wolf RH-MergeRequest: 261: scsi-block: Fix error handling with r/werror=stop RH-Jira: RHEL-50000 RH-Acked-by: Hanna Czenczek RH-Acked-by: Stefan Hajnoczi RH-Commit: [3/4] 6fcd603fc78fda65a425a1acd9a8710d81c6ed7f (kmwolf/centos-qemu-kvm) scsi_block_sgio_complete() has surprising behaviour in that there are error cases in which it directly completes the request and never calls the passed callback. In the current state of the code, this doesn't seem to result in bugs, but with future code changes, we must be careful to never rely on the callback doing some cleanup until this code smell is fixed. For now, just add warnings to make people aware of the trap. Signed-off-by: Kevin Wolf Acked-by: Paolo Bonzini Message-ID: <20240731123207.27636-4-kwolf@redhat.com> Signed-off-by: Kevin Wolf (cherry picked from commit 8a0495624f23f8f01dfb1484f367174f3b3572e8) Signed-off-by: Kevin Wolf --- hw/scsi/scsi-disk.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index e7f57f3230..b4062ac2ff 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -65,6 +65,9 @@ struct SCSIDiskClass { /* * Callbacks receive ret == 0 for success. Errors are represented either as * negative errno values, or as positive SAM status codes. + * + * Beware: For errors returned in host_status, the function may directly + * complete the request and never call the callback. */ DMAIOFunc *dma_readv; DMAIOFunc *dma_writev; @@ -359,6 +362,7 @@ 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; @@ -399,6 +403,7 @@ 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; @@ -538,6 +543,7 @@ 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; @@ -2793,6 +2799,7 @@ 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); -- 2.39.3