From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59389) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQ8pA-0001jA-PA for qemu-devel@nongnu.org; Thu, 21 Jul 2016 03:55:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bQ8p5-0002WO-Pu for qemu-devel@nongnu.org; Thu, 21 Jul 2016 03:55:16 -0400 Received: from mx3-phx2.redhat.com ([209.132.183.24]:33659) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQ8p5-0002W5-IU for qemu-devel@nongnu.org; Thu, 21 Jul 2016 03:55:11 -0400 Date: Thu, 21 Jul 2016 03:55:05 -0400 (EDT) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <1208335302.6954578.1469087705171.JavaMail.zimbra@redhat.com> In-Reply-To: <20160720162630-mutt-send-email-mst@kernel.org> References: <20160706184721.2007-1-marcandre.lureau@redhat.com> <20160706184721.2007-14-marcandre.lureau@redhat.com> <20160720162630-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 13/28] vhost-user: check vhost_user_{read, write}() return value 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 06, 2016 at 08:47:06PM +0200, marcandre.lureau@redhat.com wro= te: > > From: Marc-Andr=C3=A9 Lureau > >=20 > > More error checking to break code flow and report appropriate errors. > >=20 > > Signed-off-by: Marc-Andr=C3=A9 Lureau >=20 > So this will cause asserts here: >=20 > /* 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); That's why there is a previous patch "vhost: do not assert() on vhost_ops f= ailure". Although there is still some abort() on migration. This is a corne= r case that isn't yet solved. I think it would make sense for the backend t= o state that it is disconnected, and for vhost to ingore migration ops fail= ure in this case. > which is the opposite of what we want. >=20 > For vhost_net ioctl failure is a fatal error bit for > vhost user this means backend exited and will not change > memory so it's benign. >=20 > the right thing to do is to return 0 status to caller on most errors. >=20 > If that is necessary for debugging, add a printout for now. Yes, I am adding a VHOST_OPS_DEBUG as you proposed >=20 > > --- > > hw/virtio/vhost-user.c | 50 > > ++++++++++++++++++++++++++++++++++---------------- > > 1 file changed, 34 insertions(+), 16 deletions(-) > >=20 > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > > index 5dae496..819481d 100644 > > --- a/hw/virtio/vhost-user.c > > +++ b/hw/virtio/vhost-user.c > > @@ -214,12 +214,14 @@ static int vhost_user_set_log_base(struct vhost_d= ev > > *dev, uint64_t base, > > fds[fd_num++] =3D log->fd; > > } > > =20 > > - vhost_user_write(dev, &msg, fds, fd_num); > > + if (vhost_user_write(dev, &msg, fds, fd_num) < 0) { > > + return -1; > > + } > > =20 > > if (shmfd) { > > msg.size =3D 0; > > if (vhost_user_read(dev, &msg) < 0) { > > - return 0; > > + return -1; > > } > > =20 > > if (msg.request !=3D VHOST_USER_SET_LOG_BASE) { > > @@ -275,7 +277,9 @@ static int vhost_user_set_mem_table(struct vhost_de= v > > *dev, > > msg.size +=3D sizeof(msg.payload.memory.padding); > > msg.size +=3D fd_num * sizeof(VhostUserMemoryRegion); > > =20 > > - vhost_user_write(dev, &msg, fds, fd_num); > > + if (vhost_user_write(dev, &msg, fds, fd_num) < 0) { > > + return -1; > > + } > > =20 > > return 0; > > } > > @@ -290,7 +294,9 @@ static int vhost_user_set_vring_addr(struct vhost_d= ev > > *dev, > > .size =3D sizeof(msg.payload.addr), > > }; > > =20 > > - vhost_user_write(dev, &msg, NULL, 0); > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > + return -1; > > + } > > =20 > > return 0; > > } > > @@ -313,7 +319,9 @@ static int vhost_set_vring(struct vhost_dev *dev, > > .size =3D sizeof(msg.payload.state), > > }; > > =20 > > - vhost_user_write(dev, &msg, NULL, 0); > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > + return -1; > > + } > > =20 > > return 0; > > } > > @@ -360,10 +368,12 @@ static int vhost_user_get_vring_base(struct vhost= _dev > > *dev, > > .size =3D sizeof(msg.payload.state), > > }; > > =20 > > - vhost_user_write(dev, &msg, NULL, 0); > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > + return -1; > > + } > > =20 > > if (vhost_user_read(dev, &msg) < 0) { > > - return 0; > > + return -1; > > } > > =20 > > if (msg.request !=3D VHOST_USER_GET_VRING_BASE) { > > @@ -401,7 +411,9 @@ static int vhost_set_vring_file(struct vhost_dev *d= ev, > > msg.payload.u64 |=3D VHOST_USER_VRING_NOFD_MASK; > > } > > =20 > > - vhost_user_write(dev, &msg, fds, fd_num); > > + if (vhost_user_write(dev, &msg, fds, fd_num) < 0) { > > + return -1; > > + } > > =20 > > return 0; > > } > > @@ -427,7 +439,9 @@ static int vhost_user_set_u64(struct vhost_dev *dev= , > > int request, uint64_t u64) > > .size =3D sizeof(msg.payload.u64), > > }; > > =20 > > - vhost_user_write(dev, &msg, NULL, 0); > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > + return -1; > > + } > > =20 > > return 0; > > } > > @@ -455,10 +469,12 @@ static int vhost_user_get_u64(struct vhost_dev *d= ev, > > int request, uint64_t *u64) > > return 0; > > } > > =20 > > - vhost_user_write(dev, &msg, NULL, 0); > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > + return -1; > > + } > > =20 > > if (vhost_user_read(dev, &msg) < 0) { > > - return 0; > > + return -1; > > } > > =20 > > if (msg.request !=3D request) { > > @@ -489,7 +505,9 @@ static int vhost_user_set_owner(struct vhost_dev *d= ev) > > .flags =3D VHOST_USER_VERSION, > > }; > > =20 > > - vhost_user_write(dev, &msg, NULL, 0); > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > + return -1; > > + } > > =20 > > return 0; > > } > > @@ -501,7 +519,9 @@ static int vhost_user_reset_device(struct vhost_dev > > *dev) > > .flags =3D VHOST_USER_VERSION, > > }; > > =20 > > - vhost_user_write(dev, &msg, NULL, 0); > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > + return -1; > > + } > > =20 > > return 0; > > } > > @@ -588,7 +608,6 @@ static bool vhost_user_requires_shm_log(struct > > vhost_dev *dev) > > static int vhost_user_migration_done(struct vhost_dev *dev, char* > > mac_addr) > > { > > VhostUserMsg msg =3D { 0 }; > > - int err; > > =20 > > assert(dev->vhost_ops->backend_type =3D=3D VHOST_BACKEND_TYPE_USER= ); > > =20 > > @@ -605,8 +624,7 @@ static int vhost_user_migration_done(struct vhost_d= ev > > *dev, char* mac_addr) > > memcpy((char *)&msg.payload.u64, mac_addr, 6); > > msg.size =3D sizeof(msg.payload.u64); > > =20 > > - err =3D vhost_user_write(dev, &msg, NULL, 0); > > - return err; > > + return vhost_user_write(dev, &msg, NULL, 0); > > } > > return -1; > > } > > -- > > 2.9.0 >=20