On 21 Aug 2023, at 8:09 PM, Markus Armbruster <armbru@redhat.com> wrote:
Li Feng <fengli@smartx.com> writes:
2023年8月14日 下午8:11，Raphael Norwitz <raphael.norwitz@nutanix.com> 写道：
Thanks for the cleanup! A few comments.
On Aug 4, 2023, at 1:29 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>
---
hw/scsi/vhost-scsi-common.c | 17 ++++++++++-------
hw/scsi/vhost-scsi.c | 5 +++--
hw/scsi/vhost-user-scsi.c | 14 ++++++++------
include/hw/virtio/vhost-scsi-common.h | 2 +-
4 files changed, 22 insertions(+), 16 deletions(-)
diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
index a61cd0e907..392587dfb5 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);
Curious why you’re adding the error value to the string. Isn’t it redundant since we pass it in as the second param?
+ error_setg_errno(errp, -ret, "Error setting inflight format: %d", -ret);
I don’t understand. Here I put the error message in errp, where is redundant?
The error message will come out like
Error setting inflight format: 22: Invalid argument
You need to drop ": %d".
Two remarks:
1. The #1 reason for bad error messages is neglecting to *test* them.
2. Printing errno as a number is needlessly unfriendly to users.
[...]
Understood! Very thanks, I will fix it in the v2.