[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 10/28] vhost: change some assert() for error_
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH v3 10/28] vhost: change some assert() for error_report() or silent fail |
Date: |
Thu, 21 Jul 2016 03:57:53 -0400 (EDT) |
Hi
----- Original Message -----
> On Wed, Jul 20, 2016 at 09:41:26AM -0400, Marc-André Lureau wrote:
> >
> >
> > ----- Original Message -----
> > > On Wed, Jul 06, 2016 at 08:47:03PM +0200, address@hidden
> > > wrote:
> > > > From: Marc-André Lureau <address@hidden>
> > > >
> > > > Calling a vhost operation may fail, especially with disconnectable
> > > > backends. Treat some that look harmless as recoverable errors (print
> > > > error, or ignore on error code path).
> > > >
> > > > Signed-off-by: Marc-André Lureau <address@hidden>
> > >
> > > These might be recoverable for vhost-user not for vhost_net.
> >
> > I don't think we can hide all the error handling in vhost-user very
> > long, soon enough we will need to reset the guest device state.
>
> Interesting. This will need some thought.
>
>
> > If
> > vhost-net doesn't support error, it should rather assert() there, but
> > having the error handling done at higher level, at the vhost interface
> > level at least, not at the backend level.
>
> Interesting. That might be reasonable too but would increase the scope
> of this already large patchset even further.
>
>
> > > IMO the backend should return 0 if error is benign,
> > > report errors to vhost only if they are fatal.
> >
> > Imho whether it's fatal and how to recover as not much to do with the
> > backend (which each kind of just a proxy), it should be handled at higher
> > level, possibly up to the guest.
>
> Consider example below. EBADF means fd not writeable - remote exited
> so that's benign. EFAULT means code bug. vhost has no idea there's
> an fd though.
>
It will probably be EPIPE, right? unless the fd was closed. Hhmpf,
tcp_chr_write() actually returns the number of bytes to write if the peer is
disconnected... and io_channel_send_full() on failure EINVAL..
>
> > > For example, consider set mem table. Write failing is one thing,
> > > and it's benign, but e.g. table too big is another thing and isn't.
It depends, if the backend disconnects during that call, it isn't "fatal". A
later reconnection will restart and reset vhost-user tables.
> > > Also, we might want to distinguish between EBADF (fd closed)
> > > and other types of errors. All this knowledge belomgs in vhost user.
> > >
It's hard to hide disconnected state away, beside the need to report errors
higher up, the handling of the disconnected state is not just in the vhost-user
backend, but also in net/vhost-user.
I also notice that qemu_chr_fe_write*() will not trigger disconnect events,
while read qemu_chr_fe_read() will: vhost_dev struct will be 0'ed during the
call, by net_vhost_user_event() handler.
In most cases (there is a minor exception in set_vring_endian_legacy), vhost
actually doesn't care about errno. However, it reports up errors using errno
values. It seems it is only reported by vhost_dev_init() and vhost_dev_start(),
and the value is used for strerror/error_report() (I notice also that failing
vhost_dev_start() in vhost_scsi_start() is fatal)
> > > > ---
> > > > hw/virtio/vhost.c | 32 +++++++++++++++++++++-----------
> > > > 1 file changed, 21 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > > index 75bc51e..e03a031 100644
> > > > --- a/hw/virtio/vhost.c
> > > > +++ b/hw/virtio/vhost.c
> > > > @@ -400,7 +400,10 @@ static inline void vhost_dev_log_resize(struct
> > > > vhost_dev *dev, uint64_t size)
> > > > /* inform backend of log switching, this must be done before
> > > > releasing the current log, to ensure no logging is lost */
> > > > r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);
> > > > - assert(r >= 0);
> > > > + if (r < 0) {
> > > > + error_report("Failed to change backend log");
> > > > + }
> > > > +
> > > > vhost_log_put(dev, true);
> > > > dev->log = log;
> > > > dev->log_size = size;
> > > > @@ -567,7 +570,9 @@ static void vhost_commit(MemoryListener *listener)
> > > >
> > > > if (!dev->log_enabled) {
> > > > r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
> > > > - assert(r >= 0);
> > > > + if (r < 0) {
> > > > + error_report("Failed to set mem table");
> > > > + }
> > > > dev->memory_changed = false;
> > > > return;
> > > > }
> > > > @@ -580,7 +585,9 @@ static void vhost_commit(MemoryListener *listener)
> > > > vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER);
> > > > }
> > > > r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
> > > > - assert(r >= 0);
> > > > + if (r < 0) {
> > > > + error_report("Failed to set mem table");
> > > > + }
> > > > /* To log less, can only decrease log size after table update. */
> > > > if (dev->log_size > log_size + VHOST_LOG_BUFFER) {
> > > > vhost_dev_log_resize(dev, log_size);
> > > > @@ -649,6 +656,7 @@ static int vhost_virtqueue_set_addr(struct
> > > > vhost_dev
> > > > *dev,
> > > > };
> > > > int r = dev->vhost_ops->vhost_set_vring_addr(dev, &addr);
> > > > if (r < 0) {
> > > > + error_report("Failed to set vring addr");
> > > > return -errno;
> > > > }
> > > > return 0;
> > > > @@ -662,12 +670,15 @@ static int vhost_dev_set_features(struct
> > > > vhost_dev
> > > > *dev, bool enable_log)
> > > > features |= 0x1ULL << VHOST_F_LOG_ALL;
> > > > }
> > > > r = dev->vhost_ops->vhost_set_features(dev, features);
> > > > + if (r < 0) {
> > > > + error_report("Failed to set features");
> > > > + }
> > > > return r < 0 ? -errno : 0;
> > > > }
> > > >
> > > > static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
> > > > {
> > > > - int r, t, i, idx;
> > > > + int r, i, idx;
> > > > r = vhost_dev_set_features(dev, enable_log);
> > > > if (r < 0) {
> > > > goto err_features;
> > > > @@ -684,12 +695,10 @@ static int vhost_dev_set_log(struct vhost_dev
> > > > *dev,
> > > > bool enable_log)
> > > > err_vq:
> > > > for (; i >= 0; --i) {
> > > > idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index +
> > > > i);
> > > > - t = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> > > > - dev->log_enabled);
> > > > - assert(t >= 0);
> > > > + vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> > > > + dev->log_enabled);
> > > > }
> > > > - t = vhost_dev_set_features(dev, dev->log_enabled);
> > > > - assert(t >= 0);
> > > > + vhost_dev_set_features(dev, dev->log_enabled);
> > > > err_features:
> > > > return r;
> > > > }
> > > > @@ -937,7 +946,6 @@ static void vhost_virtqueue_stop(struct vhost_dev
> > > > *dev,
> > > > }
> > > > }
> > > >
> > > > - assert (r >= 0);
> > > > cpu_physical_memory_unmap(vq->ring,
> > > > virtio_queue_get_ring_size(vdev,
> > > > idx),
> > > > 0, virtio_queue_get_ring_size(vdev,
> > > > idx));
> > > > cpu_physical_memory_unmap(vq->used,
> > > > virtio_queue_get_used_size(vdev,
> > > > idx),
> > > > @@ -1191,7 +1199,9 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev,
> > > > VirtIODevice *vdev, int n,
> > > >
> > > > file.index = hdev->vhost_ops->vhost_get_vq_index(hdev, n);
> > > > r = hdev->vhost_ops->vhost_set_vring_call(hdev, &file);
> > > > - assert(r >= 0);
> > > > + if (r < 0) {
> > > > + error_report("Failed to set vring call");
> > > > + }
> > > > }
> > > >
> > > > uint64_t vhost_get_features(struct vhost_dev *hdev, const int
> > > > *feature_bits,
> > > > --
> > > > 2.9.0
> > >
>
- [Qemu-devel] [PATCH v3 06/28] vhost: add vhost device only after all success, (continued)
- [Qemu-devel] [PATCH v3 06/28] vhost: add vhost device only after all success, marcandre . lureau, 2016/07/06
- [Qemu-devel] [PATCH v3 07/28] vhost: make vhost_dev_cleanup() idempotent, marcandre . lureau, 2016/07/06
- [Qemu-devel] [PATCH v3 08/28] vhost-net: always call vhost_dev_cleanup() on failure, marcandre . lureau, 2016/07/06
- [Qemu-devel] [PATCH v3 09/28] vhost: fix calling vhost_dev_cleanup() after vhost_dev_init(), marcandre . lureau, 2016/07/06
- [Qemu-devel] [PATCH v3 05/28] vhost: call vhost_log_put() on cleanup, marcandre . lureau, 2016/07/06
- [Qemu-devel] [PATCH v3 10/28] vhost: change some assert() for error_report() or silent fail, marcandre . lureau, 2016/07/06
[Qemu-devel] [PATCH v3 11/28] vhost: use error_report() instead of fprintf(stderr, ...), marcandre . lureau, 2016/07/06
[Qemu-devel] [PATCH v3 12/28] vhost-user: check qemu_chr_fe_set_msgfds() return value, marcandre . lureau, 2016/07/06
[Qemu-devel] [PATCH v3 13/28] vhost-user: check vhost_user_{read, write}() return value, marcandre . lureau, 2016/07/06
[Qemu-devel] [PATCH v3 14/28] qemu-char: check socket is actually connected, marcandre . lureau, 2016/07/06
[Qemu-devel] [PATCH v3 15/28] vhost-user: keep vhost_net after a disconnection, marcandre . lureau, 2016/07/06
[Qemu-devel] [PATCH v3 16/28] Revert "vhost-net: do not crash if backend is not present", marcandre . lureau, 2016/07/06
[Qemu-devel] [PATCH v3 17/28] get_vhost_net() should be != null after vhost_user_init, marcandre . lureau, 2016/07/06