Li Feng <fen...@smartx.com> writes:
>> 2023年8月14日 下午8:11，Raphael Norwitz <raphael.norw...@nutanix.com> 写道：
>>
>> Thanks for the cleanup! A few comments.
>>
>>> On Aug 4, 2023, at 1:29 AM, Li Feng <fen...@smartx.com> wrote:
>>>
>>> Add a Error parameter to report the real error, like vhost-user-blk.
>>>
>>> Signed-off-by: Li Feng <fen...@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.
[...]
