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

[Marc-André Lureau]

Hi

----- Original Message -----
> On Tue, Jun 21, 2016 at 12:02:33PM +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>
> 
> If it's ok and we can recover, then why should we print errors?

To me, the current disconnect handling is not handled cleanly. There is not 
much/nothing in the protocol that tells when and how you can disconnect. If 
qemu vhost-user reconnection works today, it is mostly by luck. Imho, we should 
print errors if any call to vhost user fails to help further analysis of broken 
behaviour. And we shoud have a clean mechanism to handle 
shutdown/disconnect/reconnect.

> 
> > ---
> >  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 81cc5b0..e2d1614 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -398,7 +398,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;
> > @@ -565,7 +568,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;
> >      }
> > @@ -578,7 +583,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);
> > @@ -647,6 +654,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;
> > @@ -660,12 +668,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;
> > @@ -682,12 +693,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),
> > @@ -1187,7 +1195,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.7.4
>