Re: [Openvpn-devel] [PATCH] Arm inotify only in server mode
On 07/12/16 09:19, Gert Doering wrote:
> Hi,
>
> On Wed, Dec 07, 2016 at 01:45:51AM +0200, Lev Stipakov wrote:
>> #ifdef ENABLE_ASYNC_PUSH
>>/* arm inotify watcher */
>> - event_ctl (c->c2.event_set, c->c2.inotify_fd, EVENT_READ,
>> (void*)_shift);
>> + if (c->options.mode == MODE_SERVER)
>> +event_ctl (c->c2.event_set, c->c2.inotify_fd, EVENT_READ,
>> (void*)_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*)_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.
> 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.
> 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.
--
kind regards,
David Sommerseth
OpenVPN Technologies, Inc
signature.asc
Description: OpenPGP digital signature
--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today.http://sdm.link/xeonphi___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Arm inotify only in server mode
Hi,
On Wed, Dec 07, 2016 at 01:45:51AM +0200, Lev Stipakov wrote:
> #ifdef ENABLE_ASYNC_PUSH
>/* arm inotify watcher */
> - event_ctl (c->c2.event_set, c->c2.inotify_fd, EVENT_READ,
> (void*)_shift);
> + if (c->options.mode == MODE_SERVER)
> +event_ctl (c->c2.event_set, c->c2.inotify_fd, EVENT_READ,
> (void*)_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*)_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
--
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de
signature.asc
Description: PGP signature
--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today.http://sdm.link/xeonphi___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] 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(-) diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index b50a2e0..4502e10 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -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*)_shift); + if (c->options.mode == MODE_SERVER) +event_ctl (c->c2.event_set, c->c2.inotify_fd, EVENT_READ, (void*)_shift); #endif /* -- 1.9.1 -- Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today.http://sdm.link/xeonphi ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
