[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] libvhost-user: handle NOFD flag in call/kick/er
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH] libvhost-user: handle NOFD flag in call/kick/err better |
Date: |
Fri, 20 Sep 2019 09:58:12 +0100 |
User-agent: |
Mutt/1.12.1 (2019-06-15) |
On Wed, Sep 18, 2019 at 11:49:14AM +0200, Johannes Berg wrote:
> 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.
Yeah, there is a trade-off to hiding NOFD and if what I proposed isn't
convincing then it wasn't a good proposal :-):
Reviewed-by: Stefan Hajnoczi <address@hidden>
signature.asc
Description: PGP signature