On Wed, 2 Mar 2022 16:23:42 +0100 Sergio Lopez <slp@xxxxxxxxxx> wrote: > 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@xxxxxxxxxx> 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). I'd argue that adding a bool as an arg to a function to change the return value is sufficiently non-intuitive to program for that the wrapper method is actually more correct. event_notifier_get_fd() essentially becomes a shorthand for event_notifier_get_rfd(). Thanks, Alex
