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