Re: [Qemu-devel] [PATCH 1/5] virtio: Error object based virtio_error()

[Michael S. Tsirkin]

On Mon, Mar 27, 2017 at 07:46:03PM +0200, Greg Kurz wrote:
> This introduces an Error object based implementation of virtio_error(). It
> allows to implement virtio_error() wrappers in device-specific code.
> 
> Signed-off-by: Greg Kurz <address@hidden>
> ---
>  hw/virtio/virtio.c         |   21 ++++++++++++++++-----
>  include/hw/virtio/virtio.h |    1 +
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 03592c542a55..4036f4816038 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2443,6 +2443,16 @@ void virtio_device_set_child_bus_name(VirtIODevice 
> *vdev, char *bus_name)
>      vdev->bus_name = g_strdup(bus_name);
>  }
>  
> +static void virtio_device_set_broken(VirtIODevice *vdev)
> +{
> +    vdev->broken = true;
> +
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> +        virtio_set_status(vdev, vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET);
> +        virtio_notify_config(vdev);
> +    }
> +}
> +
>  void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, 
> ...)
>  {
>      va_list ap;

It's worth pondering whether we can set this for versions < 1.0 too.


> @@ -2451,12 +2461,13 @@ void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice 
> *vdev, const char *fmt, ...)
>      error_vreport(fmt, ap);
>      va_end(ap);
>  
> -    vdev->broken = true;
> +    virtio_device_set_broken(vdev);
> +}
>  
> -    if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> -        virtio_set_status(vdev, vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET);
> -        virtio_notify_config(vdev);
> -    }
> +void virtio_error_err(VirtIODevice *vdev, Error *err)
> +{
> +    error_report_err(err);
> +    virtio_device_set_broken(vdev);
>  }
>  
>  static void virtio_memory_listener_commit(MemoryListener *listener)

Should this skip error report if device is already broken?
Otherwise we'll get a ton of errors in the log.

Also, whether to stop the device, or the VM, or just warn,
seems like a policy decision. Why not set it on command line
like we do for other storage?

> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 15efcf205711..5b13c5f67b63 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -150,6 +150,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
>  void virtio_cleanup(VirtIODevice *vdev);
>  
>  void virtio_error(VirtIODevice *vdev, const char *fmt, ...) GCC_FMT_ATTR(2, 
> 3);
> +void virtio_error_err(VirtIODevice *vdev, Error *err);
>  
>  /* Set the child bus name. */
>  void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name);