[libvirt] [PATCH v2] 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/20180823084932.37416-1-r.bolshakov@yadro.com
Test syntax-check passed
src/util/vireventpoll.c | 7 +++++++
1 file changed, 7 insertions(+)
[libvirt] [PATCH v2] 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>
---

Changes since v1:
* Don't go through dispatch code on EBADF

 src/util/vireventpoll.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/util/vireventpoll.c b/src/util/vireventpoll.c
index 13d278df13..7d0ffd4113 100644
--- a/src/util/vireventpoll.c
+++ b/src/util/vireventpoll.c
@@ -643,6 +643,12 @@ int virEventPollRunOnce(void)
         EVENT_DEBUG("Poll got error event %d", errno);
         if (errno == EINTR || errno == EAGAIN)
             goto retry;
+#ifdef __APPLE__
+        if (errno == EBADF) {
+            virMutexLock(&eventLoop.lock);
+            goto stop;
+        }
+#endif
         virReportSystemError(errno, "%s",
                              _("Unable to poll on file handles"));
         return -1;
@@ -660,6 +666,7 @@ int virEventPollRunOnce(void)
     virEventPollCleanupTimeouts();
     virEventPollCleanupHandles();
 
+ stop:
     eventLoop.running = 0;
     virMutexUnlock(&eventLoop.lock);
     return 0;
-- 
2.17.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] util: eventpoll: Survive EBADF on macOS
Posted by Michal Prívozník 5 years, 7 months ago
On 08/23/2018 10:49 AM, 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
> 
> 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>
> ---
> 
> Changes since v1:
> * Don't go through dispatch code on EBADF
> 
>  src/util/vireventpoll.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/util/vireventpoll.c b/src/util/vireventpoll.c
> index 13d278df13..7d0ffd4113 100644
> --- a/src/util/vireventpoll.c
> +++ b/src/util/vireventpoll.c
> @@ -643,6 +643,12 @@ int virEventPollRunOnce(void)
>          EVENT_DEBUG("Poll got error event %d", errno);
>          if (errno == EINTR || errno == EAGAIN)
>              goto retry;
> +#ifdef __APPLE__
> +        if (errno == EBADF) {
> +            virMutexLock(&eventLoop.lock);
> +            goto stop;
> +        }
> +#endif
>          virReportSystemError(errno, "%s",
>                               _("Unable to poll on file handles"));
>          return -1;
> @@ -660,6 +666,7 @@ int virEventPollRunOnce(void)
>      virEventPollCleanupTimeouts();
>      virEventPollCleanupHandles();
>  
> + stop:

This will cause a warning about unused label on everything else than
__APPLE__. Also, I think it should be named cleanup to match pattern
from other functions of ours.

>      eventLoop.running = 0;
>      virMutexUnlock(&eventLoop.lock);
>      return 0;
> 

Fixed, ACKed and pushed.

Congratulations on your first libvirt contribution!

Michal

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