Re: [Qemu-devel] [PATCH] libvhost-user: handle NOFD flag in call/kick/err better

[Johannes Berg <johannes@sipsolutions.net>]

On Wed, 2019-09-18 at 10:39 +0100, Stefan Hajnoczi wrote:
> 
> >  vu_check_queue_msg_file(VuDev *dev, VhostUserMsg *vmsg)
> >  {
> >      int index = vmsg->payload.u64 & VHOST_USER_VRING_IDX_MASK;
> > +    bool nofd = vmsg->payload.u64 & VHOST_USER_VRING_NOFD_MASK;
> >  
> >      if (index >= dev->max_queues) {
> >          vmsg_close_fds(vmsg);
> > @@ -927,8 +928,12 @@ vu_check_queue_msg_file(VuDev *dev, VhostUserMsg *vmsg)
> >          return false;
> >      }
> >  
> > -    if (vmsg->payload.u64 & VHOST_USER_VRING_NOFD_MASK ||
> > -        vmsg->fd_num != 1) {
> > +    if (nofd) {
> > +        vmsg_close_fds(vmsg);
> > +        return true;
> > +    }

So in this particular code you quoted, I actually just aligned to have
the same "bool nofd" variable - and I made it return "true" when no FD
was given.

It couldn't make use of what you proposed:

> With the following change to vmsg_close_fds():
> 
>   for (i = 0; i < vmsg->fd_num; i++) {
>       close(vmsg->fds[i]);
>   }
> + for (i = 0; i < sizeof(vmsg->fd_num) / sizeof(vmsg->fd_num[0]); i++) {
> +     vmsg->fds[i] = -1;
> + }
> + vmsg->fd_num = 0;
> 
> ...the message handler functions below can use vmsg->fds[0] (-1) without
> worrying about NOFD.  This makes the code simpler.

because fd_num != 1 leads to the original code returning false, which
leads to the ring not getting started in vu_set_vring_kick_exec(). So we
need the special code here, can be argued if I should pull out the test
into the "bool nofd" variable or not ... *shrug*

The changes in vu_set_vring_kick_exec() and vu_set_vring_err_exec()
would indeed then not be necessary, but in vu_set_vring_call_exec() we
should still avoid the eventfd_write() if it's going to get -1.


So, yeah - could be a bit simpler there. I'd say being explicit here is
easier to understand and thus nicer, but your (or Michael's I guess?)
call.

johannes