Message ID | 1466503372-28334-7-git-send-email-marcandre.lureau@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, Jun 21, 2016 at 12:02:34PM +0200, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > Just some more error checking. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Point being? Callers just ignore it afterwards ... > --- > hw/virtio/vhost-user.c | 44 +++++++++++++++++++++++++++++++------------- > 1 file changed, 31 insertions(+), 13 deletions(-) > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index 5dae496..e51df27 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -214,7 +214,9 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base, > fds[fd_num++] = log->fd; > } > > - vhost_user_write(dev, &msg, fds, fd_num); > + if (vhost_user_write(dev, &msg, fds, fd_num) < 0) { > + return -1; > + } > > if (shmfd) { > msg.size = 0; > @@ -275,7 +277,9 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev, > msg.size += sizeof(msg.payload.memory.padding); > msg.size += fd_num * sizeof(VhostUserMemoryRegion); > > - vhost_user_write(dev, &msg, fds, fd_num); > + if (vhost_user_write(dev, &msg, fds, fd_num) < 0) { > + return -1; > + } > > return 0; > } > @@ -290,7 +294,9 @@ static int vhost_user_set_vring_addr(struct vhost_dev *dev, > .size = sizeof(msg.payload.addr), > }; > > - vhost_user_write(dev, &msg, NULL, 0); > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > + return -1; > + } > > return 0; > } > @@ -313,7 +319,9 @@ static int vhost_set_vring(struct vhost_dev *dev, > .size = sizeof(msg.payload.state), > }; > > - vhost_user_write(dev, &msg, NULL, 0); > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > + return -1; > + } > > return 0; > } > @@ -360,7 +368,9 @@ static int vhost_user_get_vring_base(struct vhost_dev *dev, > .size = sizeof(msg.payload.state), > }; > > - vhost_user_write(dev, &msg, NULL, 0); > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > + return -1; > + } > > if (vhost_user_read(dev, &msg) < 0) { > return 0; > @@ -401,7 +411,9 @@ static int vhost_set_vring_file(struct vhost_dev *dev, > msg.payload.u64 |= VHOST_USER_VRING_NOFD_MASK; > } > > - vhost_user_write(dev, &msg, fds, fd_num); > + if (vhost_user_write(dev, &msg, fds, fd_num) < 0) { > + return -1; > + } > > return 0; > } > @@ -427,7 +439,9 @@ static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64) > .size = sizeof(msg.payload.u64), > }; > > - vhost_user_write(dev, &msg, NULL, 0); > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > + return -1; > + } > > return 0; > } > @@ -455,7 +469,9 @@ static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t *u64) > return 0; > } > > - vhost_user_write(dev, &msg, NULL, 0); > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > + return -1; > + } > > if (vhost_user_read(dev, &msg) < 0) { > return 0; > @@ -489,7 +505,9 @@ static int vhost_user_set_owner(struct vhost_dev *dev) > .flags = VHOST_USER_VERSION, > }; > > - vhost_user_write(dev, &msg, NULL, 0); > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > + return -1; > + } > > return 0; > } > @@ -501,7 +519,9 @@ static int vhost_user_reset_device(struct vhost_dev *dev) > .flags = VHOST_USER_VERSION, > }; > > - vhost_user_write(dev, &msg, NULL, 0); > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > + return -1; > + } > > 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 = { 0 }; > - int err; > > assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER); > > @@ -605,8 +624,7 @@ static int vhost_user_migration_done(struct vhost_dev *dev, char* mac_addr) > memcpy((char *)&msg.payload.u64, mac_addr, 6); > msg.size = sizeof(msg.payload.u64); > > - err = vhost_user_write(dev, &msg, NULL, 0); > - return err; > + return vhost_user_write(dev, &msg, NULL, 0); > } > return -1; > } > -- > 2.7.4
Hi ----- Original Message ----- > On Tue, Jun 21, 2016 at 12:02:34PM +0200, marcandre.lureau@redhat.com wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > Just some more error checking. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Point being? Callers just ignore it afterwards ... Callers do not always ignore it (and in general it should not, should it?), this helps breaking the execution at right moment, help debugging, code consistency, good practices etc (perhaps it's too obvious to me and I am missing something?) > > > > --- > > hw/virtio/vhost-user.c | 44 +++++++++++++++++++++++++++++++------------- > > 1 file changed, 31 insertions(+), 13 deletions(-) > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > > index 5dae496..e51df27 100644 > > --- a/hw/virtio/vhost-user.c > > +++ b/hw/virtio/vhost-user.c > > @@ -214,7 +214,9 @@ static int vhost_user_set_log_base(struct vhost_dev > > *dev, uint64_t base, > > fds[fd_num++] = log->fd; > > } > > > > - vhost_user_write(dev, &msg, fds, fd_num); > > + if (vhost_user_write(dev, &msg, fds, fd_num) < 0) { > > + return -1; > > + } > > > > if (shmfd) { > > msg.size = 0; > > @@ -275,7 +277,9 @@ static int vhost_user_set_mem_table(struct vhost_dev > > *dev, > > msg.size += sizeof(msg.payload.memory.padding); > > msg.size += fd_num * sizeof(VhostUserMemoryRegion); > > > > - vhost_user_write(dev, &msg, fds, fd_num); > > + if (vhost_user_write(dev, &msg, fds, fd_num) < 0) { > > + return -1; > > + } > > > > return 0; > > } > > @@ -290,7 +294,9 @@ static int vhost_user_set_vring_addr(struct vhost_dev > > *dev, > > .size = sizeof(msg.payload.addr), > > }; > > > > - vhost_user_write(dev, &msg, NULL, 0); > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > + return -1; > > + } > > > > return 0; > > } > > @@ -313,7 +319,9 @@ static int vhost_set_vring(struct vhost_dev *dev, > > .size = sizeof(msg.payload.state), > > }; > > > > - vhost_user_write(dev, &msg, NULL, 0); > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > + return -1; > > + } > > > > return 0; > > } > > @@ -360,7 +368,9 @@ static int vhost_user_get_vring_base(struct vhost_dev > > *dev, > > .size = sizeof(msg.payload.state), > > }; > > > > - vhost_user_write(dev, &msg, NULL, 0); > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > + return -1; > > + } > > > > if (vhost_user_read(dev, &msg) < 0) { > > return 0; > > @@ -401,7 +411,9 @@ static int vhost_set_vring_file(struct vhost_dev *dev, > > msg.payload.u64 |= VHOST_USER_VRING_NOFD_MASK; > > } > > > > - vhost_user_write(dev, &msg, fds, fd_num); > > + if (vhost_user_write(dev, &msg, fds, fd_num) < 0) { > > + return -1; > > + } > > > > return 0; > > } > > @@ -427,7 +439,9 @@ static int vhost_user_set_u64(struct vhost_dev *dev, > > int request, uint64_t u64) > > .size = sizeof(msg.payload.u64), > > }; > > > > - vhost_user_write(dev, &msg, NULL, 0); > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > + return -1; > > + } > > > > return 0; > > } > > @@ -455,7 +469,9 @@ static int vhost_user_get_u64(struct vhost_dev *dev, > > int request, uint64_t *u64) > > return 0; > > } > > > > - vhost_user_write(dev, &msg, NULL, 0); > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > + return -1; > > + } > > > > if (vhost_user_read(dev, &msg) < 0) { > > return 0; > > @@ -489,7 +505,9 @@ static int vhost_user_set_owner(struct vhost_dev *dev) > > .flags = VHOST_USER_VERSION, > > }; > > > > - vhost_user_write(dev, &msg, NULL, 0); > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > + return -1; > > + } > > > > return 0; > > } > > @@ -501,7 +519,9 @@ static int vhost_user_reset_device(struct vhost_dev > > *dev) > > .flags = VHOST_USER_VERSION, > > }; > > > > - vhost_user_write(dev, &msg, NULL, 0); > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > + return -1; > > + } > > > > 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 = { 0 }; > > - int err; > > > > assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER); > > > > @@ -605,8 +624,7 @@ static int vhost_user_migration_done(struct vhost_dev > > *dev, char* mac_addr) > > memcpy((char *)&msg.payload.u64, mac_addr, 6); > > msg.size = sizeof(msg.payload.u64); > > > > - err = vhost_user_write(dev, &msg, NULL, 0); > > - return err; > > + return vhost_user_write(dev, &msg, NULL, 0); > > } > > return -1; > > } > > -- > > 2.7.4 >
On Thu, Jun 23, 2016 at 05:16:18AM -0400, Marc-André Lureau wrote: > Hi > > ----- Original Message ----- > > On Tue, Jun 21, 2016 at 12:02:34PM +0200, marcandre.lureau@redhat.com wrote: > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > > > Just some more error checking. > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > Point being? Callers just ignore it afterwards ... > > Callers do not always ignore it (and in general it should not, should it?), this helps breaking the execution at right moment, help debugging, code consistency, good practices etc (perhaps it's too obvious to me and I am missing something?) Reporting error up the stack is helpful if it's handled in some way. If we just keep guest going on this error, then we could maybe log it for debug build but that's all. > > > > > > > --- > > > hw/virtio/vhost-user.c | 44 +++++++++++++++++++++++++++++++------------- > > > 1 file changed, 31 insertions(+), 13 deletions(-) > > > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > > > index 5dae496..e51df27 100644 > > > --- a/hw/virtio/vhost-user.c > > > +++ b/hw/virtio/vhost-user.c > > > @@ -214,7 +214,9 @@ static int vhost_user_set_log_base(struct vhost_dev > > > *dev, uint64_t base, > > > fds[fd_num++] = log->fd; > > > } > > > > > > - vhost_user_write(dev, &msg, fds, fd_num); > > > + if (vhost_user_write(dev, &msg, fds, fd_num) < 0) { > > > + return -1; > > > + } > > > > > > if (shmfd) { > > > msg.size = 0; > > > @@ -275,7 +277,9 @@ static int vhost_user_set_mem_table(struct vhost_dev > > > *dev, > > > msg.size += sizeof(msg.payload.memory.padding); > > > msg.size += fd_num * sizeof(VhostUserMemoryRegion); > > > > > > - vhost_user_write(dev, &msg, fds, fd_num); > > > + if (vhost_user_write(dev, &msg, fds, fd_num) < 0) { > > > + return -1; > > > + } > > > > > > return 0; > > > } > > > @@ -290,7 +294,9 @@ static int vhost_user_set_vring_addr(struct vhost_dev > > > *dev, > > > .size = sizeof(msg.payload.addr), > > > }; > > > > > > - vhost_user_write(dev, &msg, NULL, 0); > > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > > + return -1; > > > + } > > > > > > return 0; > > > } > > > @@ -313,7 +319,9 @@ static int vhost_set_vring(struct vhost_dev *dev, > > > .size = sizeof(msg.payload.state), > > > }; > > > > > > - vhost_user_write(dev, &msg, NULL, 0); > > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > > + return -1; > > > + } > > > > > > return 0; > > > } > > > @@ -360,7 +368,9 @@ static int vhost_user_get_vring_base(struct vhost_dev > > > *dev, > > > .size = sizeof(msg.payload.state), > > > }; > > > > > > - vhost_user_write(dev, &msg, NULL, 0); > > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > > + return -1; > > > + } > > > > > > if (vhost_user_read(dev, &msg) < 0) { > > > return 0; > > > @@ -401,7 +411,9 @@ static int vhost_set_vring_file(struct vhost_dev *dev, > > > msg.payload.u64 |= VHOST_USER_VRING_NOFD_MASK; > > > } > > > > > > - vhost_user_write(dev, &msg, fds, fd_num); > > > + if (vhost_user_write(dev, &msg, fds, fd_num) < 0) { > > > + return -1; > > > + } > > > > > > return 0; > > > } > > > @@ -427,7 +439,9 @@ static int vhost_user_set_u64(struct vhost_dev *dev, > > > int request, uint64_t u64) > > > .size = sizeof(msg.payload.u64), > > > }; > > > > > > - vhost_user_write(dev, &msg, NULL, 0); > > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > > + return -1; > > > + } > > > > > > return 0; > > > } > > > @@ -455,7 +469,9 @@ static int vhost_user_get_u64(struct vhost_dev *dev, > > > int request, uint64_t *u64) > > > return 0; > > > } > > > > > > - vhost_user_write(dev, &msg, NULL, 0); > > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > > + return -1; > > > + } > > > > > > if (vhost_user_read(dev, &msg) < 0) { > > > return 0; > > > @@ -489,7 +505,9 @@ static int vhost_user_set_owner(struct vhost_dev *dev) > > > .flags = VHOST_USER_VERSION, > > > }; > > > > > > - vhost_user_write(dev, &msg, NULL, 0); > > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > > + return -1; > > > + } > > > > > > return 0; > > > } > > > @@ -501,7 +519,9 @@ static int vhost_user_reset_device(struct vhost_dev > > > *dev) > > > .flags = VHOST_USER_VERSION, > > > }; > > > > > > - vhost_user_write(dev, &msg, NULL, 0); > > > + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { > > > + return -1; > > > + } > > > > > > 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 = { 0 }; > > > - int err; > > > > > > assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER); > > > > > > @@ -605,8 +624,7 @@ static int vhost_user_migration_done(struct vhost_dev > > > *dev, char* mac_addr) > > > memcpy((char *)&msg.payload.u64, mac_addr, 6); > > > msg.size = sizeof(msg.payload.u64); > > > > > > - err = vhost_user_write(dev, &msg, NULL, 0); > > > - return err; > > > + return vhost_user_write(dev, &msg, NULL, 0); > > > } > > > return -1; > > > } > > > -- > > > 2.7.4 > >
Hi On Thu, Jun 23, 2016 at 7:08 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> >> Callers do not always ignore it (and in general it should not, should it?), this helps breaking the execution at right moment, help debugging, code consistency, good practices etc (perhaps it's too obvious to me and I am missing something?) > > Reporting error up the stack is helpful if it's handled in some way. > If we just keep guest going on this error, then we could > maybe log it for debug build but that's all. Reporting up to guest somehow would be a good thing at some point, so I think we should start from the bottom. vhost-user lacks error handling, let's add it. Regarding debug build messages, I don't think it's enough. As long as we don't have an official supported way to handle disconnect. It's better to report an error than be silent.
On Fri, Jun 24, 2016 at 02:49:22PM +0200, Marc-André Lureau wrote: > Hi > > On Thu, Jun 23, 2016 at 7:08 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > >> > >> Callers do not always ignore it (and in general it should not, should it?), this helps breaking the execution at right moment, help debugging, code consistency, good practices etc (perhaps it's too obvious to me and I am missing something?) > > > > Reporting error up the stack is helpful if it's handled in some way. > > If we just keep guest going on this error, then we could > > maybe log it for debug build but that's all. > > Reporting up to guest somehow would be a good thing at some point, I'm not so sure. If backend will reconnect shortly, we can handle everything transparently. > so > I think we should start from the bottom. vhost-user lacks error > handling, let's add it. > > Regarding debug build messages, I don't think it's enough. As long as > we don't have an official supported way to handle disconnect. It's > better to report an error than be silent. Let's just work on handling it. If we need debug messages to help us reach that goal fine. But I don't see many reasons to propagate return codes back and forth if caller just prints and ignores it. Print it where it's detected :) > > -- > Marc-André Lureau
On Mon, Jul 4, 2016 at 5:46 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > Let's just work on handling it. If we need debug messages to help us > reach that goal fine. But I don't see many reasons to propagate > return codes back and forth if caller just prints and ignores it. > Print it where it's detected :) Adding more debug messages is not the point of this patch though, it is to break the code flow when an error occurs, not later. Sure we can add error_report() in vhost_user_write() that would be more consistent with vhost_user_read() actually.
On Tue, Jul 05, 2016 at 12:01:49AM +0200, Marc-André Lureau wrote: > On Mon, Jul 4, 2016 at 5:46 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > Let's just work on handling it. If we need debug messages to help us > > reach that goal fine. But I don't see many reasons to propagate > > return codes back and forth if caller just prints and ignores it. > > Print it where it's detected :) > > > Adding more debug messages is not the point of this patch though, it > is to break the code flow when an error occurs, not later. Sure we can > add error_report() in vhost_user_write() that would be more consistent > with vhost_user_read() actually. Yes but we should look for more ways to continue after an error, not for ways to break the flow IMHO. > > -- > Marc-André Lureau
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 5dae496..e51df27 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -214,7 +214,9 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base, fds[fd_num++] = log->fd; } - vhost_user_write(dev, &msg, fds, fd_num); + if (vhost_user_write(dev, &msg, fds, fd_num) < 0) { + return -1; + } if (shmfd) { msg.size = 0; @@ -275,7 +277,9 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev, msg.size += sizeof(msg.payload.memory.padding); msg.size += fd_num * sizeof(VhostUserMemoryRegion); - vhost_user_write(dev, &msg, fds, fd_num); + if (vhost_user_write(dev, &msg, fds, fd_num) < 0) { + return -1; + } return 0; } @@ -290,7 +294,9 @@ static int vhost_user_set_vring_addr(struct vhost_dev *dev, .size = sizeof(msg.payload.addr), }; - vhost_user_write(dev, &msg, NULL, 0); + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { + return -1; + } return 0; } @@ -313,7 +319,9 @@ static int vhost_set_vring(struct vhost_dev *dev, .size = sizeof(msg.payload.state), }; - vhost_user_write(dev, &msg, NULL, 0); + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { + return -1; + } return 0; } @@ -360,7 +368,9 @@ static int vhost_user_get_vring_base(struct vhost_dev *dev, .size = sizeof(msg.payload.state), }; - vhost_user_write(dev, &msg, NULL, 0); + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { + return -1; + } if (vhost_user_read(dev, &msg) < 0) { return 0; @@ -401,7 +411,9 @@ static int vhost_set_vring_file(struct vhost_dev *dev, msg.payload.u64 |= VHOST_USER_VRING_NOFD_MASK; } - vhost_user_write(dev, &msg, fds, fd_num); + if (vhost_user_write(dev, &msg, fds, fd_num) < 0) { + return -1; + } return 0; } @@ -427,7 +439,9 @@ static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64) .size = sizeof(msg.payload.u64), }; - vhost_user_write(dev, &msg, NULL, 0); + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { + return -1; + } return 0; } @@ -455,7 +469,9 @@ static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t *u64) return 0; } - vhost_user_write(dev, &msg, NULL, 0); + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { + return -1; + } if (vhost_user_read(dev, &msg) < 0) { return 0; @@ -489,7 +505,9 @@ static int vhost_user_set_owner(struct vhost_dev *dev) .flags = VHOST_USER_VERSION, }; - vhost_user_write(dev, &msg, NULL, 0); + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { + return -1; + } return 0; } @@ -501,7 +519,9 @@ static int vhost_user_reset_device(struct vhost_dev *dev) .flags = VHOST_USER_VERSION, }; - vhost_user_write(dev, &msg, NULL, 0); + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { + return -1; + } 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 = { 0 }; - int err; assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER); @@ -605,8 +624,7 @@ static int vhost_user_migration_done(struct vhost_dev *dev, char* mac_addr) memcpy((char *)&msg.payload.u64, mac_addr, 6); msg.size = sizeof(msg.payload.u64); - err = vhost_user_write(dev, &msg, NULL, 0); - return err; + return vhost_user_write(dev, &msg, NULL, 0); } return -1; }