From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59762) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQ8rq-0003rF-AK for qemu-devel@nongnu.org; Thu, 21 Jul 2016 03:58:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bQ8rl-00030L-AW for qemu-devel@nongnu.org; Thu, 21 Jul 2016 03:58:01 -0400 Received: from mx3-phx2.redhat.com ([209.132.183.24]:33733) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQ8rl-00030F-1f for qemu-devel@nongnu.org; Thu, 21 Jul 2016 03:57:57 -0400 Date: Thu, 21 Jul 2016 03:57:53 -0400 (EDT) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <1468020466.6954884.1469087873857.JavaMail.zimbra@redhat.com> In-Reply-To: <20160720164529-mutt-send-email-mst@kernel.org> References: <20160706184721.2007-1-marcandre.lureau@redhat.com> <20160706184721.2007-11-marcandre.lureau@redhat.com> <20160720162900-mutt-send-email-mst@kernel.org> <583237603.6741968.1469022086004.JavaMail.zimbra@redhat.com> <20160720164529-mutt-send-email-mst@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 10/28] vhost: change some assert() for error_report() or silent fail List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: marcandre lureau , qemu-devel@nongnu.org, mukawa@igel.co.jp, yuanhan liu , victork@redhat.com, jonshin@cisco.com Hi ----- Original Message ----- > On Wed, Jul 20, 2016 at 09:41:26AM -0400, Marc-Andr=C3=A9 Lureau wrote: > >=20 > >=20 > > ----- Original Message ----- > > > On Wed, Jul 06, 2016 at 08:47:03PM +0200, marcandre.lureau@redhat.com > > > wrote: > > > > From: Marc-Andr=C3=A9 Lureau > > > >=20 > > > > Calling a vhost operation may fail, especially with disconnectable > > > > backends. Treat some that look harmless as recoverable errors (prin= t > > > > error, or ignore on error code path). > > > >=20 > > > > Signed-off-by: Marc-Andr=C3=A9 Lureau > > >=20 > > > These might be recoverable for vhost-user not for vhost_net. > >=20 > > 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. >=20 > Interesting. This will need some thought. >=20 >=20 > > 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. >=20 > Interesting. That might be reasonable too but would increase the scope > of this already large patchset even further. >=20 >=20 > > > IMO the backend should return 0 if error is benign, > > > report errors to vhost only if they are fatal. > >=20 > > 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 high= er > > level, possibly up to the guest. >=20 > 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. >=20 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 discon= nected... and io_channel_send_full() on failure EINVAL..=20 >=20 > > > 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. > > >=20 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 t= he call, by net_vhost_user_event() handler. In most cases (there is a minor exception in set_vring_endian_legacy), vhos= t actually doesn't care about errno. However, it reports up errors using er= rno 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 t= hat failing vhost_dev_start() in vhost_scsi_start() is fatal) > > > > --- > > > > hw/virtio/vhost.c | 32 +++++++++++++++++++++----------- > > > > 1 file changed, 21 insertions(+), 11 deletions(-) > > > >=20 > > > > 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 =3D dev->vhost_ops->vhost_set_log_base(dev, log_base, log); > > > > - assert(r >=3D 0); > > > > + if (r < 0) { > > > > + error_report("Failed to change backend log"); > > > > + } > > > > + > > > > vhost_log_put(dev, true); > > > > dev->log =3D log; > > > > dev->log_size =3D size; > > > > @@ -567,7 +570,9 @@ static void vhost_commit(MemoryListener *listen= er) > > > > =20 > > > > if (!dev->log_enabled) { > > > > r =3D dev->vhost_ops->vhost_set_mem_table(dev, dev->mem); > > > > - assert(r >=3D 0); > > > > + if (r < 0) { > > > > + error_report("Failed to set mem table"); > > > > + } > > > > dev->memory_changed =3D false; > > > > return; > > > > } > > > > @@ -580,7 +585,9 @@ static void vhost_commit(MemoryListener *listen= er) > > > > vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER); > > > > } > > > > r =3D dev->vhost_ops->vhost_set_mem_table(dev, dev->mem); > > > > - assert(r >=3D 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 =3D 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 |=3D 0x1ULL << VHOST_F_LOG_ALL; > > > > } > > > > r =3D dev->vhost_ops->vhost_set_features(dev, features); > > > > + if (r < 0) { > > > > + error_report("Failed to set features"); > > > > + } > > > > return r < 0 ? -errno : 0; > > > > } > > > > =20 > > > > static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_lo= g) > > > > { > > > > - int r, t, i, idx; > > > > + int r, i, idx; > > > > r =3D 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 >=3D 0; --i) { > > > > idx =3D dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_in= dex + > > > > i); > > > > - t =3D vhost_virtqueue_set_addr(dev, dev->vqs + i, idx, > > > > - dev->log_enabled); > > > > - assert(t >=3D 0); > > > > + vhost_virtqueue_set_addr(dev, dev->vqs + i, idx, > > > > + dev->log_enabled); > > > > } > > > > - t =3D vhost_dev_set_features(dev, dev->log_enabled); > > > > - assert(t >=3D 0); > > > > + vhost_dev_set_features(dev, dev->log_enabled); > > > > err_features: > > > > return r; > > > > } > > > > @@ -937,7 +946,6 @@ static void vhost_virtqueue_stop(struct vhost_d= ev > > > > *dev, > > > > } > > > > } > > > > =20 > > > > - assert (r >=3D 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 *h= dev, > > > > VirtIODevice *vdev, int n, > > > > =20 > > > > file.index =3D hdev->vhost_ops->vhost_get_vq_index(hdev, n); > > > > r =3D hdev->vhost_ops->vhost_set_vring_call(hdev, &file); > > > > - assert(r >=3D 0); > > > > + if (r < 0) { > > > > + error_report("Failed to set vring call"); > > > > + } > > > > } > > > > =20 > > > > uint64_t vhost_get_features(struct vhost_dev *hdev, const int > > > > *feature_bits, > > > > -- > > > > 2.9.0 > > >=20 >=20