----- Original Message ----- > On Wed, Jul 06, 2016 at 08:47:03PM +0200, marcandre.lur...@redhat.com wrote: > > From: Marc-André Lureau <marcandre.lur...@redhat.com> > > > > 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 <marcandre.lur...@redhat.com> > > 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. 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. > 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. > 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. > > Also, we might want to distinguish between EBADF (fd closed) > and other types of errors. All this knowledge belomgs in vhost user. > > > --- > > 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 >