[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/5] virtio: Error object based virtio_error()
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [PATCH 1/5] virtio: Error object based virtio_error() |
Date: |
Tue, 28 Mar 2017 10:14:09 +0200 |
On Mon, 27 Mar 2017 21:20:56 +0300
"Michael S. Tsirkin" <address@hidden> wrote:
> 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.
>
I don't understand this question... Are you talking of the NEEDS_RESET
status bit (we may expose this flag to non-virtio1 drivers?) or the
broken flag itself (we should not implement broken legacy devices) ?
>
> > @@ -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.
>
I don't think so: if the device is broken, it stops processing virtqueues
in and out, until it gets reset. And even though we would have a stubborn
guest that keeps breaking the device again and again, libvirt has size
limited and rotating log files.
> 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?
>
Huh? This patch simply introduces a new API to a feature that underwent
several rounds of discussion and reached a reasonable consensus (even
your R-b).
I'm not sure this 9pfs series is the right place to talk about all the
behavior changes you're suggesting for virtio_error()... I'd rather
drop this patch and duplicate code in virtio-9p instead if I want the
fixes to go to 2.9.
Cc'ing Connie and Stefanha for insights.
> > 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);
> >
pgpTHuBIm_Z3r.pgp
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH 0/5] 9pfs: handle transport errors, Greg Kurz, 2017/03/27
- [Qemu-devel] [PATCH 1/5] virtio: Error object based virtio_error(), Greg Kurz, 2017/03/27
- Re: [Qemu-devel] [PATCH 1/5] virtio: Error object based virtio_error(), Michael S. Tsirkin, 2017/03/27
- Re: [Qemu-devel] [PATCH 1/5] virtio: Error object based virtio_error(), Cornelia Huck, 2017/03/28
- Re: [Qemu-devel] [PATCH 1/5] virtio: Error object based virtio_error(),
Greg Kurz <=
- Re: [Qemu-devel] [PATCH 1/5] virtio: Error object based virtio_error(), Cornelia Huck, 2017/03/28
- Re: [Qemu-devel] [PATCH 1/5] virtio: Error object based virtio_error(), Greg Kurz, 2017/03/28
- Re: [Qemu-devel] [PATCH 1/5] virtio: Error object based virtio_error(), Cornelia Huck, 2017/03/28
- Re: [Qemu-devel] [PATCH 1/5] virtio: Error object based virtio_error(), Greg Kurz, 2017/03/31
[Qemu-devel] [PATCH 2/5] virtio-9p: factor out virtio_9p_error_err(), Greg Kurz, 2017/03/27
[Qemu-devel] [PATCH 3/5] fsdev: don't allow unknown format in marshal/unmarshal, Greg Kurz, 2017/03/27
[Qemu-devel] [PATCH 4/5] 9pfs: drop pdu_push_and_notify(), Greg Kurz, 2017/03/27
[Qemu-devel] [PATCH 5/5] 9pfs: handle broken transport, Greg Kurz, 2017/03/27