Hi,
As a general remark: could you try to stick to the 80 char line length
> limit?
>
Ok.
> > +#ifdef _WIN32
> > + }
> > #endif
>
> As Simon mentioned too, this code is getting more and more hard to read.
> Can we maybe do this in some cleaner way? Maybe add some helper
> function(s)?
>
Already fixed in v3.
> +#ifdef _WIN32
> > + }
> > +#endif
>
> As above :)
>
Already fixed in v3.
> > + if (tt->send_ring->head == tt->send_ring->tail)
> > + {
> > + /* nothing to read from tun -> remove tun read flag set
> by IOW_READ */
> > + flags &= ~IOW_READ_TUN;
> > + }
> > + }
> > + }
> > +#endif
>
> Looks like this should be a helper function (with a good name that helps
> understand what this does).
>
Implemented helper function tuntap_ring_empty().
> + c->c2.event_set_status = ret;
>
> This chunk is the same as the "fast path" check above. Looks like you
> should re-arrange this code so you don't have to duplicate it.
>
Not quite (already discussed verbally).
> +#ifdef _WIN32
> > + if (c->c1.tuntap && c->c1.tuntap->wintun)
> > + {
> > + if (c->c1.tuntap->send_ring->head !=
> c->c1.tuntap->send_ring->tail)
>
> Looking this deep into the tuntap struct from another module looks like
> a layering violation. Consider adding a helper function like
>
Fixed with the helper from above (tuntap_ring_empty).
> + {
> > + /* add ring buffer event */
> > + struct rw_handle rw = { .read =
> c->c1.tuntap->send_tail_moved };
> > + event_ctl(mtcp->es, &rw, EVENT_READ, MTCP_TUN);
> > + }
>
> Can this not be done by tun_set() below? At first glance, if either
> tun_event_handle() returns tt->send_tail_moved for wintun (instaed of
> tt->rw_handle), of if tt->rw_handle would be set to send_tail_moved for
> wintun. The latter might even allow you to get rid of
> tt->send_tail_removed completely.
>
Thanks for the idea.
Indeed, I did some refactoring and reused rw_handle.read/write
instead of send/receive_tail_moved. Also got rid of wintun-specific
event_ctl calls - now it is handled in tun_set().
> > + if (tt->send_ring->head == tt->send_ring->tail)
>
> This should use the same helper function as suggested above.
>
Done.
> + struct tun_ring *send_ring;
> > + struct tun_ring *receive_ring;
> > + HANDLE send_tail_moved;
> > + HANDLE receive_tail_moved;
>
> These are wintun-specific, right? Should we maybe clarify this by
> prefixing them with wintun_ ?
>
*_tail_moved are gone, added "wintun_" prefix to *_ring.
> > +#define WINTUN_RING_CAPACITY 0x800000
>
> Can you explain why this is the right size? Document either in a
> comment, or in the commit message.
>
Those are from WG client, mentioned it in a comment.
-Steffan
V4 is coming!
--
-Lev
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
