[Qemu-devel] [PATCH 0/5] 9pfs: handle transport errors

[Qemu-devel] [PATCH 0/5] 9pfs: handle transport errors

[Greg Kurz]

The 9p protocol relies on a reliable transport, but the current code
treats transport errors (ie, failure to marshal or unmarshal) as if
they were coming from the backend. This doesn't make sense: if the
transport failed, we should notify the guest that the transport is
broken and needs to be reset, using transport specific means.

This series modifies the existing virtio-9p transport so that it can
notify the guest about transport failures. The core 9p code is modified
as well so that it stops handling requests when the transport fails.

---

Greg Kurz (5):
      virtio: Error object based virtio_error()
      virtio-9p: factor out virtio_9p_error_err()
      fsdev: don't allow unknown format in marshal/unmarshal
      9pfs: drop pdu_push_and_notify()
      9pfs: handle broken transport


 fsdev/9p-iov-marshal.c     |    4 +--
 hw/9pfs/9p.c               |   43 +++++++++++++++++++++----------
 hw/9pfs/9p.h               |    1 +
 hw/9pfs/virtio-9p-device.c |   61 ++++++++++++++++++++++++++++++++------------
 hw/virtio/virtio.c         |   21 ++++++++++++---
 include/hw/virtio/virtio.h |    1 +
 6 files changed, 93 insertions(+), 38 deletions(-)

--
Greg

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

[Greg Kurz]

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 <groug@kaod.org>
---
 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;
@@ -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)
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);

[Qemu-devel] [PATCH 2/5] virtio-9p: factor out virtio_9p_error_err()

[Greg Kurz]

When an unrecoverable is hit, we need to set the broken flag of the virtio
device, detach the queue element and free it. This is currently open coded
in handle_9p_output(). It is fine since this is the only function that can
set the broken flag. But if we want to be able to do this from other places,
we must consolidate the logic in a helper.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/virtio-9p-device.c |   45 +++++++++++++++++++++++++++++---------------
 1 file changed, 30 insertions(+), 15 deletions(-)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 27a4a32f5c4c..873b22baf0f9 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -22,21 +22,42 @@
 
 static const struct V9fsTransport virtio_9p_transport;
 
+static void virtio_9p_free_element(V9fsVirtioState *v, unsigned int idx)
+{
+    VirtQueueElement **pelem = &v->elems[idx];
+    g_free(*pelem);
+    *pelem = NULL;
+}
+
 static void virtio_9p_push_and_notify(V9fsPDU *pdu)
 {
     V9fsState *s = pdu->s;
     V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
-    VirtQueueElement *elem = v->elems[pdu->idx];
 
     /* push onto queue and notify */
-    virtqueue_push(v->vq, elem, pdu->size);
-    g_free(elem);
-    v->elems[pdu->idx] = NULL;
+    virtqueue_push(v->vq, v->elems[pdu->idx], pdu->size);
+    virtio_9p_free_element(v, pdu->idx);
 
     /* FIXME: we should batch these completions */
     virtio_notify(VIRTIO_DEVICE(v), v->vq);
 }
 
+static void virtio_9p_error_err(V9fsVirtioState *v, unsigned int idx,
+                                Error *err)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(v);
+
+    virtio_error_err(vdev, err);
+    virtqueue_detach_element(v->vq, v->elems[idx], 0);
+    virtio_9p_free_element(v, idx);
+}
+
+#define virtio_9p_error(v, idx, ...) { \
+    Error *err = NULL;                 \
+    error_setg(&err, ## __VA_ARGS__);  \
+    virtio_9p_error_err(v, idx, err);  \
+}
+
 static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
 {
     V9fsVirtioState *v = (V9fsVirtioState *)vdev;
@@ -56,22 +77,19 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
         if (!elem) {
             goto out_free_pdu;
         }
+        v->elems[pdu->idx] = elem;
 
         if (elem->in_num == 0) {
-            virtio_error(vdev,
-                         "The guest sent a VirtFS request without space for "
-                         "the reply");
-            goto out_free_req;
+            virtio_9p_error(v, pdu->idx, "The guest sent a VirtFS request without space for the reply");
+            goto out_free_pdu;
         }
         QEMU_BUILD_BUG_ON(sizeof(out) != 7);
 
-        v->elems[pdu->idx] = elem;
         len = iov_to_buf(elem->out_sg, elem->out_num, 0,
                          &out, sizeof(out));
         if (len != sizeof(out)) {
-            virtio_error(vdev, "The guest sent a malformed VirtFS request: "
-                         "header size is %zd, should be 7", len);
-            goto out_free_req;
+            virtio_9p_error(v, pdu->idx, "The guest sent a malformed VirtFS request: header size is %zd, should be 7", len);
+            goto out_free_pdu;
         }
 
         pdu->size = le32_to_cpu(out.size_le);
@@ -85,9 +103,6 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
 
     return;
 
-out_free_req:
-    virtqueue_detach_element(vq, elem, 0);
-    g_free(elem);
 out_free_pdu:
     pdu_free(pdu);
 }

