Re: [Qemu-devel] [PATCH v3 10/28] vhost: change some assert() for error_report() or silent fail

["Marc-André Lureau" <mlureau@redhat.com>]

----- Original Message -----
> On Wed, Jul 06, 2016 at 08:47:03PM +0200, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@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.lureau@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
>