You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
80 lines
2.9 KiB
80 lines
2.9 KiB
4 weeks ago
|
From eebe5fe8cbc854a6365e7c1adbb701079b137bcb Mon Sep 17 00:00:00 2001
|
||
|
|
From: Kevin Wolf <kwolf@redhat.com>
|
||
|
|
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 <kwolf@redhat.com>
|
||
|
|
RH-MergeRequest: 261: scsi-block: Fix error handling with r/werror=stop
|
||
|
|
RH-Jira: RHEL-50000
|
||
|
|
RH-Acked-by: Hanna Czenczek <hreitz@redhat.com>
|
||
|
|
RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
|
||
|
|
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 <kwolf@redhat.com>
|
||
|
|
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
|
||
|
|
Message-ID: <20240731123207.27636-4-kwolf@redhat.com>
|
||
|
|
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
|
||
|
|
(cherry picked from commit 8a0495624f23f8f01dfb1484f367174f3b3572e8)
|
||
|
|
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
|
||
|
|
---
|
||
|
|
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
|
||
|
|
|