[Qemu-devel] [PATCH 3/5] fsdev: don't allow unknown format in marshal/unmarshal

[Greg Kurz]

The code only uses well known format strings. An unknown format token is a
bug.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fsdev/9p-iov-marshal.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c
index 1d16f8df4bd4..a1c9beddd2e7 100644
--- a/fsdev/9p-iov-marshal.c
+++ b/fsdev/9p-iov-marshal.c
@@ -168,7 +168,7 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset,
             break;
         }
         default:
-            break;
+            g_assert_not_reached();
         }
         if (copied < 0) {
             return copied;
@@ -281,7 +281,7 @@ ssize_t v9fs_iov_vmarshal(struct iovec *in_sg, int in_num, size_t offset,
             break;
         }
         default:
-            break;
+            g_assert_not_reached();
         }
         if (copied < 0) {
             return copied;

[Qemu-devel] [PATCH 4/5] 9pfs: drop pdu_push_and_notify()

[Greg Kurz]

Only pdu_complete() needs to notify the client that a request has completed.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p.c |    7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 48babce836b6..09a8c79cf781 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -65,11 +65,6 @@ ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
     return ret;
 }
 
-static void pdu_push_and_notify(V9fsPDU *pdu)
-{
-    pdu->s->transport->push_and_notify(pdu);
-}
-
 static int omode_to_uflags(int8_t mode)
 {
     int ret = 0;
@@ -667,7 +662,7 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu, ssize_t len)
     pdu->size = len;
     pdu->id = id;
 
-    pdu_push_and_notify(pdu);
+    pdu->s->transport->push_and_notify(pdu);
 
     /* Now wakeup anybody waiting in flush for this request */
     if (!qemu_co_queue_next(&pdu->complete)) {

[Qemu-devel] [PATCH 5/5] 9pfs: handle broken transport

[Greg Kurz]

The 9p protocol is transport agnostic: if an error occurs when copying data
to/from the client, this should be handled by the transport layer [1] and
the 9p server should simply stop processing requests [2].

[1] can be implemented in the transport marshal/unmarshal handlers. In the
case of virtio, this means calling virtio_error() to inform the guest that
the device isn't functional anymore.

[2] means that the pdu_complete() function shouldn't send a reply back to
the client if the transport had a failure. This cannot be decided using the
current error path though, since we cannot discriminate if the error comes
from the transport or the backend. This patch hence introduces a flag in
the 9pfs state to record that the transport is broken. The device needs to
be reset for the flag to be unset.

This fixes Coverity issue CID 1348518.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p.c               |   36 ++++++++++++++++++++++++++++--------
 hw/9pfs/9p.h               |    1 +
 hw/9pfs/virtio-9p-device.c |   16 ++++++++++++++--
 3 files changed, 43 insertions(+), 10 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 09a8c79cf781..79d8292e9a7a 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -50,6 +50,9 @@ ssize_t pdu_marshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
     ret = pdu->s->transport->pdu_vmarshal(pdu, offset, fmt, ap);
     va_end(ap);
 
+    if (ret < 0) {
+        pdu->s->transport_broken = true;
+    }
     return ret;
 }
 
@@ -62,6 +65,9 @@ ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
     ret = pdu->s->transport->pdu_vunmarshal(pdu, offset, fmt, ap);
     va_end(ap);
 
+    if (ret < 0) {
+        pdu->s->transport_broken = true;
+    }
     return ret;
 }
 
@@ -623,15 +629,15 @@ void pdu_free(V9fsPDU *pdu)
     QLIST_INSERT_HEAD(&s->free_list, pdu, next);
 }
 
