Re: [Qemu-devel] [PATCH 06/24] vhost-user: check vhost_user_write() return value

["Michael S. Tsirkin" <mst@redhat.com>]

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