Re: [PATCH 2/4] virtio: forward errors into qdev_report_runtime_error()

[Vladimir Sementsov-Ogievskiy]

On 5/19/22 17:19, Konstantin Khlebnikov wrote:
> Repalce virtio_error() with macro which forms structured Error and
> reports it as device runtime-error in addition to present actions.
>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> ---
>   hw/virtio/virtio.c         |    9 +++------
>   include/hw/virtio/virtio.h |   10 +++++++++-
>   2 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 5d607aeaa0..638d779bf2 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3642,13 +3642,10 @@ void virtio_device_set_child_bus_name(VirtIODevice 
> *vdev, char *bus_name)
>       vdev->bus_name = g_strdup(bus_name);
>   }
>   
> -void G_GNUC_PRINTF(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
> +void virtio_fatal_error(VirtIODevice *vdev, Error *err)
>   {
> -    va_list ap;
> -
> -    va_start(ap, fmt);
> -    error_vreport(fmt, ap);
> -    va_end(ap);
> +    qdev_report_runtime_error(&vdev->parent_obj, err);
> +    error_report_err(err);
>   
>       if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>           vdev->status = vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET;
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index db1c0ddf6b..a165e35b0b 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -16,6 +16,7 @@
>   
>   #include "exec/memory.h"
>   #include "hw/qdev-core.h"
> +#include "qapi/error.h"
>   #include "net/net.h"
>   #include "migration/vmstate.h"
>   #include "qemu/event_notifier.h"
> @@ -172,7 +173,14 @@ void virtio_init(VirtIODevice *vdev, uint16_t device_id, 
> size_t config_size);
>   
>   void virtio_cleanup(VirtIODevice *vdev);
>   
> -void virtio_error(VirtIODevice *vdev, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
> +#define virtio_error(vdev, fmt, ...) {              \
> +    Error *_err = NULL;                             \
> +    error_setg(&_err, (fmt), ## __VA_ARGS__);       \
> +    virtio_fatal_error(vdev, _err);                 \
> +} while (0)
> +
> +/* Reports and frees error, breaks device */
> +void virtio_fatal_error(VirtIODevice *vdev, Error *err);
>   
>   /* Set the child bus name. */
>   void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name);

Hmm. So we create temporary Error object just to pass it to 
qdev_report_runtime_error..

I think we can avoid introducing this intermediate Error object together with 
new macro:
just convert argument list to string with help of g_strdup_vprintf() in 
original virtio_error(), error_report this string and pass to 
qdev_report_runtime_error() (which should be simplified to get just a string).

--
Best regards,
Vladimir