-/*
- * We don't do error checking for pdu_marshal/unmarshal here
- * because we always expect to have enough space to encode
- * error details
- */
 static void coroutine_fn pdu_complete(V9fsPDU *pdu, ssize_t len)
 {
     int8_t id = pdu->id + 1; /* Response */
     V9fsState *s = pdu->s;
+    int ret;
+
+    if (s->transport_broken) {
+        goto out_complete;
+    }
 
     if (len < 0) {
         int err = -len;
@@ -643,11 +649,19 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu, ssize_t len)
             str.data = strerror(err);
             str.size = strlen(str.data);
 
-            len += pdu_marshal(pdu, len, "s", &str);
+            ret = pdu_marshal(pdu, len, "s", &str);
+            if (ret < 0) {
+                goto out_complete;
+            }
+            len += ret;
             id = P9_RERROR;
         }
 
-        len += pdu_marshal(pdu, len, "d", err);
+        ret = pdu_marshal(pdu, len, "d", err);
+        if (ret < 0) {
+            goto out_complete;
+        }
+        len += ret;
 
         if (s->proto_version == V9FS_PROTO_2000L) {
             id = P9_RLERROR;
@@ -656,7 +670,10 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu, ssize_t len)
     }
 
     /* fill out the header */
-    pdu_marshal(pdu, 0, "dbw", (int32_t)len, id, pdu->tag);
+    ret = pdu_marshal(pdu, 0, "dbw", (int32_t)len, id, pdu->tag);
+    if (ret < 0) {
+        goto out_complete;
+    }
 
     /* keep these in sync */
     pdu->size = len;
@@ -664,6 +681,7 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu, ssize_t len)
 
     pdu->s->transport->push_and_notify(pdu);
 
+out_complete:
     /* Now wakeup anybody waiting in flush for this request */
     if (!qemu_co_queue_next(&pdu->complete)) {
         pdu_free(pdu);
@@ -3593,6 +3611,8 @@ void v9fs_reset(V9fsState *s)
     while (!data.done) {
         aio_poll(qemu_get_aio_context(), true);
     }
+
+    s->transport_broken = false;
 }
 
 static void __attribute__((__constructor__)) v9fs_set_fd_limit(void)
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index b7e836251e13..523abdb0a582 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -240,6 +240,7 @@ typedef struct V9fsState
     Error *migration_blocker;
     V9fsConf fsconf;
     V9fsQID root_qid;
+    bool transport_broken;
 } V9fsState;
 
 /* 9p2000.L open flags */
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 873b22baf0f9..6ea9acfc28a9 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -172,8 +172,14 @@ static ssize_t virtio_pdu_vmarshal(V9fsPDU *pdu, size_t offset,
     V9fsState *s = pdu->s;
     V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
     VirtQueueElement *elem = v->elems[pdu->idx];
+    int ret;
 
-    return v9fs_iov_vmarshal(elem->in_sg, elem->in_num, offset, 1, fmt, ap);
+    ret = v9fs_iov_vmarshal(elem->in_sg, elem->in_num, offset, 1, fmt, ap);
+    if (ret < 0) {
+        virtio_9p_error(v, pdu->idx,
+                        "Failed to marshal VirtFS reply type %d", pdu->id);
+    }
+    return ret;
 }
 
 static ssize_t virtio_pdu_vunmarshal(V9fsPDU *pdu, size_t offset,
@@ -182,8 +188,14 @@ static ssize_t virtio_pdu_vunmarshal(V9fsPDU *pdu, size_t offset,
     V9fsState *s = pdu->s;
     V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
     VirtQueueElement *elem = v->elems[pdu->idx];
+    int ret;
 
-    return v9fs_iov_vunmarshal(elem->out_sg, elem->out_num, offset, 1, fmt, ap);
+    ret = v9fs_iov_vunmarshal(elem->out_sg, elem->out_num, offset, 1, fmt, ap);
+    if (ret < 0) {
+        virtio_9p_error(v, pdu->idx,
+                        "Failed to unmarshal VirtFS request type %d", pdu->id);
+    }
+    return ret;
 }
 
 /* The size parameter is used by other transports. Do not drop it. */

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 <groug@kaod.org>
> ---
>  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);

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

[Cornelia Huck]

On Mon, 27 Mar 2017 21:20:56 +0300
"Michael S. Tsirkin" <mst@redhat.com> 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 <groug@kaod.org>
> > ---
> >  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'm a bit torn there. In theory, setting an unknown status bit should
not really do harm; but we can't be sure that there aren't legacy
drivers out there that will crash when they notice an unknown status
bit, and I'm not sure we want that.

> 
> 
> > @@ -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.

One would hope that qemu stops processing broken devices, but a check
might be better.

> 
> 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?

I would trust the device implementation to make the decision: Can we
recover, can we start using the device again after a reset, or are we
so broken that we want to terminate the vm?

Note that all of this already applies to the existing virtio_error(); I
think we should discuss this independently of this patch.

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

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 3882 bytes --]

