hw/scsi/vhost-scsi-common.c | 17 ++++++++++------- hw/scsi/vhost-scsi.c | 5 +++-- hw/scsi/vhost-user-scsi.c | 14 ++++++++------ hw/virtio/vhost-user.c | 10 +--------- include/hw/virtio/vhost-scsi-common.h | 2 +- 5 files changed, 23 insertions(+), 25 deletions(-)
The patchset fixes the regression issue of vhost reconnect. It's a serious bug that the vhost-user will lose the reconnect forever. The 2nd patch enhances the error handle of vhost-user-scsi. This patchset's parent commit is: https://lore.kernel.org/all/20230731121018.2856310-1-fengli@smartx.com/ Li Feng (2): vhost-user: fix lost reconnect vhost: Add Error parameter to vhost_scsi_common_start() hw/scsi/vhost-scsi-common.c | 17 ++++++++++------- hw/scsi/vhost-scsi.c | 5 +++-- hw/scsi/vhost-user-scsi.c | 14 ++++++++------ hw/virtio/vhost-user.c | 10 +--------- include/hw/virtio/vhost-scsi-common.h | 2 +- 5 files changed, 23 insertions(+), 25 deletions(-) -- 2.41.0
The patchset fixes the regression issue of vhost reconnect. It's a serious bug that the vhost-user will lose the reconnect forever. The 2nd patch enhances the error handle of vhost-user-scsi. This patchset's parent commit is: https://lore.kernel.org/all/20230731121018.2856310-1-fengli@smartx.com/ Changes for v3: - Fix the code style. Changes for v2: - Add a event_cb in VhostAsyncCallback to be called when dev is NULL; - Fix the error report message. Li Feng (2): vhost-user: Fix lost reconnect vhost: Add Error parameter to vhost_scsi_common_start() hw/block/vhost-user-blk.c | 2 +- hw/scsi/vhost-scsi-common.c | 16 +++++++++------- hw/scsi/vhost-scsi.c | 5 +++-- hw/scsi/vhost-user-scsi.c | 17 ++++++++++------- hw/virtio/vhost-user-gpio.c | 2 +- hw/virtio/vhost-user.c | 9 +++++++-- include/hw/virtio/vhost-scsi-common.h | 2 +- include/hw/virtio/vhost-user.h | 3 ++- 8 files changed, 34 insertions(+), 22 deletions(-) -- 2.41.0
When the vhost-user is reconnecting to the backend, and if the vhost-user fails
at the get_features in vhost_dev_init(), then the reconnect will fail
and it will not be retriggered forever.
The reason is:
When the vhost-user fails at get_features, the vhost_dev_cleanup will be called
immediately.
vhost_dev_cleanup calls 'memset(hdev, 0, sizeof(struct vhost_dev))'.
The reconnect path is:
vhost_user_blk_event
vhost_user_async_close(.. vhost_user_blk_disconnect ..)
qemu_chr_fe_set_handlers <----- clear the notifier callback
schedule vhost_user_async_close_bh
The vhost->vdev is null, so the vhost_user_blk_disconnect will not be
called, then the event fd callback will not be reinstalled.
All vhost-user devices have this issue, including vhost-user-blk/scsi.
With this patch, if the vdev->vdev is null, the fd callback will still
be reinstalled.
Fixes: 71e076a07d ("hw/virtio: generalise CHR_EVENT_CLOSED handling")
Signed-off-by: Li Feng <fengli@smartx.com>
---
hw/block/vhost-user-blk.c | 2 +-
hw/scsi/vhost-user-scsi.c | 3 ++-
hw/virtio/vhost-user-gpio.c | 2 +-
hw/virtio/vhost-user.c | 9 +++++++--
include/hw/virtio/vhost-user.h | 3 ++-
5 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 3c69fa47d5..95c758200d 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -391,7 +391,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
case CHR_EVENT_CLOSED:
/* defer close until later to avoid circular close */
vhost_user_async_close(dev, &s->chardev, &s->dev,
- vhost_user_blk_disconnect);
+ vhost_user_blk_disconnect, vhost_user_blk_event);
break;
case CHR_EVENT_BREAK:
case CHR_EVENT_MUX_IN:
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index a7fa8e8df2..e931df9f5b 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -236,7 +236,8 @@ static void vhost_user_scsi_event(void *opaque, QEMUChrEvent event)
case CHR_EVENT_CLOSED:
/* defer close until later to avoid circular close */
vhost_user_async_close(dev, &vs->conf.chardev, &vsc->dev,
- vhost_user_scsi_disconnect);
+ vhost_user_scsi_disconnect,
+ vhost_user_scsi_event);
break;
case CHR_EVENT_BREAK:
case CHR_EVENT_MUX_IN:
diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index d9979aa5db..04c2cc79f4 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -283,7 +283,7 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent event)
case CHR_EVENT_CLOSED:
/* defer close until later to avoid circular close */
vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
- vu_gpio_disconnect);
+ vu_gpio_disconnect, vu_gpio_event);
break;
case CHR_EVENT_BREAK:
case CHR_EVENT_MUX_IN:
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 8dcf049d42..12c4a41f30 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2643,6 +2643,7 @@ typedef struct {
DeviceState *dev;
CharBackend *cd;
struct vhost_dev *vhost;
+ IOEventHandler *event_cb;
} VhostAsyncCallback;
static void vhost_user_async_close_bh(void *opaque)
@@ -2657,7 +2658,10 @@ static void vhost_user_async_close_bh(void *opaque)
*/
if (vhost->vdev) {
data->cb(data->dev);
- }
+ } else if (data->event_cb) {
+ qemu_chr_fe_set_handlers(data->cd, NULL, NULL, data->event_cb,
+ NULL, data->dev, NULL, true);
+ }
g_free(data);
}
@@ -2669,7 +2673,8 @@ static void vhost_user_async_close_bh(void *opaque)
*/
void vhost_user_async_close(DeviceState *d,
CharBackend *chardev, struct vhost_dev *vhost,
- vu_async_close_fn cb)
+ vu_async_close_fn cb,
+ IOEventHandler *event_cb)
{
if (!runstate_check(RUN_STATE_SHUTDOWN)) {
/*
diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
index 191216a74f..649e9dd54f 100644
--- a/include/hw/virtio/vhost-user.h
+++ b/include/hw/virtio/vhost-user.h
@@ -84,6 +84,7 @@ typedef void (*vu_async_close_fn)(DeviceState *cb);
void vhost_user_async_close(DeviceState *d,
CharBackend *chardev, struct vhost_dev *vhost,
- vu_async_close_fn cb);
+ vu_async_close_fn cb,
+ IOEventHandler *event_cb);
#endif
--
2.41.0
> On Aug 30, 2023, at 12:57 AM, Li Feng <fengli@smartx.com> wrote:
>
> When the vhost-user is reconnecting to the backend, and if the vhost-user fails
> at the get_features in vhost_dev_init(), then the reconnect will fail
> and it will not be retriggered forever.
>
> The reason is:
> When the vhost-user fails at get_features, the vhost_dev_cleanup will be called
> immediately.
>
> vhost_dev_cleanup calls 'memset(hdev, 0, sizeof(struct vhost_dev))'.
>
> The reconnect path is:
> vhost_user_blk_event
> vhost_user_async_close(.. vhost_user_blk_disconnect ..)
> qemu_chr_fe_set_handlers <----- clear the notifier callback
> schedule vhost_user_async_close_bh
>
> The vhost->vdev is null, so the vhost_user_blk_disconnect will not be
> called, then the event fd callback will not be reinstalled.
>
> All vhost-user devices have this issue, including vhost-user-blk/scsi.
>
> With this patch, if the vdev->vdev is null, the fd callback will still
> be reinstalled.
>
> Fixes: 71e076a07d ("hw/virtio: generalise CHR_EVENT_CLOSED handling")
>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> Signed-off-by: Li Feng <fengli@smartx.com>
> ---
> hw/block/vhost-user-blk.c | 2 +-
> hw/scsi/vhost-user-scsi.c | 3 ++-
> hw/virtio/vhost-user-gpio.c | 2 +-
> hw/virtio/vhost-user.c | 9 +++++++--
> include/hw/virtio/vhost-user.h | 3 ++-
> 5 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 3c69fa47d5..95c758200d 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -391,7 +391,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> case CHR_EVENT_CLOSED:
> /* defer close until later to avoid circular close */
> vhost_user_async_close(dev, &s->chardev, &s->dev,
> - vhost_user_blk_disconnect);
> + vhost_user_blk_disconnect, vhost_user_blk_event);
> break;
> case CHR_EVENT_BREAK:
> case CHR_EVENT_MUX_IN:
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index a7fa8e8df2..e931df9f5b 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -236,7 +236,8 @@ static void vhost_user_scsi_event(void *opaque, QEMUChrEvent event)
> case CHR_EVENT_CLOSED:
> /* defer close until later to avoid circular close */
> vhost_user_async_close(dev, &vs->conf.chardev, &vsc->dev,
> - vhost_user_scsi_disconnect);
> + vhost_user_scsi_disconnect,
> + vhost_user_scsi_event);
> break;
> case CHR_EVENT_BREAK:
> case CHR_EVENT_MUX_IN:
> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> index d9979aa5db..04c2cc79f4 100644
> --- a/hw/virtio/vhost-user-gpio.c
> +++ b/hw/virtio/vhost-user-gpio.c
> @@ -283,7 +283,7 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent event)
> case CHR_EVENT_CLOSED:
> /* defer close until later to avoid circular close */
> vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
> - vu_gpio_disconnect);
> + vu_gpio_disconnect, vu_gpio_event);
> break;
> case CHR_EVENT_BREAK:
> case CHR_EVENT_MUX_IN:
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 8dcf049d42..12c4a41f30 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -2643,6 +2643,7 @@ typedef struct {
> DeviceState *dev;
> CharBackend *cd;
> struct vhost_dev *vhost;
> + IOEventHandler *event_cb;
> } VhostAsyncCallback;
>
> static void vhost_user_async_close_bh(void *opaque)
> @@ -2657,7 +2658,10 @@ static void vhost_user_async_close_bh(void *opaque)
> */
> if (vhost->vdev) {
> data->cb(data->dev);
> - }
> + } else if (data->event_cb) {
> + qemu_chr_fe_set_handlers(data->cd, NULL, NULL, data->event_cb,
> + NULL, data->dev, NULL, true);
> + }
>
> g_free(data);
> }
> @@ -2669,7 +2673,8 @@ static void vhost_user_async_close_bh(void *opaque)
> */
> void vhost_user_async_close(DeviceState *d,
> CharBackend *chardev, struct vhost_dev *vhost,
> - vu_async_close_fn cb)
> + vu_async_close_fn cb,
> + IOEventHandler *event_cb)
> {
> if (!runstate_check(RUN_STATE_SHUTDOWN)) {
> /*
> diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> index 191216a74f..649e9dd54f 100644
> --- a/include/hw/virtio/vhost-user.h
> +++ b/include/hw/virtio/vhost-user.h
> @@ -84,6 +84,7 @@ typedef void (*vu_async_close_fn)(DeviceState *cb);
>
> void vhost_user_async_close(DeviceState *d,
> CharBackend *chardev, struct vhost_dev *vhost,
> - vu_async_close_fn cb);
> + vu_async_close_fn cb,
> + IOEventHandler *event_cb);
>
> #endif
> --
> 2.41.0
>
>
Add a Error parameter to report the real error, like vhost-user-blk.
Signed-off-by: Li Feng <fengli@smartx.com>
---
hw/scsi/vhost-scsi-common.c | 16 +++++++++-------
hw/scsi/vhost-scsi.c | 5 +++--
hw/scsi/vhost-user-scsi.c | 14 ++++++++------
include/hw/virtio/vhost-scsi-common.h | 2 +-
4 files changed, 21 insertions(+), 16 deletions(-)
diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
index a61cd0e907..4c8637045d 100644
--- a/hw/scsi/vhost-scsi-common.c
+++ b/hw/scsi/vhost-scsi-common.c
@@ -16,6 +16,7 @@
*/
#include "qemu/osdep.h"
+#include "qapi/error.h"
#include "qemu/error-report.h"
#include "qemu/module.h"
#include "hw/virtio/vhost.h"
@@ -25,7 +26,7 @@
#include "hw/virtio/virtio-access.h"
#include "hw/fw-path-provider.h"
-int vhost_scsi_common_start(VHostSCSICommon *vsc)
+int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp)
{
int ret, i;
VirtIODevice *vdev = VIRTIO_DEVICE(vsc);
@@ -35,18 +36,19 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
VirtIOSCSICommon *vs = (VirtIOSCSICommon *)vsc;
if (!k->set_guest_notifiers) {
- error_report("binding does not support guest notifiers");
+ error_setg(errp, "binding does not support guest notifiers");
return -ENOSYS;
}
ret = vhost_dev_enable_notifiers(&vsc->dev, vdev);
if (ret < 0) {
+ error_setg_errno(errp, -ret, "Error enabling host notifiers");
return ret;
}
ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, true);
if (ret < 0) {
- error_report("Error binding guest notifier");
+ error_setg_errno(errp, -ret, "Error binding guest notifier");
goto err_host_notifiers;
}
@@ -54,7 +56,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
if (ret < 0) {
- error_report("Error setting inflight format: %d", -ret);
+ error_setg_errno(errp, -ret, "Error setting inflight format");
goto err_guest_notifiers;
}
@@ -64,21 +66,21 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
vs->conf.virtqueue_size,
vsc->inflight);
if (ret < 0) {
- error_report("Error getting inflight: %d", -ret);
+ error_setg_errno(errp, -ret, "Error getting inflight");
goto err_guest_notifiers;
}
}
ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
if (ret < 0) {
- error_report("Error setting inflight: %d", -ret);
+ error_setg_errno(errp, -ret, "Error setting inflight");
goto err_guest_notifiers;
}
}
ret = vhost_dev_start(&vsc->dev, vdev, true);
if (ret < 0) {
- error_report("Error start vhost dev");
+ error_setg_errno(errp, -ret, "Error starting vhost dev");
goto err_guest_notifiers;
}
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 443f67daa4..01a3ab4277 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -75,6 +75,7 @@ static int vhost_scsi_start(VHostSCSI *s)
int ret, abi_version;
VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
const VhostOps *vhost_ops = vsc->dev.vhost_ops;
+ Error *local_err = NULL;
ret = vhost_ops->vhost_scsi_get_abi_version(&vsc->dev, &abi_version);
if (ret < 0) {
@@ -88,14 +89,14 @@ static int vhost_scsi_start(VHostSCSI *s)
return -ENOSYS;
}
- ret = vhost_scsi_common_start(vsc);
+ ret = vhost_scsi_common_start(vsc, &local_err);
if (ret < 0) {
return ret;
}
ret = vhost_scsi_set_endpoint(s);
if (ret < 0) {
- error_report("Error setting vhost-scsi endpoint");
+ error_reportf_err(local_err, "Error setting vhost-scsi endpoint");
vhost_scsi_common_stop(vsc);
}
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index e931df9f5b..62fc98bb1c 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -43,12 +43,12 @@ enum VhostUserProtocolFeature {
VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
};
-static int vhost_user_scsi_start(VHostUserSCSI *s)
+static int vhost_user_scsi_start(VHostUserSCSI *s, Error **errp)
{
VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
int ret;
- ret = vhost_scsi_common_start(vsc);
+ ret = vhost_scsi_common_start(vsc, errp);
s->started_vu = (ret < 0 ? false : true);
return ret;
@@ -73,6 +73,7 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
bool should_start = virtio_device_should_start(vdev, status);
+ Error *local_err = NULL;
int ret;
if (!s->connected) {
@@ -84,9 +85,10 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
}
if (should_start) {
- ret = vhost_user_scsi_start(s);
+ ret = vhost_user_scsi_start(s, &local_err);
if (ret < 0) {
- error_report("unable to start vhost-user-scsi: %s", strerror(-ret));
+ error_reportf_err(local_err, "unable to start vhost-user-scsi: %s",
+ strerror(-ret));
qemu_chr_fe_disconnect(&vs->conf.chardev);
}
} else {
@@ -139,7 +141,7 @@ static void vhost_user_scsi_handle_output(VirtIODevice *vdev, VirtQueue *vq)
* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
* vhost here instead of waiting for .set_status().
*/
- ret = vhost_user_scsi_start(s);
+ ret = vhost_user_scsi_start(s, &local_err);
if (ret < 0) {
error_reportf_err(local_err, "vhost-user-scsi: vhost start failed: ");
qemu_chr_fe_disconnect(&vs->conf.chardev);
@@ -184,7 +186,7 @@ static int vhost_user_scsi_connect(DeviceState *dev, Error **errp)
/* restore vhost state */
if (virtio_device_started(vdev, vdev->status)) {
- ret = vhost_user_scsi_start(s);
+ ret = vhost_user_scsi_start(s, errp);
if (ret < 0) {
return ret;
}
diff --git a/include/hw/virtio/vhost-scsi-common.h b/include/hw/virtio/vhost-scsi-common.h
index 18f115527c..c5d2c09455 100644
--- a/include/hw/virtio/vhost-scsi-common.h
+++ b/include/hw/virtio/vhost-scsi-common.h
@@ -39,7 +39,7 @@ struct VHostSCSICommon {
struct vhost_inflight *inflight;
};
-int vhost_scsi_common_start(VHostSCSICommon *vsc);
+int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp);
void vhost_scsi_common_stop(VHostSCSICommon *vsc);
char *vhost_scsi_common_get_fw_dev_path(FWPathProvider *p, BusState *bus,
DeviceState *dev);
--
2.41.0Li Feng <fengli@smartx.com> writes:
> Add a Error parameter to report the real error, like vhost-user-blk.
>
> Signed-off-by: Li Feng <fengli@smartx.com>
> ---
> hw/scsi/vhost-scsi-common.c | 16 +++++++++-------
> hw/scsi/vhost-scsi.c | 5 +++--
> hw/scsi/vhost-user-scsi.c | 14 ++++++++------
> include/hw/virtio/vhost-scsi-common.h | 2 +-
> 4 files changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
> index a61cd0e907..4c8637045d 100644
> --- a/hw/scsi/vhost-scsi-common.c
> +++ b/hw/scsi/vhost-scsi-common.c
> @@ -16,6 +16,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qapi/error.h"
> #include "qemu/error-report.h"
> #include "qemu/module.h"
> #include "hw/virtio/vhost.h"
> @@ -25,7 +26,7 @@
> #include "hw/virtio/virtio-access.h"
> #include "hw/fw-path-provider.h"
>
> -int vhost_scsi_common_start(VHostSCSICommon *vsc)
> +int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp)
> {
> int ret, i;
> VirtIODevice *vdev = VIRTIO_DEVICE(vsc);
> @@ -35,18 +36,19 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> VirtIOSCSICommon *vs = (VirtIOSCSICommon *)vsc;
>
> if (!k->set_guest_notifiers) {
> - error_report("binding does not support guest notifiers");
> + error_setg(errp, "binding does not support guest notifiers");
> return -ENOSYS;
> }
>
> ret = vhost_dev_enable_notifiers(&vsc->dev, vdev);
> if (ret < 0) {
> + error_setg_errno(errp, -ret, "Error enabling host notifiers");
Looks like the error is silent before your patch. Correct?
> return ret;
> }
>
> ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, true);
> if (ret < 0) {
> - error_report("Error binding guest notifier");
> + error_setg_errno(errp, -ret, "Error binding guest notifier");
Error message now provides more detail.
> goto err_host_notifiers;
> }
>
> @@ -54,7 +56,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
>
> ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
> if (ret < 0) {
> - error_report("Error setting inflight format: %d", -ret);
> + error_setg_errno(errp, -ret, "Error setting inflight format");
Error message now shows errno in human-readable form.
> goto err_guest_notifiers;
> }
>
> @@ -64,21 +66,21 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> vs->conf.virtqueue_size,
> vsc->inflight);
> if (ret < 0) {
> - error_report("Error getting inflight: %d", -ret);
> + error_setg_errno(errp, -ret, "Error getting inflight");
Likewise.
> goto err_guest_notifiers;
> }
> }
>
> ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
> if (ret < 0) {
> - error_report("Error setting inflight: %d", -ret);
> + error_setg_errno(errp, -ret, "Error setting inflight");
Likewise.
> goto err_guest_notifiers;
> }
> }
>
> ret = vhost_dev_start(&vsc->dev, vdev, true);
> if (ret < 0) {
> - error_report("Error start vhost dev");
> + error_setg_errno(errp, -ret, "Error starting vhost dev");
Likewise.
> goto err_guest_notifiers;
> }
>
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 443f67daa4..01a3ab4277 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -75,6 +75,7 @@ static int vhost_scsi_start(VHostSCSI *s)
> int ret, abi_version;
> VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
> const VhostOps *vhost_ops = vsc->dev.vhost_ops;
> + Error *local_err = NULL;
>
> ret = vhost_ops->vhost_scsi_get_abi_version(&vsc->dev, &abi_version);
> if (ret < 0) {
> @@ -88,14 +89,14 @@ static int vhost_scsi_start(VHostSCSI *s)
> return -ENOSYS;
> }
>
> - ret = vhost_scsi_common_start(vsc);
> + ret = vhost_scsi_common_start(vsc, &local_err);
> if (ret < 0) {
> return ret;
> }
>
> ret = vhost_scsi_set_endpoint(s);
> if (ret < 0) {
> - error_report("Error setting vhost-scsi endpoint");
> + error_reportf_err(local_err, "Error setting vhost-scsi endpoint");
> vhost_scsi_common_stop(vsc);
> }
>
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index e931df9f5b..62fc98bb1c 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -43,12 +43,12 @@ enum VhostUserProtocolFeature {
> VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
> };
>
> -static int vhost_user_scsi_start(VHostUserSCSI *s)
> +static int vhost_user_scsi_start(VHostUserSCSI *s, Error **errp)
> {
> VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
> int ret;
>
> - ret = vhost_scsi_common_start(vsc);
> + ret = vhost_scsi_common_start(vsc, errp);
> s->started_vu = (ret < 0 ? false : true);
>
> return ret;
> @@ -73,6 +73,7 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
> VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
> VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
> bool should_start = virtio_device_should_start(vdev, status);
> + Error *local_err = NULL;
> int ret;
>
> if (!s->connected) {
> @@ -84,9 +85,10 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
> }
>
> if (should_start) {
> - ret = vhost_user_scsi_start(s);
> + ret = vhost_user_scsi_start(s, &local_err);
> if (ret < 0) {
> - error_report("unable to start vhost-user-scsi: %s", strerror(-ret));
> + error_reportf_err(local_err, "unable to start vhost-user-scsi: %s",
> + strerror(-ret));
> qemu_chr_fe_disconnect(&vs->conf.chardev);
> }
> } else {
> @@ -139,7 +141,7 @@ static void vhost_user_scsi_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> * Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
> * vhost here instead of waiting for .set_status().
> */
> - ret = vhost_user_scsi_start(s);
> + ret = vhost_user_scsi_start(s, &local_err);
> if (ret < 0) {
> error_reportf_err(local_err, "vhost-user-scsi: vhost start failed: ");
The error_reportf_err() is wrong before the patch, as I just pointed out
in my review of "[PATCH v3 5/5] vhost-user-scsi: start vhost when guest
kicks". It is correct afterwards.
> qemu_chr_fe_disconnect(&vs->conf.chardev);
> @@ -184,7 +186,7 @@ static int vhost_user_scsi_connect(DeviceState *dev, Error **errp)
>
> /* restore vhost state */
> if (virtio_device_started(vdev, vdev->status)) {
> - ret = vhost_user_scsi_start(s);
> + ret = vhost_user_scsi_start(s, errp);
> if (ret < 0) {
> return ret;
> }
Wrong before the patch, as I just pointed out in my review of "[PATCH v3
4/5] vhost-user-scsi: support reconnect to backend". Correct afterwards.
> diff --git a/include/hw/virtio/vhost-scsi-common.h b/include/hw/virtio/vhost-scsi-common.h
> index 18f115527c..c5d2c09455 100644
> --- a/include/hw/virtio/vhost-scsi-common.h
> +++ b/include/hw/virtio/vhost-scsi-common.h
> @@ -39,7 +39,7 @@ struct VHostSCSICommon {
> struct vhost_inflight *inflight;
> };
>
> -int vhost_scsi_common_start(VHostSCSICommon *vsc);
> +int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp);
> void vhost_scsi_common_stop(VHostSCSICommon *vsc);
> char *vhost_scsi_common_get_fw_dev_path(FWPathProvider *p, BusState *bus,
> DeviceState *dev);
Please mention in the commit message that error messages improve, and
silent errors are now reported.
> On 1 Sep 2023, at 8:00 PM, Markus Armbruster <armbru@redhat.com> wrote:
>
> Li Feng <fengli@smartx.com <mailto:fengli@smartx.com>> writes:
>
>> Add a Error parameter to report the real error, like vhost-user-blk.
>>
>> Signed-off-by: Li Feng <fengli@smartx.com>
>> ---
>> hw/scsi/vhost-scsi-common.c | 16 +++++++++-------
>> hw/scsi/vhost-scsi.c | 5 +++--
>> hw/scsi/vhost-user-scsi.c | 14 ++++++++------
>> include/hw/virtio/vhost-scsi-common.h | 2 +-
>> 4 files changed, 21 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
>> index a61cd0e907..4c8637045d 100644
>> --- a/hw/scsi/vhost-scsi-common.c
>> +++ b/hw/scsi/vhost-scsi-common.c
>> @@ -16,6 +16,7 @@
>> */
>>
>> #include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> #include "qemu/error-report.h"
>> #include "qemu/module.h"
>> #include "hw/virtio/vhost.h"
>> @@ -25,7 +26,7 @@
>> #include "hw/virtio/virtio-access.h"
>> #include "hw/fw-path-provider.h"
>>
>> -int vhost_scsi_common_start(VHostSCSICommon *vsc)
>> +int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp)
>> {
>> int ret, i;
>> VirtIODevice *vdev = VIRTIO_DEVICE(vsc);
>> @@ -35,18 +36,19 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
>> VirtIOSCSICommon *vs = (VirtIOSCSICommon *)vsc;
>>
>> if (!k->set_guest_notifiers) {
>> - error_report("binding does not support guest notifiers");
>> + error_setg(errp, "binding does not support guest notifiers");
>> return -ENOSYS;
>> }
>>
>> ret = vhost_dev_enable_notifiers(&vsc->dev, vdev);
>> if (ret < 0) {
>> + error_setg_errno(errp, -ret, "Error enabling host notifiers");
>
> Looks like the error is silent before your patch. Correct?
Yes, before this patch, it’s a silent error.
>
>> return ret;
>> }
>>
>> ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, true);
>> if (ret < 0) {
>> - error_report("Error binding guest notifier");
>> + error_setg_errno(errp, -ret, "Error binding guest notifier");
>
> Error message now provides more detail.
Yes.
>
>> goto err_host_notifiers;
>> }
>>
>> @@ -54,7 +56,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
>>
>> ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
>> if (ret < 0) {
>> - error_report("Error setting inflight format: %d", -ret);
>> + error_setg_errno(errp, -ret, "Error setting inflight format");
>
> Error message now shows errno in human-readable form.
Yes.
>
>> goto err_guest_notifiers;
>> }
>>
>> @@ -64,21 +66,21 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
>> vs->conf.virtqueue_size,
>> vsc->inflight);
>> if (ret < 0) {
>> - error_report("Error getting inflight: %d", -ret);
>> + error_setg_errno(errp, -ret, "Error getting inflight");
>
> Likewise.
Yes.
>
>> goto err_guest_notifiers;
>> }
>> }
>>
>> ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
>> if (ret < 0) {
>> - error_report("Error setting inflight: %d", -ret);
>> + error_setg_errno(errp, -ret, "Error setting inflight");
>
> Likewise.
Yes.
>
>> goto err_guest_notifiers;
>> }
>> }
>>
>> ret = vhost_dev_start(&vsc->dev, vdev, true);
>> if (ret < 0) {
>> - error_report("Error start vhost dev");
>> + error_setg_errno(errp, -ret, "Error starting vhost dev");
>
> Likewise.
Yes.
>
>> goto err_guest_notifiers;
>> }
>>
>> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
>> index 443f67daa4..01a3ab4277 100644
>> --- a/hw/scsi/vhost-scsi.c
>> +++ b/hw/scsi/vhost-scsi.c
>> @@ -75,6 +75,7 @@ static int vhost_scsi_start(VHostSCSI *s)
>> int ret, abi_version;
>> VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
>> const VhostOps *vhost_ops = vsc->dev.vhost_ops;
>> + Error *local_err = NULL;
>>
>> ret = vhost_ops->vhost_scsi_get_abi_version(&vsc->dev, &abi_version);
>> if (ret < 0) {
>> @@ -88,14 +89,14 @@ static int vhost_scsi_start(VHostSCSI *s)
>> return -ENOSYS;
>> }
>>
>> - ret = vhost_scsi_common_start(vsc);
>> + ret = vhost_scsi_common_start(vsc, &local_err);
>> if (ret < 0) {
>> return ret;
>> }
>>
>> ret = vhost_scsi_set_endpoint(s);
>> if (ret < 0) {
>> - error_report("Error setting vhost-scsi endpoint");
>> + error_reportf_err(local_err, "Error setting vhost-scsi endpoint");
>> vhost_scsi_common_stop(vsc);
>> }
>>
>> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
>> index e931df9f5b..62fc98bb1c 100644
>> --- a/hw/scsi/vhost-user-scsi.c
>> +++ b/hw/scsi/vhost-user-scsi.c
>> @@ -43,12 +43,12 @@ enum VhostUserProtocolFeature {
>> VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
>> };
>>
>> -static int vhost_user_scsi_start(VHostUserSCSI *s)
>> +static int vhost_user_scsi_start(VHostUserSCSI *s, Error **errp)
>> {
>> VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
>> int ret;
>>
>> - ret = vhost_scsi_common_start(vsc);
>> + ret = vhost_scsi_common_start(vsc, errp);
>> s->started_vu = (ret < 0 ? false : true);
>>
>> return ret;
>> @@ -73,6 +73,7 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
>> VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
>> VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
>> bool should_start = virtio_device_should_start(vdev, status);
>> + Error *local_err = NULL;
>> int ret;
>>
>> if (!s->connected) {
>> @@ -84,9 +85,10 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
>> }
>>
>> if (should_start) {
>> - ret = vhost_user_scsi_start(s);
>> + ret = vhost_user_scsi_start(s, &local_err);
>> if (ret < 0) {
>> - error_report("unable to start vhost-user-scsi: %s", strerror(-ret));
>> + error_reportf_err(local_err, "unable to start vhost-user-scsi: %s",
>> + strerror(-ret));
>> qemu_chr_fe_disconnect(&vs->conf.chardev);
>> }
>> } else {
>> @@ -139,7 +141,7 @@ static void vhost_user_scsi_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>> * Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
>> * vhost here instead of waiting for .set_status().
>> */
>> - ret = vhost_user_scsi_start(s);
>> + ret = vhost_user_scsi_start(s, &local_err);
>> if (ret < 0) {
>> error_reportf_err(local_err, "vhost-user-scsi: vhost start failed: ");
>
> The error_reportf_err() is wrong before the patch, as I just pointed out
> in my review of "[PATCH v3 5/5] vhost-user-scsi: start vhost when guest
> kicks". It is correct afterwards.
Yes.
>
>> qemu_chr_fe_disconnect(&vs->conf.chardev);
>> @@ -184,7 +186,7 @@ static int vhost_user_scsi_connect(DeviceState *dev, Error **errp)
>>
>> /* restore vhost state */
>> if (virtio_device_started(vdev, vdev->status)) {
>> - ret = vhost_user_scsi_start(s);
>> + ret = vhost_user_scsi_start(s, errp);
>> if (ret < 0) {
>> return ret;
>> }
>
> Wrong before the patch, as I just pointed out in my review of "[PATCH v3
> 4/5] vhost-user-scsi: support reconnect to backend". Correct afterwards.
Yes.
>
>> diff --git a/include/hw/virtio/vhost-scsi-common.h b/include/hw/virtio/vhost-scsi-common.h
>> index 18f115527c..c5d2c09455 100644
>> --- a/include/hw/virtio/vhost-scsi-common.h
>> +++ b/include/hw/virtio/vhost-scsi-common.h
>> @@ -39,7 +39,7 @@ struct VHostSCSICommon {
>> struct vhost_inflight *inflight;
>> };
>>
>> -int vhost_scsi_common_start(VHostSCSICommon *vsc);
>> +int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp);
>> void vhost_scsi_common_stop(VHostSCSICommon *vsc);
>> char *vhost_scsi_common_get_fw_dev_path(FWPathProvider *p, BusState *bus,
>> DeviceState *dev);
>
> Please mention in the commit message that error messages improve, and
> silent errors are now reported.
Ack.
On Tue, Sep 12, 2023 at 04:32:59PM +0800, Li Feng wrote: > Please mention in the commit message that error messages improve, and > silent errors are now reported. > > Ack. Still waiting for v4 with the updated commit log. -- MST
> On Aug 30, 2023, at 12:57 AM, Li Feng <fengli@smartx.com> wrote:
>
> Add a Error parameter to report the real error, like vhost-user-blk.
>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> Signed-off-by: Li Feng <fengli@smartx.com>
> ---
> hw/scsi/vhost-scsi-common.c | 16 +++++++++-------
> hw/scsi/vhost-scsi.c | 5 +++--
> hw/scsi/vhost-user-scsi.c | 14 ++++++++------
> include/hw/virtio/vhost-scsi-common.h | 2 +-
> 4 files changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
> index a61cd0e907..4c8637045d 100644
> --- a/hw/scsi/vhost-scsi-common.c
> +++ b/hw/scsi/vhost-scsi-common.c
> @@ -16,6 +16,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qapi/error.h"
> #include "qemu/error-report.h"
> #include "qemu/module.h"
> #include "hw/virtio/vhost.h"
> @@ -25,7 +26,7 @@
> #include "hw/virtio/virtio-access.h"
> #include "hw/fw-path-provider.h"
>
> -int vhost_scsi_common_start(VHostSCSICommon *vsc)
> +int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp)
> {
> int ret, i;
> VirtIODevice *vdev = VIRTIO_DEVICE(vsc);
> @@ -35,18 +36,19 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> VirtIOSCSICommon *vs = (VirtIOSCSICommon *)vsc;
>
> if (!k->set_guest_notifiers) {
> - error_report("binding does not support guest notifiers");
> + error_setg(errp, "binding does not support guest notifiers");
> return -ENOSYS;
> }
>
> ret = vhost_dev_enable_notifiers(&vsc->dev, vdev);
> if (ret < 0) {
> + error_setg_errno(errp, -ret, "Error enabling host notifiers");
> return ret;
> }
>
> ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, true);
> if (ret < 0) {
> - error_report("Error binding guest notifier");
> + error_setg_errno(errp, -ret, "Error binding guest notifier");
> goto err_host_notifiers;
> }
>
> @@ -54,7 +56,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
>
> ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
> if (ret < 0) {
> - error_report("Error setting inflight format: %d", -ret);
> + error_setg_errno(errp, -ret, "Error setting inflight format");
> goto err_guest_notifiers;
> }
>
> @@ -64,21 +66,21 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> vs->conf.virtqueue_size,
> vsc->inflight);
> if (ret < 0) {
> - error_report("Error getting inflight: %d", -ret);
> + error_setg_errno(errp, -ret, "Error getting inflight");
> goto err_guest_notifiers;
> }
> }
>
> ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
> if (ret < 0) {
> - error_report("Error setting inflight: %d", -ret);
> + error_setg_errno(errp, -ret, "Error setting inflight");
> goto err_guest_notifiers;
> }
> }
>
> ret = vhost_dev_start(&vsc->dev, vdev, true);
> if (ret < 0) {
> - error_report("Error start vhost dev");
> + error_setg_errno(errp, -ret, "Error starting vhost dev");
> goto err_guest_notifiers;
> }
>
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 443f67daa4..01a3ab4277 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -75,6 +75,7 @@ static int vhost_scsi_start(VHostSCSI *s)
> int ret, abi_version;
> VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
> const VhostOps *vhost_ops = vsc->dev.vhost_ops;
> + Error *local_err = NULL;
>
> ret = vhost_ops->vhost_scsi_get_abi_version(&vsc->dev, &abi_version);
> if (ret < 0) {
> @@ -88,14 +89,14 @@ static int vhost_scsi_start(VHostSCSI *s)
> return -ENOSYS;
> }
>
> - ret = vhost_scsi_common_start(vsc);
> + ret = vhost_scsi_common_start(vsc, &local_err);
> if (ret < 0) {
> return ret;
> }
>
> ret = vhost_scsi_set_endpoint(s);
> if (ret < 0) {
> - error_report("Error setting vhost-scsi endpoint");
> + error_reportf_err(local_err, "Error setting vhost-scsi endpoint");
> vhost_scsi_common_stop(vsc);
> }
>
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index e931df9f5b..62fc98bb1c 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -43,12 +43,12 @@ enum VhostUserProtocolFeature {
> VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
> };
>
> -static int vhost_user_scsi_start(VHostUserSCSI *s)
> +static int vhost_user_scsi_start(VHostUserSCSI *s, Error **errp)
> {
> VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
> int ret;
>
> - ret = vhost_scsi_common_start(vsc);
> + ret = vhost_scsi_common_start(vsc, errp);
> s->started_vu = (ret < 0 ? false : true);
>
> return ret;
> @@ -73,6 +73,7 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
> VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
> VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
> bool should_start = virtio_device_should_start(vdev, status);
> + Error *local_err = NULL;
> int ret;
>
> if (!s->connected) {
> @@ -84,9 +85,10 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
> }
>
> if (should_start) {
> - ret = vhost_user_scsi_start(s);
> + ret = vhost_user_scsi_start(s, &local_err);
> if (ret < 0) {
> - error_report("unable to start vhost-user-scsi: %s", strerror(-ret));
> + error_reportf_err(local_err, "unable to start vhost-user-scsi: %s",
> + strerror(-ret));
> qemu_chr_fe_disconnect(&vs->conf.chardev);
> }
> } else {
> @@ -139,7 +141,7 @@ static void vhost_user_scsi_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> * Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
> * vhost here instead of waiting for .set_status().
> */
> - ret = vhost_user_scsi_start(s);
> + ret = vhost_user_scsi_start(s, &local_err);
> if (ret < 0) {
> error_reportf_err(local_err, "vhost-user-scsi: vhost start failed: ");
> qemu_chr_fe_disconnect(&vs->conf.chardev);
> @@ -184,7 +186,7 @@ static int vhost_user_scsi_connect(DeviceState *dev, Error **errp)
>
> /* restore vhost state */
> if (virtio_device_started(vdev, vdev->status)) {
> - ret = vhost_user_scsi_start(s);
> + ret = vhost_user_scsi_start(s, errp);
> if (ret < 0) {
> return ret;
> }
> diff --git a/include/hw/virtio/vhost-scsi-common.h b/include/hw/virtio/vhost-scsi-common.h
> index 18f115527c..c5d2c09455 100644
> --- a/include/hw/virtio/vhost-scsi-common.h
> +++ b/include/hw/virtio/vhost-scsi-common.h
> @@ -39,7 +39,7 @@ struct VHostSCSICommon {
> struct vhost_inflight *inflight;
> };
>
> -int vhost_scsi_common_start(VHostSCSICommon *vsc);
> +int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp);
> void vhost_scsi_common_stop(VHostSCSICommon *vsc);
> char *vhost_scsi_common_get_fw_dev_path(FWPathProvider *p, BusState *bus,
> DeviceState *dev);
> --
> 2.41.0
>
The patchset fixes the regression issue of vhost reconnect. It's a serious bug that the vhost-user will lose the reconnect forever. The 2nd patch enhances the error handle of vhost-user-scsi. This patchset's parent commit is: https://lore.kernel.org/all/20230731121018.2856310-1-fengli@smartx.com/ Changes for v2: - Add a event_cb in VhostAsyncCallback to be called when dev is NULL; - Fix the error report message. Li Feng (2): vhost-user: Fix lost reconnect vhost: Add Error parameter to vhost_scsi_common_start() hw/block/vhost-user-blk.c | 2 +- hw/scsi/vhost-scsi-common.c | 16 +++++++++------- hw/scsi/vhost-scsi.c | 5 +++-- hw/scsi/vhost-user-scsi.c | 17 ++++++++++------- hw/virtio/vhost-user-gpio.c | 2 +- hw/virtio/vhost-user.c | 10 ++++++++-- include/hw/virtio/vhost-scsi-common.h | 2 +- include/hw/virtio/vhost-user.h | 4 +++- 8 files changed, 36 insertions(+), 22 deletions(-) -- 2.41.0
When the vhost-user is reconnecting to the backend, and if the vhost-user fails
at the get_features in vhost_dev_init(), then the reconnect will fail
and it will not be retriggered forever.
The reason is:
When the vhost-user fails at get_features, the vhost_dev_cleanup will be called
immediately.
vhost_dev_cleanup calls 'memset(hdev, 0, sizeof(struct vhost_dev))'.
The reconnect path is:
vhost_user_blk_event
vhost_user_async_close(.. vhost_user_blk_disconnect ..)
qemu_chr_fe_set_handlers <----- clear the notifier callback
schedule vhost_user_async_close_bh
The vhost->vdev is null, so the vhost_user_blk_disconnect will not be
called, then the event fd callback will not be reinstalled.
All vhost-user devices have this issue, including vhost-user-blk/scsi.
With this patch, if the vdev->vdev is null, the fd callback will still
be reinstalled.
Fixes: 71e076a07d ("hw/virtio: generalise CHR_EVENT_CLOSED handling")
Signed-off-by: Li Feng <fengli@smartx.com>
---
hw/block/vhost-user-blk.c | 2 +-
hw/scsi/vhost-user-scsi.c | 3 ++-
hw/virtio/vhost-user-gpio.c | 2 +-
hw/virtio/vhost-user.c | 10 ++++++++--
include/hw/virtio/vhost-user.h | 4 +++-
5 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 3c69fa47d5..95c758200d 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -391,7 +391,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
case CHR_EVENT_CLOSED:
/* defer close until later to avoid circular close */
vhost_user_async_close(dev, &s->chardev, &s->dev,
- vhost_user_blk_disconnect);
+ vhost_user_blk_disconnect, vhost_user_blk_event);
break;
case CHR_EVENT_BREAK:
case CHR_EVENT_MUX_IN:
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index a7fa8e8df2..e931df9f5b 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -236,7 +236,8 @@ static void vhost_user_scsi_event(void *opaque, QEMUChrEvent event)
case CHR_EVENT_CLOSED:
/* defer close until later to avoid circular close */
vhost_user_async_close(dev, &vs->conf.chardev, &vsc->dev,
- vhost_user_scsi_disconnect);
+ vhost_user_scsi_disconnect,
+ vhost_user_scsi_event);
break;
case CHR_EVENT_BREAK:
case CHR_EVENT_MUX_IN:
diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index d9979aa5db..04c2cc79f4 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -283,7 +283,7 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent event)
case CHR_EVENT_CLOSED:
/* defer close until later to avoid circular close */
vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
- vu_gpio_disconnect);
+ vu_gpio_disconnect, vu_gpio_event);
break;
case CHR_EVENT_BREAK:
case CHR_EVENT_MUX_IN:
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 8dcf049d42..9540766dd3 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2643,6 +2643,7 @@ typedef struct {
DeviceState *dev;
CharBackend *cd;
struct vhost_dev *vhost;
+ IOEventHandler *event_cb;
} VhostAsyncCallback;
static void vhost_user_async_close_bh(void *opaque)
@@ -2657,7 +2658,10 @@ static void vhost_user_async_close_bh(void *opaque)
*/
if (vhost->vdev) {
data->cb(data->dev);
- }
+ } else if (data->event_cb) {
+ qemu_chr_fe_set_handlers(data->cd, NULL, NULL, data->event_cb,
+ NULL, data->dev, NULL, true);
+ }
g_free(data);
}
@@ -2669,7 +2673,9 @@ static void vhost_user_async_close_bh(void *opaque)
*/
void vhost_user_async_close(DeviceState *d,
CharBackend *chardev, struct vhost_dev *vhost,
- vu_async_close_fn cb)
+ vu_async_close_fn cb,
+ IOEventHandler *event_cb
+ )
{
if (!runstate_check(RUN_STATE_SHUTDOWN)) {
/*
diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
index 191216a74f..5fdc711d4e 100644
--- a/include/hw/virtio/vhost-user.h
+++ b/include/hw/virtio/vhost-user.h
@@ -84,6 +84,8 @@ typedef void (*vu_async_close_fn)(DeviceState *cb);
void vhost_user_async_close(DeviceState *d,
CharBackend *chardev, struct vhost_dev *vhost,
- vu_async_close_fn cb);
+ vu_async_close_fn cb,
+ IOEventHandler *event_cb
+ );
#endif
--
2.41.0
> On Aug 24, 2023, at 3:41 AM, Li Feng <fengli@smartx.com> wrote:
>
> When the vhost-user is reconnecting to the backend, and if the vhost-user fails
> at the get_features in vhost_dev_init(), then the reconnect will fail
> and it will not be retriggered forever.
>
> The reason is:
> When the vhost-user fails at get_features, the vhost_dev_cleanup will be called
> immediately.
>
> vhost_dev_cleanup calls 'memset(hdev, 0, sizeof(struct vhost_dev))'.
>
> The reconnect path is:
> vhost_user_blk_event
> vhost_user_async_close(.. vhost_user_blk_disconnect ..)
> qemu_chr_fe_set_handlers <----- clear the notifier callback
> schedule vhost_user_async_close_bh
>
> The vhost->vdev is null, so the vhost_user_blk_disconnect will not be
> called, then the event fd callback will not be reinstalled.
>
> All vhost-user devices have this issue, including vhost-user-blk/scsi.
>
> With this patch, if the vdev->vdev is null, the fd callback will still
> be reinstalled.
>
> Fixes: 71e076a07d ("hw/virtio: generalise CHR_EVENT_CLOSED handling")
>
A couple of NITs, otherwise LGTM
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> Signed-off-by: Li Feng <fengli@smartx.com>
> ---
> hw/block/vhost-user-blk.c | 2 +-
> hw/scsi/vhost-user-scsi.c | 3 ++-
> hw/virtio/vhost-user-gpio.c | 2 +-
> hw/virtio/vhost-user.c | 10 ++++++++--
> include/hw/virtio/vhost-user.h | 4 +++-
> 5 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 3c69fa47d5..95c758200d 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -391,7 +391,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> case CHR_EVENT_CLOSED:
> /* defer close until later to avoid circular close */
> vhost_user_async_close(dev, &s->chardev, &s->dev,
> - vhost_user_blk_disconnect);
> + vhost_user_blk_disconnect, vhost_user_blk_event);
> break;
> case CHR_EVENT_BREAK:
> case CHR_EVENT_MUX_IN:
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index a7fa8e8df2..e931df9f5b 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -236,7 +236,8 @@ static void vhost_user_scsi_event(void *opaque, QEMUChrEvent event)
> case CHR_EVENT_CLOSED:
> /* defer close until later to avoid circular close */
> vhost_user_async_close(dev, &vs->conf.chardev, &vsc->dev,
> - vhost_user_scsi_disconnect);
> + vhost_user_scsi_disconnect,
> + vhost_user_scsi_event);
> break;
> case CHR_EVENT_BREAK:
> case CHR_EVENT_MUX_IN:
> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> index d9979aa5db..04c2cc79f4 100644
> --- a/hw/virtio/vhost-user-gpio.c
> +++ b/hw/virtio/vhost-user-gpio.c
> @@ -283,7 +283,7 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent event)
> case CHR_EVENT_CLOSED:
> /* defer close until later to avoid circular close */
> vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
> - vu_gpio_disconnect);
> + vu_gpio_disconnect, vu_gpio_event);
> break;
> case CHR_EVENT_BREAK:
> case CHR_EVENT_MUX_IN:
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 8dcf049d42..9540766dd3 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -2643,6 +2643,7 @@ typedef struct {
> DeviceState *dev;
> CharBackend *cd;
> struct vhost_dev *vhost;
> + IOEventHandler *event_cb;
> } VhostAsyncCallback;
>
> static void vhost_user_async_close_bh(void *opaque)
> @@ -2657,7 +2658,10 @@ static void vhost_user_async_close_bh(void *opaque)
> */
> if (vhost->vdev) {
> data->cb(data->dev);
> - }
> + } else if (data->event_cb) {
> + qemu_chr_fe_set_handlers(data->cd, NULL, NULL, data->event_cb,
> + NULL, data->dev, NULL, true);
> + }
>
> g_free(data);
> }
> @@ -2669,7 +2673,9 @@ static void vhost_user_async_close_bh(void *opaque)
> */
> void vhost_user_async_close(DeviceState *d,
> CharBackend *chardev, struct vhost_dev *vhost,
> - vu_async_close_fn cb)
> + vu_async_close_fn cb,
> + IOEventHandler *event_cb
Nit: why the newline before the closing parenthesis?
> + )
> {
> if (!runstate_check(RUN_STATE_SHUTDOWN)) {
> /*
> diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> index 191216a74f..5fdc711d4e 100644
> --- a/include/hw/virtio/vhost-user.h
> +++ b/include/hw/virtio/vhost-user.h
> @@ -84,6 +84,8 @@ typedef void (*vu_async_close_fn)(DeviceState *cb);
>
> void vhost_user_async_close(DeviceState *d,
> CharBackend *chardev, struct vhost_dev *vhost,
> - vu_async_close_fn cb);
> + vu_async_close_fn cb,
> + IOEventHandler *event_cb
Nit: ditto - don’t think we need this newline before );
> + );
>
> #endif
> --
> 2.41.0
>
> On 30 Aug 2023, at 6:11 AM, Raphael Norwitz <raphael.norwitz@nutanix.com> wrote:
>
>
>
>> On Aug 24, 2023, at 3:41 AM, Li Feng <fengli@smartx.com> wrote:
>>
>> When the vhost-user is reconnecting to the backend, and if the vhost-user fails
>> at the get_features in vhost_dev_init(), then the reconnect will fail
>> and it will not be retriggered forever.
>>
>> The reason is:
>> When the vhost-user fails at get_features, the vhost_dev_cleanup will be called
>> immediately.
>>
>> vhost_dev_cleanup calls 'memset(hdev, 0, sizeof(struct vhost_dev))'.
>>
>> The reconnect path is:
>> vhost_user_blk_event
>> vhost_user_async_close(.. vhost_user_blk_disconnect ..)
>> qemu_chr_fe_set_handlers <----- clear the notifier callback
>> schedule vhost_user_async_close_bh
>>
>> The vhost->vdev is null, so the vhost_user_blk_disconnect will not be
>> called, then the event fd callback will not be reinstalled.
>>
>> All vhost-user devices have this issue, including vhost-user-blk/scsi.
>>
>> With this patch, if the vdev->vdev is null, the fd callback will still
>> be reinstalled.
>>
>> Fixes: 71e076a07d ("hw/virtio: generalise CHR_EVENT_CLOSED handling")
>>
>
> A couple of NITs, otherwise LGTM
>
> Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com <mailto:raphael.norwitz@nutanix.com>>
>
>> Signed-off-by: Li Feng <fengli@smartx.com>
>> ---
>> hw/block/vhost-user-blk.c | 2 +-
>> hw/scsi/vhost-user-scsi.c | 3 ++-
>> hw/virtio/vhost-user-gpio.c | 2 +-
>> hw/virtio/vhost-user.c | 10 ++++++++--
>> include/hw/virtio/vhost-user.h | 4 +++-
>> 5 files changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>> index 3c69fa47d5..95c758200d 100644
>> --- a/hw/block/vhost-user-blk.c
>> +++ b/hw/block/vhost-user-blk.c
>> @@ -391,7 +391,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
>> case CHR_EVENT_CLOSED:
>> /* defer close until later to avoid circular close */
>> vhost_user_async_close(dev, &s->chardev, &s->dev,
>> - vhost_user_blk_disconnect);
>> + vhost_user_blk_disconnect, vhost_user_blk_event);
>> break;
>> case CHR_EVENT_BREAK:
>> case CHR_EVENT_MUX_IN:
>> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
>> index a7fa8e8df2..e931df9f5b 100644
>> --- a/hw/scsi/vhost-user-scsi.c
>> +++ b/hw/scsi/vhost-user-scsi.c
>> @@ -236,7 +236,8 @@ static void vhost_user_scsi_event(void *opaque, QEMUChrEvent event)
>> case CHR_EVENT_CLOSED:
>> /* defer close until later to avoid circular close */
>> vhost_user_async_close(dev, &vs->conf.chardev, &vsc->dev,
>> - vhost_user_scsi_disconnect);
>> + vhost_user_scsi_disconnect,
>> + vhost_user_scsi_event);
>> break;
>> case CHR_EVENT_BREAK:
>> case CHR_EVENT_MUX_IN:
>> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
>> index d9979aa5db..04c2cc79f4 100644
>> --- a/hw/virtio/vhost-user-gpio.c
>> +++ b/hw/virtio/vhost-user-gpio.c
>> @@ -283,7 +283,7 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent event)
>> case CHR_EVENT_CLOSED:
>> /* defer close until later to avoid circular close */
>> vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
>> - vu_gpio_disconnect);
>> + vu_gpio_disconnect, vu_gpio_event);
>> break;
>> case CHR_EVENT_BREAK:
>> case CHR_EVENT_MUX_IN:
>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>> index 8dcf049d42..9540766dd3 100644
>> --- a/hw/virtio/vhost-user.c
>> +++ b/hw/virtio/vhost-user.c
>> @@ -2643,6 +2643,7 @@ typedef struct {
>> DeviceState *dev;
>> CharBackend *cd;
>> struct vhost_dev *vhost;
>> + IOEventHandler *event_cb;
>> } VhostAsyncCallback;
>>
>> static void vhost_user_async_close_bh(void *opaque)
>> @@ -2657,7 +2658,10 @@ static void vhost_user_async_close_bh(void *opaque)
>> */
>> if (vhost->vdev) {
>> data->cb(data->dev);
>> - }
>> + } else if (data->event_cb) {
>> + qemu_chr_fe_set_handlers(data->cd, NULL, NULL, data->event_cb,
>> + NULL, data->dev, NULL, true);
>> + }
>>
>> g_free(data);
>> }
>> @@ -2669,7 +2673,9 @@ static void vhost_user_async_close_bh(void *opaque)
>> */
>> void vhost_user_async_close(DeviceState *d,
>> CharBackend *chardev, struct vhost_dev *vhost,
>> - vu_async_close_fn cb)
>> + vu_async_close_fn cb,
>> + IOEventHandler *event_cb
>
> Nit: why the newline before the closing parenthesis?
Acked.
>
>> + )
>> {
>> if (!runstate_check(RUN_STATE_SHUTDOWN)) {
>> /*
>> diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
>> index 191216a74f..5fdc711d4e 100644
>> --- a/include/hw/virtio/vhost-user.h
>> +++ b/include/hw/virtio/vhost-user.h
>> @@ -84,6 +84,8 @@ typedef void (*vu_async_close_fn)(DeviceState *cb);
>>
>> void vhost_user_async_close(DeviceState *d,
>> CharBackend *chardev, struct vhost_dev *vhost,
>> - vu_async_close_fn cb);
>> + vu_async_close_fn cb,
>> + IOEventHandler *event_cb
>
> Nit: ditto - don’t think we need this newline before );
Acked.
>
>> + );
>>
>> #endif
>> --
>> 2.41.0
Add a Error parameter to report the real error, like vhost-user-blk.
Signed-off-by: Li Feng <fengli@smartx.com>
---
hw/scsi/vhost-scsi-common.c | 16 +++++++++-------
hw/scsi/vhost-scsi.c | 5 +++--
hw/scsi/vhost-user-scsi.c | 14 ++++++++------
include/hw/virtio/vhost-scsi-common.h | 2 +-
4 files changed, 21 insertions(+), 16 deletions(-)
diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
index a61cd0e907..4c8637045d 100644
--- a/hw/scsi/vhost-scsi-common.c
+++ b/hw/scsi/vhost-scsi-common.c
@@ -16,6 +16,7 @@
*/
#include "qemu/osdep.h"
+#include "qapi/error.h"
#include "qemu/error-report.h"
#include "qemu/module.h"
#include "hw/virtio/vhost.h"
@@ -25,7 +26,7 @@
#include "hw/virtio/virtio-access.h"
#include "hw/fw-path-provider.h"
-int vhost_scsi_common_start(VHostSCSICommon *vsc)
+int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp)
{
int ret, i;
VirtIODevice *vdev = VIRTIO_DEVICE(vsc);
@@ -35,18 +36,19 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
VirtIOSCSICommon *vs = (VirtIOSCSICommon *)vsc;
if (!k->set_guest_notifiers) {
- error_report("binding does not support guest notifiers");
+ error_setg(errp, "binding does not support guest notifiers");
return -ENOSYS;
}
ret = vhost_dev_enable_notifiers(&vsc->dev, vdev);
if (ret < 0) {
+ error_setg_errno(errp, -ret, "Error enabling host notifiers");
return ret;
}
ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, true);
if (ret < 0) {
- error_report("Error binding guest notifier");
+ error_setg_errno(errp, -ret, "Error binding guest notifier");
goto err_host_notifiers;
}
@@ -54,7 +56,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
if (ret < 0) {
- error_report("Error setting inflight format: %d", -ret);
+ error_setg_errno(errp, -ret, "Error setting inflight format");
goto err_guest_notifiers;
}
@@ -64,21 +66,21 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
vs->conf.virtqueue_size,
vsc->inflight);
if (ret < 0) {
- error_report("Error getting inflight: %d", -ret);
+ error_setg_errno(errp, -ret, "Error getting inflight");
goto err_guest_notifiers;
}
}
ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
if (ret < 0) {
- error_report("Error setting inflight: %d", -ret);
+ error_setg_errno(errp, -ret, "Error setting inflight");
goto err_guest_notifiers;
}
}
ret = vhost_dev_start(&vsc->dev, vdev, true);
if (ret < 0) {
- error_report("Error start vhost dev");
+ error_setg_errno(errp, -ret, "Error starting vhost dev");
goto err_guest_notifiers;
}
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 443f67daa4..01a3ab4277 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -75,6 +75,7 @@ static int vhost_scsi_start(VHostSCSI *s)
int ret, abi_version;
VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
const VhostOps *vhost_ops = vsc->dev.vhost_ops;
+ Error *local_err = NULL;
ret = vhost_ops->vhost_scsi_get_abi_version(&vsc->dev, &abi_version);
if (ret < 0) {
@@ -88,14 +89,14 @@ static int vhost_scsi_start(VHostSCSI *s)
return -ENOSYS;
}
- ret = vhost_scsi_common_start(vsc);
+ ret = vhost_scsi_common_start(vsc, &local_err);
if (ret < 0) {
return ret;
}
ret = vhost_scsi_set_endpoint(s);
if (ret < 0) {
- error_report("Error setting vhost-scsi endpoint");
+ error_reportf_err(local_err, "Error setting vhost-scsi endpoint");
vhost_scsi_common_stop(vsc);
}
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index e931df9f5b..62fc98bb1c 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -43,12 +43,12 @@ enum VhostUserProtocolFeature {
VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
};
-static int vhost_user_scsi_start(VHostUserSCSI *s)
+static int vhost_user_scsi_start(VHostUserSCSI *s, Error **errp)
{
VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
int ret;
- ret = vhost_scsi_common_start(vsc);
+ ret = vhost_scsi_common_start(vsc, errp);
s->started_vu = (ret < 0 ? false : true);
return ret;
@@ -73,6 +73,7 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
bool should_start = virtio_device_should_start(vdev, status);
+ Error *local_err = NULL;
int ret;
if (!s->connected) {
@@ -84,9 +85,10 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
}
if (should_start) {
- ret = vhost_user_scsi_start(s);
+ ret = vhost_user_scsi_start(s, &local_err);
if (ret < 0) {
- error_report("unable to start vhost-user-scsi: %s", strerror(-ret));
+ error_reportf_err(local_err, "unable to start vhost-user-scsi: %s",
+ strerror(-ret));
qemu_chr_fe_disconnect(&vs->conf.chardev);
}
} else {
@@ -139,7 +141,7 @@ static void vhost_user_scsi_handle_output(VirtIODevice *vdev, VirtQueue *vq)
* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
* vhost here instead of waiting for .set_status().
*/
- ret = vhost_user_scsi_start(s);
+ ret = vhost_user_scsi_start(s, &local_err);
if (ret < 0) {
error_reportf_err(local_err, "vhost-user-scsi: vhost start failed: ");
qemu_chr_fe_disconnect(&vs->conf.chardev);
@@ -184,7 +186,7 @@ static int vhost_user_scsi_connect(DeviceState *dev, Error **errp)
/* restore vhost state */
if (virtio_device_started(vdev, vdev->status)) {
- ret = vhost_user_scsi_start(s);
+ ret = vhost_user_scsi_start(s, errp);
if (ret < 0) {
return ret;
}
diff --git a/include/hw/virtio/vhost-scsi-common.h b/include/hw/virtio/vhost-scsi-common.h
index 18f115527c..c5d2c09455 100644
--- a/include/hw/virtio/vhost-scsi-common.h
+++ b/include/hw/virtio/vhost-scsi-common.h
@@ -39,7 +39,7 @@ struct VHostSCSICommon {
struct vhost_inflight *inflight;
};
-int vhost_scsi_common_start(VHostSCSICommon *vsc);
+int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp);
void vhost_scsi_common_stop(VHostSCSICommon *vsc);
char *vhost_scsi_common_get_fw_dev_path(FWPathProvider *p, BusState *bus,
DeviceState *dev);
--
2.41.0
> On Aug 24, 2023, at 3:41 AM, Li Feng <fengli@smartx.com> wrote:
>
> Add a Error parameter to report the real error, like vhost-user-blk.
>
> Signed-off-by: Li Feng <fengli@smartx.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> ---
> hw/scsi/vhost-scsi-common.c | 16 +++++++++-------
> hw/scsi/vhost-scsi.c | 5 +++--
> hw/scsi/vhost-user-scsi.c | 14 ++++++++------
> include/hw/virtio/vhost-scsi-common.h | 2 +-
> 4 files changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
> index a61cd0e907..4c8637045d 100644
> --- a/hw/scsi/vhost-scsi-common.c
> +++ b/hw/scsi/vhost-scsi-common.c
> @@ -16,6 +16,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qapi/error.h"
> #include "qemu/error-report.h"
> #include "qemu/module.h"
> #include "hw/virtio/vhost.h"
> @@ -25,7 +26,7 @@
> #include "hw/virtio/virtio-access.h"
> #include "hw/fw-path-provider.h"
>
> -int vhost_scsi_common_start(VHostSCSICommon *vsc)
> +int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp)
> {
> int ret, i;
> VirtIODevice *vdev = VIRTIO_DEVICE(vsc);
> @@ -35,18 +36,19 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> VirtIOSCSICommon *vs = (VirtIOSCSICommon *)vsc;
>
> if (!k->set_guest_notifiers) {
> - error_report("binding does not support guest notifiers");
> + error_setg(errp, "binding does not support guest notifiers");
> return -ENOSYS;
> }
>
> ret = vhost_dev_enable_notifiers(&vsc->dev, vdev);
> if (ret < 0) {
> + error_setg_errno(errp, -ret, "Error enabling host notifiers");
> return ret;
> }
>
> ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, true);
> if (ret < 0) {
> - error_report("Error binding guest notifier");
> + error_setg_errno(errp, -ret, "Error binding guest notifier");
> goto err_host_notifiers;
> }
>
> @@ -54,7 +56,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
>
> ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
> if (ret < 0) {
> - error_report("Error setting inflight format: %d", -ret);
> + error_setg_errno(errp, -ret, "Error setting inflight format");
> goto err_guest_notifiers;
> }
>
> @@ -64,21 +66,21 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> vs->conf.virtqueue_size,
> vsc->inflight);
> if (ret < 0) {
> - error_report("Error getting inflight: %d", -ret);
> + error_setg_errno(errp, -ret, "Error getting inflight");
> goto err_guest_notifiers;
> }
> }
>
> ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
> if (ret < 0) {
> - error_report("Error setting inflight: %d", -ret);
> + error_setg_errno(errp, -ret, "Error setting inflight");
> goto err_guest_notifiers;
> }
> }
>
> ret = vhost_dev_start(&vsc->dev, vdev, true);
> if (ret < 0) {
> - error_report("Error start vhost dev");
> + error_setg_errno(errp, -ret, "Error starting vhost dev");
> goto err_guest_notifiers;
> }
>
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 443f67daa4..01a3ab4277 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -75,6 +75,7 @@ static int vhost_scsi_start(VHostSCSI *s)
> int ret, abi_version;
> VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
> const VhostOps *vhost_ops = vsc->dev.vhost_ops;
> + Error *local_err = NULL;
>
> ret = vhost_ops->vhost_scsi_get_abi_version(&vsc->dev, &abi_version);
> if (ret < 0) {
> @@ -88,14 +89,14 @@ static int vhost_scsi_start(VHostSCSI *s)
> return -ENOSYS;
> }
>
> - ret = vhost_scsi_common_start(vsc);
> + ret = vhost_scsi_common_start(vsc, &local_err);
> if (ret < 0) {
> return ret;
> }
>
> ret = vhost_scsi_set_endpoint(s);
> if (ret < 0) {
> - error_report("Error setting vhost-scsi endpoint");
> + error_reportf_err(local_err, "Error setting vhost-scsi endpoint");
> vhost_scsi_common_stop(vsc);
> }
>
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index e931df9f5b..62fc98bb1c 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -43,12 +43,12 @@ enum VhostUserProtocolFeature {
> VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
> };
>
> -static int vhost_user_scsi_start(VHostUserSCSI *s)
> +static int vhost_user_scsi_start(VHostUserSCSI *s, Error **errp)
> {
> VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
> int ret;
>
> - ret = vhost_scsi_common_start(vsc);
> + ret = vhost_scsi_common_start(vsc, errp);
> s->started_vu = (ret < 0 ? false : true);
>
> return ret;
> @@ -73,6 +73,7 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
> VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
> VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
> bool should_start = virtio_device_should_start(vdev, status);
> + Error *local_err = NULL;
> int ret;
>
> if (!s->connected) {
> @@ -84,9 +85,10 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
> }
>
> if (should_start) {
> - ret = vhost_user_scsi_start(s);
> + ret = vhost_user_scsi_start(s, &local_err);
> if (ret < 0) {
> - error_report("unable to start vhost-user-scsi: %s", strerror(-ret));
> + error_reportf_err(local_err, "unable to start vhost-user-scsi: %s",
> + strerror(-ret));
> qemu_chr_fe_disconnect(&vs->conf.chardev);
> }
> } else {
> @@ -139,7 +141,7 @@ static void vhost_user_scsi_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> * Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
> * vhost here instead of waiting for .set_status().
> */
> - ret = vhost_user_scsi_start(s);
> + ret = vhost_user_scsi_start(s, &local_err);
> if (ret < 0) {
> error_reportf_err(local_err, "vhost-user-scsi: vhost start failed: ");
> qemu_chr_fe_disconnect(&vs->conf.chardev);
> @@ -184,7 +186,7 @@ static int vhost_user_scsi_connect(DeviceState *dev, Error **errp)
>
> /* restore vhost state */
> if (virtio_device_started(vdev, vdev->status)) {
> - ret = vhost_user_scsi_start(s);
> + ret = vhost_user_scsi_start(s, errp);
> if (ret < 0) {
> return ret;
> }
> diff --git a/include/hw/virtio/vhost-scsi-common.h b/include/hw/virtio/vhost-scsi-common.h
> index 18f115527c..c5d2c09455 100644
> --- a/include/hw/virtio/vhost-scsi-common.h
> +++ b/include/hw/virtio/vhost-scsi-common.h
> @@ -39,7 +39,7 @@ struct VHostSCSICommon {
> struct vhost_inflight *inflight;
> };
>
> -int vhost_scsi_common_start(VHostSCSICommon *vsc);
> +int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp);
> void vhost_scsi_common_stop(VHostSCSICommon *vsc);
> char *vhost_scsi_common_get_fw_dev_path(FWPathProvider *p, BusState *bus,
> DeviceState *dev);
> --
> 2.41.0
>
© 2016 - 2024 Red Hat, Inc.