Re: [PATCH 2/2] vhost: Add Error parameter to vhost_scsi_common_start()

[Markus Armbruster <armbru@redhat.com>]

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.

[...]