On Mon, 27 Mar 2017 21:20:56 +0300
"Michael S. Tsirkin" <mst@redhat.com> 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 <groug@kaod.org>
> > ---
> >  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);  


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Cornelia Huck]

On Tue, 28 Mar 2017 10:14:09 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Mon, 27 Mar 2017 21:20:56 +0300
> "Michael S. Tsirkin" <mst@redhat.com> 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 <groug@kaod.org>
> > > ---
> > >  hw/virtio/virtio.c         |   21 ++++++++++++++++-----
> > >  include/hw/virtio/virtio.h |    1 +
> > >  2 files changed, 17 insertions(+), 5 deletions(-)
> > > 

> > 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.

I agree that we should discuss this outside of this patch series. It's
not like it is introducing a new error case.

> 
> Cc'ing Connie and Stefanha for insights.

See my reply to Michael's mail.

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

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 1775 bytes --]

On Tue, 28 Mar 2017 10:24:21 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Tue, 28 Mar 2017 10:14:09 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > On Mon, 27 Mar 2017 21:20:56 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> 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 <groug@kaod.org>
> > > > ---
> > > >  hw/virtio/virtio.c         |   21 ++++++++++++++++-----
> > > >  include/hw/virtio/virtio.h |    1 +
> > > >  2 files changed, 17 insertions(+), 5 deletions(-)
> > > >   
> 
> > > 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.  
> 
> I agree that we should discuss this outside of this patch series. It's
> not like it is introducing a new error case.
> 
> > 
> > Cc'ing Connie and Stefanha for insights.  
> 
> See my reply to Michael's mail.
> 

Yeah, I saw that just after pressing the send button :)

The points raised by Michael make a lot of sense anyway. Maybe we can
discuss them for 2.10 ?

Cheers.

--
Greg

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Cornelia Huck]

On Tue, 28 Mar 2017 11:34:15 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Tue, 28 Mar 2017 10:24:21 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > On Tue, 28 Mar 2017 10:14:09 +0200
> > Greg Kurz <groug@kaod.org> wrote:
> > 
> > > On Mon, 27 Mar 2017 21:20:56 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> 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 <groug@kaod.org>
> > > > > ---
> > > > >  hw/virtio/virtio.c         |   21 ++++++++++++++++-----
> > > > >  include/hw/virtio/virtio.h |    1 +
> > > > >  2 files changed, 17 insertions(+), 5 deletions(-)
> > > > >   
> > 
> > > > 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.  
> > 
> > I agree that we should discuss this outside of this patch series. It's
> > not like it is introducing a new error case.
> > 
> > > 
> > > Cc'ing Connie and Stefanha for insights.  
> > 
> > See my reply to Michael's mail.
> > 
> 
> Yeah, I saw that just after pressing the send button :)

:)

> 
> The points raised by Michael make a lot of sense anyway. Maybe we can
> discuss them for 2.10 ?

Certainly. We should not delay any fixes for 2.9, though.

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

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 2406 bytes --]

On Tue, 28 Mar 2017 12:14:59 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Tue, 28 Mar 2017 11:34:15 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > On Tue, 28 Mar 2017 10:24:21 +0200
> > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> >   
> > > On Tue, 28 Mar 2017 10:14:09 +0200
> > > Greg Kurz <groug@kaod.org> wrote:
> > >   
> > > > On Mon, 27 Mar 2017 21:20:56 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> 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 <groug@kaod.org>
> > > > > > ---
> > > > > >  hw/virtio/virtio.c         |   21 ++++++++++++++++-----
> > > > > >  include/hw/virtio/virtio.h |    1 +
> > > > > >  2 files changed, 17 insertions(+), 5 deletions(-)
> > > > > >     
> > >   
> > > > > 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.    
> > > 
> > > I agree that we should discuss this outside of this patch series. It's
> > > not like it is introducing a new error case.
> > >   
> > > > 
> > > > Cc'ing Connie and Stefanha for insights.    
> > > 
> > > See my reply to Michael's mail.
> > >   
> > 
> > Yeah, I saw that just after pressing the send button :)  
> 
> :)
> 
> > 
> > The points raised by Michael make a lot of sense anyway. Maybe we can
> > discuss them for 2.10 ?  
> 
> Certainly. We should not delay any fixes for 2.9, though.
> 

Michael,

Your comments call for some more discussion to take place during 2.10 devel.

Can you please ack this patch and I'll take it through my tree for 2.9 ?

Cheers.

--
Greg

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]