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