Re: [PATCH 1/2] Allow returning EventNotifier's wfd

[Sergio Lopez]

On Wed, Mar 02, 2022 at 08:12:34AM -0700, Alex Williamson wrote:
> On Wed,  2 Mar 2022 12:36:43 +0100
> Sergio Lopez <slp@redhat.com> wrote:
> 
> > event_notifier_get_fd(const EventNotifier *e) always returns
> > EventNotifier's read file descriptor (rfd). This is not a problem when
> > the EventNotifier is backed by a an eventfd, as a single file
> > descriptor is used both for reading and triggering events (rfd ==
> > wfd).
> > 
> > But, when EventNotifier is backed by a pipefd, we have two file
> > descriptors, one that can only be used for reads (rfd), and the other
> > only for writes (wfd).
> > 
> > There's, at least, one known situation in which we need to obtain wfd
> > instead of rfd, which is when setting up the file that's going to be
> > sent to the peer in vhost's SET_VRING_CALL.
> > 
> > Extend event_notifier_get_fd() to receive an argument which indicates
> > whether the caller wants to obtain rfd (false) or wfd (true).
> 
> There are about 50 places where we add the false arg here and 1 where
> we use true.  Seems it would save a lot of churn to hide this
> internally, event_notifier_get_fd() returns an rfd, a new
> event_notifier_get_wfd() returns the wfd.  Thanks,

I agree. In fact, that's what I implemented in the first place. I
changed to this version in which event_notifier_get_fd() is extended
because it feels more "correct". But yes, the pragmatic option would
be adding a new event_notifier_get_wfd().

I'll wait for more reviews, and unless someone voices against it, I'll
respin the patches with that strategy (I already have it around here).

Thanks,
Sergio.

signature.asc
Description: PGP signature