[v3,10/28] vhost: change some assert() for error_report() or silent fail

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>
---
 hw/virtio/vhost.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

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>

So this is handy for now until disconnect handling stabilizes,
but eventually these bnign errors should be hidden from the user.
What I'd like to see is something like

#define VHOST_DISCONNECT_DEBUG error_report

and then use that.

After things stabilize we'll be able to drop these easily.



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

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.

IMO the backend should return 0 if error is benign,
report errors to vhost only if they are fatal.

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

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

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

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.


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

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