[libvirt] [PATCH] util: eventpoll: Survive EBADF on macOS

Roman Bolshakov posted 1 patch 5 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180821211203.99194-1-r.bolshakov@yadro.com
Test syntax-check passed
There is a newer version of this series
src/util/vireventpoll.c | 8 ++++++++
1 file changed, 8 insertions(+)
[libvirt] [PATCH] util: eventpoll: Survive EBADF on macOS
Posted by Roman Bolshakov 5 years, 7 months ago
Fixes:
https://www.redhat.com/archives/libvir-list/2017-January/msg00978.html

QEMU is probed through monitor fd to check capabilities during libvirtd init.
The monitor fd is closed after probing by virQEMUCapsInitQMPCommandFree
that calls virQEMUCapsInitQMPCommandAbort that calls qemuMonitorClose,
the latter one notifies the event loop via an interrupt handle in
qemuMonitorUnregister and after then closes monitor fd.

There could be a case when interrupt is sent after eventLoop is unlocked
but before virEventPollRunOnce blocks in poll, shortly before file
descriptor is closed by qemuMonitorClose. Then poll receives closed monitor
fd in fdset and returns EBADF.

EBADF is not mentioned as a valid errno on macOS poll man-page but such
behaviour can appear release-to-release, according to cpython:
https://github.com/python/cpython/blob/master/Modules/selectmodule.c#L1161

The change also fixes the issue in qemucapabilitiestest. It returns
Bad file descriptor message 25 times without the fix.

Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 src/util/vireventpoll.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/util/vireventpoll.c b/src/util/vireventpoll.c
index 13d278df13..6d19374c52 100644
--- a/src/util/vireventpoll.c
+++ b/src/util/vireventpoll.c
@@ -643,10 +643,18 @@ int virEventPollRunOnce(void)
         EVENT_DEBUG("Poll got error event %d", errno);
         if (errno == EINTR || errno == EAGAIN)
             goto retry;
+#ifdef __APPLE__
+        if (errno == EBADF) {
+            ret = 0;
+            goto dispatch;
+        }
+#endif
         virReportSystemError(errno, "%s",
                              _("Unable to poll on file handles"));
         return -1;
     }
+
+ dispatch:
     EVENT_DEBUG("Poll got %d event(s)", ret);
 
     virMutexLock(&eventLoop.lock);
-- 
2.15.2 (Apple Git-101.1)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: eventpoll: Survive EBADF on macOS
Posted by Daniel P. Berrangé 5 years, 7 months ago
On Wed, Aug 22, 2018 at 12:12:03AM +0300, Roman Bolshakov wrote:
> Fixes:
> https://www.redhat.com/archives/libvir-list/2017-January/msg00978.html
> 
> QEMU is probed through monitor fd to check capabilities during libvirtd init.
> The monitor fd is closed after probing by virQEMUCapsInitQMPCommandFree
> that calls virQEMUCapsInitQMPCommandAbort that calls qemuMonitorClose,
> the latter one notifies the event loop via an interrupt handle in
> qemuMonitorUnregister and after then closes monitor fd.
> 
> There could be a case when interrupt is sent after eventLoop is unlocked
> but before virEventPollRunOnce blocks in poll, shortly before file
> descriptor is closed by qemuMonitorClose. Then poll receives closed monitor
> fd in fdset and returns EBADF.
> 
> EBADF is not mentioned as a valid errno on macOS poll man-page but such
> behaviour can appear release-to-release, according to cpython:
> https://github.com/python/cpython/blob/master/Modules/selectmodule.c#L1161

Yeah, this really does look like broken macOS behaviour, as it should
be returning POLLNVAL instead.

> 
> The change also fixes the issue in qemucapabilitiestest. It returns
> Bad file descriptor message 25 times without the fix.
> 
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
>  src/util/vireventpoll.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/util/vireventpoll.c b/src/util/vireventpoll.c
> index 13d278df13..6d19374c52 100644
> --- a/src/util/vireventpoll.c
> +++ b/src/util/vireventpoll.c
> @@ -643,10 +643,18 @@ int virEventPollRunOnce(void)
>          EVENT_DEBUG("Poll got error event %d", errno);
>          if (errno == EINTR || errno == EAGAIN)
>              goto retry;
> +#ifdef __APPLE__
> +        if (errno == EBADF) {
> +            ret = 0;
> +            goto dispatch;
> +        }
> +#endif
>          virReportSystemError(errno, "%s",
>                               _("Unable to poll on file handles"));
>          return -1;
>      }
> +
> + dispatch:

I don't think we should go through the dispatch code when
we get EBADF, as the contents of the 'revents' fields are
undefined after we get an error.

We should jump straight to the end where we have
"eventLoop.running = 0;"

If any FDs were in fact ready, we'll get them on the next iteration of
the loop anyway.

>      EVENT_DEBUG("Poll got %d event(s)", ret);
>  
>      virMutexLock(&eventLoop.lock);


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: eventpoll: Survive EBADF on macOS
Posted by Roman Bolshakov 5 years, 7 months ago
On Wed, Aug 22, 2018 at 10:41:18AM +0100, Daniel P. Berrangé wrote:
> I don't think we should go through the dispatch code when
> we get EBADF, as the contents of the 'revents' fields are
> undefined after we get an error.
> 
> We should jump straight to the end where we have
> "eventLoop.running = 0;"
> 
> If any FDs were in fact ready, we'll get them on the next iteration of
> the loop anyway.
> 

Alright, but the thread won't hold evenLoop.lock after such jump and
virMutexUnlock may fail with ENOPERM. I suppose that could confuse
valgrind & friends.
What if we change eventLoop.running to 0 under the eventLoop.lock right in
the macOS EBADF handler and then return?

--
Roman

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: eventpoll: Survive EBADF on macOS
Posted by Daniel P. Berrangé 5 years, 7 months ago
On Thu, Aug 23, 2018 at 01:54:46AM +0300, Roman Bolshakov wrote:
> On Wed, Aug 22, 2018 at 10:41:18AM +0100, Daniel P. Berrangé wrote:
> > I don't think we should go through the dispatch code when
> > we get EBADF, as the contents of the 'revents' fields are
> > undefined after we get an error.
> > 
> > We should jump straight to the end where we have
> > "eventLoop.running = 0;"
> > 
> > If any FDs were in fact ready, we'll get them on the next iteration of
> > the loop anyway.
> > 
> 
> Alright, but the thread won't hold evenLoop.lock after such jump and
> virMutexUnlock may fail with ENOPERM. I suppose that could confuse
> valgrind & friends.
> What if we change eventLoop.running to 0 under the eventLoop.lock right in
> the macOS EBADF handler and then return?

Nah, just call virMutexLock before doing the jump.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list