Arm inotify only in server mode
PATCH 1/1 Arm inotify only in server mode
Async-push is a server side feature and inotify_fd is initialized in server mode.
Trac #786
--- src/openvpn/forward.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
src/openvpn/forward.c +2 -1
@@ -1578,7 +1578,8 @@ io_wait_dowork (struct context *c, const unsigned int flags)#ifdef ENABLE_ASYNC_PUSH /* arm inotify watcher */ -event_ctl (c->c2.event_set, c->c2.inotify_fd,EVENT_READ , (void *)&file_shift); +if (c->options.mode ==MODE_SERVER ) +event_ctl (c->c2.event_set, c->c2.inotify_fd,EVENT_READ , (void *)&file_shift);#endif /*
Signature · 1 line
1.9.1
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
ACK. Also tested using the reproducer in Trac. This patch fixes the issue when OpenVPN is built using --enable-async-push.
Your patch has been applied to the master branch.
commit 7084a3993fa35c6fb71abe8aac7b30f442205e2a
Author: Lev Stipakov Date: Wed Dec 7 01:45:51 2016 +0200
Arm inotify only in server mode
Trac: #786
Signed-off-by: Lev Stipakov <[email protected]...>
Acked-by: David Sommerseth <[email protected]...>
Message-Id: <[email protected]...>
URL: https://www.mail-archive.com/[email protected]/msg13415.html
Signed-off-by: David Sommerseth <[email protected]...>- --
kind regards,
David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
iQIcBAEBAgAGBQJYR342AAoJEIbPlEyWcf3ysysP/jfNdapMwuRJ8hhqJFMOvMzg ss+EOhWpZHzC7RJfBUeYWVdXMMGL+I8beKGZTHoC1DEPkVOV4wg2FHB0wpvIQMja BrEo/AiCBUrMC5eiIn8IO6b6ATKQih/oD07GcECcSZ0/Vica3goP1hVik7JUkAZA 2fsDjGh+5g4KfObHMaTjpID9UfQV7wsPU/C2pWc0ywq51CZXingWQnWwaYKS93OZ i7EKxQrWy0Nxs0YhzloVI/2k6mcwwP0mzeZdQ+imhjtuXoYmSisXxUSNDrwvgYUN byMb4qhcuuDRquR3mRO0jjVR7nlL68NDlX57Z60ocbxskZBzhfxMM1/CkXgVLC5L TBU6IHaTWn/5/d4jo4aA50sDBQohu73Yf4/SnqwcWURsui4yJCejVQTFeKyKPxiD CQ1T0n9swXRsgYqsKioZHv3Mywaciq5y3+3BpJ9oGg9mfYcoQxQ5kJNY6jjIYC3C xQiFFP92bqi7d761bKhwsJU//DZ4FRwqWihhkUL9VJ4WhZmVdrJjYEcyknnRPm1F /Woc9V2sBhIZ00VCfvu0WNvLb1w40q6PV695uqfOQzjhPuhXp3q0ZEOp76ArqZqN OW6DjKodACCZKkSP2Ef1/9jsmXmL/46348FVBSa4Qbr7PNRlHe1LjTYQSg/aS1y4 T33QanBd0sAGstvfuf2C
=Lcf0
-----END PGP SIGNATURE-----
Hi,
src/openvpn/forward.c io_wait_dowork (struct context *c, const unsigned int flags) · line 1583
Quoted 6 lines · Lev Stipakov
#ifdef ENABLE_ASYNC_PUSH /* arm inotify watcher */src/openvpn/forward.c +2
-1-event_ctl (c->c2.event_set, c->c2.inotify_fd,EVENT_READ , (void *)&file_shift); +if (c->options.mode ==MODE_SERVER ) +event_ctl (c->c2.event_set, c->c2.inotify_fd,EVENT_READ , (void *)&file_shift);#endif
I find this a non-robust approach to this.
What is wrong with
if ( c->c2.inotify_fd > 0 )
{
event_ctl (c->c2.event_set, c->c2.inotify_fd, EVENT_READ, (void*)&file_shift);
}(as I suggested on IRC yesterday evening)?
This will ensure that inotify is armed *if* it got prepped, independent of "whatever other flags are active in the system" - next thing, someone adds a "--no-inotify" run-time switch, and you'll have to revisit this place again - or someone else finds a use for inotify in client mode, and this will also need touching.
It will fix the issue at hand, but is not good code.
Also, braces. See CodingStyle in the wiki.
gert
Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany [email protected]...
fax: +49-89-35655025 [email protected]...Quoted 12 lines · Gert Doering
Hi,
src/openvpn/forward.c io_wait_dowork (struct context *c, const unsigned int flags) · line 1583
Quoted 6 lines · Lev Stipakov
#ifdef ENABLE_ASYNC_PUSH /* arm inotify watcher */src/openvpn/forward.c +2
-1-event_ctl (c->c2.event_set, c->c2.inotify_fd,EVENT_READ , (void *)&file_shift); +if (c->options.mode ==MODE_SERVER ) +event_ctl (c->c2.event_set, c->c2.inotify_fd,EVENT_READ , (void *)&file_shift);#endif I find this a non-robust approach to this.
Quoted 8 lines
What is wrong with
if ( c->c2.inotify_fd > 0 ) { event_ctl (c->c2.event_set, c->c2.inotify_fd, EVENT_READ, (void*)&file_shift); }(as I suggested on IRC yesterday evening)?
That scenario is already tackled in tunnel_server_tcp() [mtcp.c:708] and tunnel_server_udp_single_threaded() [mudp.c:300], which I would say is the proper place to catch this error.
The race we saw in trac ticket #786 is related to being in non-server mode. So if OpenVPN is not running in server mode, we shouldn't touch c->c2.inotify_fd at all.
Quoted 5 lines
This will ensure that inotify is armed *if* it got prepped, independent of "whatever other flags are active in the system" - next thing, someone adds a "--no-inotify" run-time switch, and you'll have to revisit this place again - or someone else finds a use for inotify in client mode, and this will also need touching.
If such a --no-inotify option is added, it would need to be handled in the tunnel_server_*() functions anyhow. And to implement such a feature without checking all the ENABLE_ASYNC_PUSH #ifdefs would be a poor implementation.
Quoted 1 line
Also, braces. See CodingStyle in the wiki.
Yes, that is right once we've formatted the whole code tree to be consistent. Currently this kind of missing braces is just as common. Which is why I didn't really react to this one in this round.
Signature · 4 lines
kind regards, David Sommerseth OpenVPN Technologies, Inc