| Message ID | 20220117094917.178-1-lstipakov@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v3] tun: remove tun_finalize() | expand |
Hi, On 17/01/2022 10:49, Lev Stipakov wrote: [cut] > - > -static inline int > -read_tun_buffered(struct tuntap *tt, struct buffer *buf) > -{ > - return tun_finalize(tt->hand, &tt->reads, buf); > -} > +int > +tun_write_win32(struct tuntap *tt, struct buffer *buf); for prototypes we put the return type on the same line as the function name (unless we have line length issues, but not in this case). Cheers,
This is probably something committer (looks at cron2) could fix on the fly, unless there are more issues which would require v4? ma 17. tammik. 2022 klo 11.53 Antonio Quartulli (a@unstable.cc) kirjoitti: > > Hi, > > On 17/01/2022 10:49, Lev Stipakov wrote: > [cut] > > - > > -static inline int > > -read_tun_buffered(struct tuntap *tt, struct buffer *buf) > > -{ > > - return tun_finalize(tt->hand, &tt->reads, buf); > > -} > > +int > > +tun_write_win32(struct tuntap *tt, struct buffer *buf); > > for prototypes we put the return type on the same line as the function > name (unless we have line length issues, but not in this case). > > Cheers, > > > -- > Antonio Quartulli
Hi, On Mon, Jan 17, 2022 at 11:56:51AM +0200, Lev Stipakov wrote: > This is probably something committer (looks at cron2) could fix on the > fly, unless there are more issues which would require v4? If that's the only thing, I can fix that on the fly. gert
Gentle nudge. ma 17. tammik. 2022 klo 13.42 Gert Doering (gert@greenie.muc.de) kirjoitti: > > Hi, > > On Mon, Jan 17, 2022 at 11:56:51AM +0200, Lev Stipakov wrote: > > This is probably something committer (looks at cron2) could fix on the > > fly, unless there are more issues which would require v4? > > If that's the only thing, I can fix that on the fly. > > gert > -- > "If was one thing all people took for granted, was conviction that if you > feed honest figures into a computer, honest figures come out. Never doubted > it myself till I met a computer with a sense of humor." > Robert A. Heinlein, The Moon is a Harsh Mistress > > Gert Doering - Munich, Germany gert@greenie.muc.de
Hi, On Mon, Jan 17, 2022 at 4:51 AM Lev Stipakov <lstipakov@gmail.com> wrote: > From: Lev Stipakov <lev@openvpn.net> > > tun_finalize() is essentially subset of socket_finalize() apart from: > > - using WSAFoo() functions instead of Foo() > > - "from" address is not returned > > There is no clear official statement that one can use non-WSA > API on handles, so let's be on a safe side and use both. > > Introduce sockethandle_t abstraction, which represents > socket and handle. Add SocketHandle* routines which call > proper API depends on underlying type in abstraction. > > Rename socket_finalize() to sockethandle_finalize(), take > sockethandle_t and new routines into use and kick tun_finalize(). > > Signed-off-by: Lev Stipakov <lev@openvpn.net> > --- > v3: replace macros with inline functions > > v2: explicitly initialize .is_handle to false for readablity > > > src/openvpn/forward.c | 3 +- > src/openvpn/socket.c | 37 ++++++++--------- > src/openvpn/socket.h | 49 ++++++++++++++++++---- > src/openvpn/tun.c | 94 +++++++++---------------------------------- > src/openvpn/tun.h | 34 +--------------- > 5 files changed, 80 insertions(+), 137 deletions(-) > > diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c > index f82386a1..a905f993 100644 > --- a/src/openvpn/forward.c > +++ b/src/openvpn/forward.c > @@ -1115,7 +1115,8 @@ read_incoming_tun(struct context *c) > } > else > { > - read_tun_buffered(c->c1.tuntap, &c->c2.buf); > + sockethandle_t sh = { .is_handle = true, .h = c->c1.tuntap->hand > }; > + sockethandle_finalize(sh, &c->c1.tuntap->reads, &c->c2.buf, NULL); > } > #else /* ifdef _WIN32 */ > ASSERT(buf_init(&c->c2.buf, FRAME_HEADROOM(&c->c2.frame))); > diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c > index df736746..780c5cb3 100644 > --- a/src/openvpn/socket.c > +++ b/src/openvpn/socket.c > @@ -3198,7 +3198,8 @@ link_socket_read_tcp(struct link_socket *sock, > if (!sock->stream_buf.residual_fully_formed) > { > #ifdef _WIN32 > - len = socket_finalize(sock->sd, &sock->reads, buf, NULL); > + sockethandle_t sh = { .s = sock->sd }; > + len = sockethandle_finalize(sh, &sock->reads, buf, NULL); > #else > struct buffer frag; > stream_buf_get_next(&sock->stream_buf, &frag); > @@ -3664,10 +3665,10 @@ socket_send_queue(struct link_socket *sock, struct > buffer *buf, const struct lin > } > > int > -socket_finalize(SOCKET s, > - struct overlapped_io *io, > - struct buffer *buf, > - struct link_socket_actual *from) > +sockethandle_finalize(sockethandle_t sh, > + struct overlapped_io *io, > + struct buffer *buf, > + struct link_socket_actual *from) > { > int ret = -1; > BOOL status; > @@ -3675,13 +3676,7 @@ socket_finalize(SOCKET s, > switch (io->iostate) > { > case IOSTATE_QUEUED: > - status = WSAGetOverlappedResult( > - s, > - &io->overlapped, > - &io->size, > - FALSE, > - &io->flags > - ); > + status = SocketHandleGetOverlappedResult(sh, io); > if (status) > { > /* successful return for a queued operation */ > @@ -3693,18 +3688,18 @@ socket_finalize(SOCKET s, > io->iostate = IOSTATE_INITIAL; > ASSERT(ResetEvent(io->overlapped.hEvent)); > > - dmsg(D_WIN32_IO, "WIN32 I/O: Socket Completion success > [%d]", ret); > + dmsg(D_WIN32_IO, "WIN32 I/O: Completion success [%d]", > ret); > } > else > { > /* error during a queued operation */ > ret = -1; > - if (WSAGetLastError() != WSA_IO_INCOMPLETE) > + if (SocketHandleGetLastError(sh) != ERROR_IO_INCOMPLETE) > WSA_IO_INCOMPLETE is the same as ERROR_IO_INCOMPLETE, so using them interchangeably looks ok. { > /* if no error (i.e. just not finished yet), then > DON'T execute this code */ > io->iostate = IOSTATE_INITIAL; > ASSERT(ResetEvent(io->overlapped.hEvent)); > - msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: Socket > Completion error"); > + msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: Completion > error"); > } > } > break; > @@ -3715,9 +3710,9 @@ socket_finalize(SOCKET s, > if (io->status) > { > /* error return for a non-queued operation */ > - WSASetLastError(io->status); > + SocketHandleSetLastError(sh, io->status); > ret = -1; > - msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: Socket Completion > non-queued error"); > + msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: Completion > non-queued error"); > } > else > { > @@ -3727,14 +3722,14 @@ socket_finalize(SOCKET s, > *buf = io->buf; > } > ret = io->size; > - dmsg(D_WIN32_IO, "WIN32 I/O: Socket Completion non-queued > success [%d]", ret); > + dmsg(D_WIN32_IO, "WIN32 I/O: Completion non-queued > success [%d]", ret); > } > break; > > case IOSTATE_INITIAL: /* were we called without proper queueing? > */ > - WSASetLastError(WSAEINVAL); > + SocketHandleSetInvalError(sh); > ret = -1; > - dmsg(D_WIN32_IO, "WIN32 I/O: Socket Completion BAD STATE"); > + dmsg(D_WIN32_IO, "WIN32 I/O: Completion BAD STATE"); > break; > > default: > @@ -3742,7 +3737,7 @@ socket_finalize(SOCKET s, > } > > /* return from address if requested */ > - if (from) > + if (!sh.is_handle && from) > { > if (ret >= 0 && io->addr_defined) > { > diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h > index cc1e0c36..77c3d720 100644 > --- a/src/openvpn/socket.h > +++ b/src/openvpn/socket.h > @@ -262,11 +262,44 @@ int socket_send_queue(struct link_socket *sock, > struct buffer *buf, > const struct link_socket_actual *to); > > -int socket_finalize( > - SOCKET s, > - struct overlapped_io *io, > - struct buffer *buf, > - struct link_socket_actual *from); > +typedef struct { > + union { > + SOCKET s; > + HANDLE h; > + }; > + bool is_handle; > +} sockethandle_t; > + > +int sockethandle_finalize(sockethandle_t sh, > + struct overlapped_io *io, > + struct buffer *buf, > + struct link_socket_actual *from); > + > +static inline BOOL > +SocketHandleGetOverlappedResult(sockethandle_t sh, struct overlapped_io > *io) > +{ > + return sh.is_handle ? > + GetOverlappedResult(sh.h, &io->overlapped, &io->size, FALSE) : > + WSAGetOverlappedResult(sh.s, &io->overlapped, &io->size, FALSE, > &io->flags); > +} > + > +static inline int > +SocketHandleGetLastError(sockethandle_t sh) > +{ > + return sh.is_handle ? (int)GetLastError() : WSAGetLastError(); > +} > + > +inline static void > +SocketHandleSetLastError(sockethandle_t sh, DWORD err) > +{ > + sh.is_handle ? SetLastError(err) : WSASetLastError(err); > To be consistent, one could add an explicit cast here too and avoid a potential compiler warning: WSASetLastError((int) err); > +} > + > +static inline void > +SocketHandleSetInvalError(sockethandle_t sh) > +{ > + sh.is_handle ? SetLastError(ERROR_INVALID_FUNCTION) : > WSASetLastError(WSAEINVAL); > +} > I'm not sure whether ERROR_INVALID_FUNCTION is the appropriate error code here. WSAEINVAL is more like invalid-argument (so ERROR_BAD_ARGUMENTS?). But this agrees with the original, so let it be. > > #else /* ifdef _WIN32 */ > > @@ -1020,7 +1053,8 @@ link_socket_read_udp_win32(struct link_socket *sock, > struct buffer *buf, > struct link_socket_actual *from) > { > - return socket_finalize(sock->sd, &sock->reads, buf, from); > + sockethandle_t sh = { .s = sock->sd }; > + return sockethandle_finalize(sh, &sock->reads, buf, from); > } > > #else /* ifdef _WIN32 */ > @@ -1078,9 +1112,10 @@ link_socket_write_win32(struct link_socket *sock, > { > int err = 0; > int status = 0; > + sockethandle_t sh = { .s = sock->sd }; > if (overlapped_io_active(&sock->writes)) > { > - status = socket_finalize(sock->sd, &sock->writes, NULL, NULL); > + status = sockethandle_finalize(sh, &sock->writes, NULL, NULL); > if (status < 0) > { > err = WSAGetLastError(); > diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c > index 12bdd200..fe32127b 100644 > --- a/src/openvpn/tun.c > +++ b/src/openvpn/tun.c > @@ -3561,87 +3561,29 @@ tun_write_queue(struct tuntap *tt, struct buffer > *buf) > } > > int > -tun_finalize( > - HANDLE h, > - struct overlapped_io *io, > - struct buffer *buf) > +tun_write_win32(struct tuntap *tt, struct buffer *buf) > { > - int ret = -1; > - BOOL status; > - > - switch (io->iostate) > + int err = 0; > + int status = 0; > + if (overlapped_io_active(&tt->writes)) > { > - case IOSTATE_QUEUED: > - status = GetOverlappedResult( > - h, > - &io->overlapped, > - &io->size, > - FALSE > - ); > - if (status) > - { > - /* successful return for a queued operation */ > - if (buf) > - { > - *buf = io->buf; > - } > - ret = io->size; > - io->iostate = IOSTATE_INITIAL; > - ASSERT(ResetEvent(io->overlapped.hEvent)); > - dmsg(D_WIN32_IO, "WIN32 I/O: TAP Completion success > [%d]", ret); > - } > - else > - { > - /* error during a queued operation */ > - ret = -1; > - if (GetLastError() != ERROR_IO_INCOMPLETE) > - { > - /* if no error (i.e. just not finished yet), > - * then DON'T execute this code */ > - io->iostate = IOSTATE_INITIAL; > - ASSERT(ResetEvent(io->overlapped.hEvent)); > - msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: TAP Completion > error"); > - } > - } > - break; > - > - case IOSTATE_IMMEDIATE_RETURN: > - io->iostate = IOSTATE_INITIAL; > - ASSERT(ResetEvent(io->overlapped.hEvent)); > - if (io->status) > - { > - /* error return for a non-queued operation */ > - SetLastError(io->status); > - ret = -1; > - msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: TAP Completion > non-queued error"); > - } > - else > - { > - /* successful return for a non-queued operation */ > - if (buf) > - { > - *buf = io->buf; > - } > - ret = io->size; > - dmsg(D_WIN32_IO, "WIN32 I/O: TAP Completion non-queued > success [%d]", ret); > - } > - break; > - > - case IOSTATE_INITIAL: /* were we called without proper queueing? > */ > - SetLastError(ERROR_INVALID_FUNCTION); > - ret = -1; > - dmsg(D_WIN32_IO, "WIN32 I/O: TAP Completion BAD STATE"); > - break; > - > - default: > - ASSERT(0); > + sockethandle_t sh = { .is_handle = true, .h = tt->hand }; > + status = sockethandle_finalize(sh, &tt->writes, NULL, NULL); > + if (status < 0) > + { > + err = GetLastError(); > + } > } > - > - if (buf) > + tun_write_queue(tt, buf); > + if (status < 0) > { > - buf->len = ret; > + SetLastError(err); > + return status; > + } > + else > + { > + return BLEN(buf); > } > - return ret; > } > > static const struct device_instance_id_interface * > diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h > index d4657537..a6661be0 100644 > --- a/src/openvpn/tun.h > +++ b/src/openvpn/tun.h > @@ -437,8 +437,6 @@ int tun_read_queue(struct tuntap *tt, int maxsize); > > int tun_write_queue(struct tuntap *tt, struct buffer *buf); > > -int tun_finalize(HANDLE h, struct overlapped_io *io, struct buffer *buf); > - > static inline bool > tuntap_stop(int status) > { > @@ -466,36 +464,8 @@ tuntap_abort(int status) > return false; > } > > -static inline int > -tun_write_win32(struct tuntap *tt, struct buffer *buf) > -{ > - int err = 0; > - int status = 0; > - if (overlapped_io_active(&tt->writes)) > - { > - status = tun_finalize(tt->hand, &tt->writes, NULL); > - if (status < 0) > - { > - err = GetLastError(); > - } > - } > - tun_write_queue(tt, buf); > - if (status < 0) > - { > - SetLastError(err); > - return status; > - } > - else > - { > - return BLEN(buf); > - } > -} > - > -static inline int > -read_tun_buffered(struct tuntap *tt, struct buffer *buf) > -{ > - return tun_finalize(tt->hand, &tt->reads, buf); > -} > +int > +tun_write_win32(struct tuntap *tt, struct buffer *buf); > Antonio wanted this to be in one line though we are not terribly consistent about this. > static inline ULONG > wintun_ring_packet_align(ULONG size) > In spite of those nits meant to annoy the author, I think this looks good. Acked-by: Selva Nair <selva.nair@gmail.com> <div dir="ltr"><div>Hi,</div><div><br></div><div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jan 17, 2022 at 4:51 AM Lev Stipakov <<a href="mailto:lstipakov@gmail.com" target="_blank">lstipakov@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">From: Lev Stipakov <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><br> <br> tun_finalize() is essentially subset of socket_finalize() apart from:<br> <br> - using WSAFoo() functions instead of Foo()<br> <br> - "from" address is not returned<br> <br> There is no clear official statement that one can use non-WSA<br> API on handles, so let's be on a safe side and use both.<br> <br> Introduce sockethandle_t abstraction, which represents<br> socket and handle. Add SocketHandle* routines which call<br> proper API depends on underlying type in abstraction.<br> <br> Rename socket_finalize() to sockethandle_finalize(), take<br> sockethandle_t and new routines into use and kick tun_finalize().<br> <br> Signed-off-by: Lev Stipakov <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><br> ---<br> v3: replace macros with inline functions<br> <br> v2: explicitly initialize .is_handle to false for readablity <br> <br> <br> src/openvpn/forward.c | 3 +-<br> src/openvpn/socket.c | 37 ++++++++---------<br> src/openvpn/socket.h | 49 ++++++++++++++++++----<br> src/openvpn/tun.c | 94 +++++++++----------------------------------<br> src/openvpn/tun.h | 34 +---------------<br> 5 files changed, 80 insertions(+), 137 deletions(-)<br> <br> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c<br> index f82386a1..a905f993 100644<br> --- a/src/openvpn/forward.c<br> +++ b/src/openvpn/forward.c<br> @@ -1115,7 +1115,8 @@ read_incoming_tun(struct context *c)<br> }<br> else<br> {<br> - read_tun_buffered(c->c1.tuntap, &c->c2.buf);<br> + sockethandle_t sh = { .is_handle = true, .h = c->c1.tuntap->hand };<br> + sockethandle_finalize(sh, &c->c1.tuntap->reads, &c->c2.buf, NULL);<br> }<br> #else /* ifdef _WIN32 */<br> ASSERT(buf_init(&c->c2.buf, FRAME_HEADROOM(&c->c2.frame)));<br> diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c<br> index df736746..780c5cb3 100644<br> --- a/src/openvpn/socket.c<br> +++ b/src/openvpn/socket.c<br> @@ -3198,7 +3198,8 @@ link_socket_read_tcp(struct link_socket *sock,<br> if (!sock->stream_buf.residual_fully_formed)<br> {<br> #ifdef _WIN32<br> - len = socket_finalize(sock->sd, &sock->reads, buf, NULL);<br> + sockethandle_t sh = { .s = sock->sd };<br> + len = sockethandle_finalize(sh, &sock->reads, buf, NULL);<br> #else<br> struct buffer frag;<br> stream_buf_get_next(&sock->stream_buf, &frag);<br> @@ -3664,10 +3665,10 @@ socket_send_queue(struct link_socket *sock, struct buffer *buf, const struct lin<br> }<br> <br> int<br> -socket_finalize(SOCKET s,<br> - struct overlapped_io *io,<br> - struct buffer *buf,<br> - struct link_socket_actual *from)<br> +sockethandle_finalize(sockethandle_t sh,<br> + struct overlapped_io *io,<br> + struct buffer *buf,<br> + struct link_socket_actual *from)<br> {<br> int ret = -1;<br> BOOL status;<br> @@ -3675,13 +3676,7 @@ socket_finalize(SOCKET s,<br> switch (io->iostate)<br> {<br> case IOSTATE_QUEUED:<br> - status = WSAGetOverlappedResult(<br> - s,<br> - &io->overlapped,<br> - &io->size,<br> - FALSE,<br> - &io->flags<br> - );<br> + status = SocketHandleGetOverlappedResult(sh, io);<br> if (status)<br> {<br> /* successful return for a queued operation */<br> @@ -3693,18 +3688,18 @@ socket_finalize(SOCKET s,<br> io->iostate = IOSTATE_INITIAL;<br> ASSERT(ResetEvent(io->overlapped.hEvent));<br> <br> - dmsg(D_WIN32_IO, "WIN32 I/O: Socket Completion success [%d]", ret);<br> + dmsg(D_WIN32_IO, "WIN32 I/O: Completion success [%d]", ret);<br> }<br> else<br> {<br> /* error during a queued operation */<br> ret = -1;<br> - if (WSAGetLastError() != WSA_IO_INCOMPLETE)<br> + if (SocketHandleGetLastError(sh) != ERROR_IO_INCOMPLETE)<br></blockquote><div><br></div><div>WSA_IO_INCOMPLETE is the same as ERROR_IO_INCOMPLETE, so using them</div><div>interchangeably looks ok. <br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> {<br> /* if no error (i.e. just not finished yet), then DON'T execute this code */<br> io->iostate = IOSTATE_INITIAL;<br> ASSERT(ResetEvent(io->overlapped.hEvent));<br> - msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: Socket Completion error");<br> + msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: Completion error");<br> }<br> }<br> break;<br> @@ -3715,9 +3710,9 @@ socket_finalize(SOCKET s,<br> if (io->status)<br> {<br> /* error return for a non-queued operation */<br> - WSASetLastError(io->status);<br> + SocketHandleSetLastError(sh, io->status);<br> ret = -1;<br> - msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: Socket Completion non-queued error");<br> + msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: Completion non-queued error");<br> }<br> else<br> {<br> @@ -3727,14 +3722,14 @@ socket_finalize(SOCKET s,<br> *buf = io->buf;<br> }<br> ret = io->size;<br> - dmsg(D_WIN32_IO, "WIN32 I/O: Socket Completion non-queued success [%d]", ret);<br> + dmsg(D_WIN32_IO, "WIN32 I/O: Completion non-queued success [%d]", ret);<br> }<br> break;<br> <br> case IOSTATE_INITIAL: /* were we called without proper queueing? */<br> - WSASetLastError(WSAEINVAL);<br> + SocketHandleSetInvalError(sh);<br> ret = -1;<br> - dmsg(D_WIN32_IO, "WIN32 I/O: Socket Completion BAD STATE");<br> + dmsg(D_WIN32_IO, "WIN32 I/O: Completion BAD STATE");<br> break;<br> <br> default:<br> @@ -3742,7 +3737,7 @@ socket_finalize(SOCKET s,<br> }<br> <br> /* return from address if requested */<br> - if (from)<br> + if (!sh.is_handle && from)<br> {<br> if (ret >= 0 && io->addr_defined)<br> {<br> diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h<br> index cc1e0c36..77c3d720 100644<br> --- a/src/openvpn/socket.h<br> +++ b/src/openvpn/socket.h<br> @@ -262,11 +262,44 @@ int socket_send_queue(struct link_socket *sock,<br> struct buffer *buf,<br> const struct link_socket_actual *to);<br> <br> -int socket_finalize(<br> - SOCKET s,<br> - struct overlapped_io *io,<br> - struct buffer *buf,<br> - struct link_socket_actual *from);<br> +typedef struct {<br> + union {<br> + SOCKET s;<br> + HANDLE h;<br> + };<br> + bool is_handle;<br> +} sockethandle_t;<br> +<br> +int sockethandle_finalize(sockethandle_t sh,<br> + struct overlapped_io *io,<br> + struct buffer *buf,<br> + struct link_socket_actual *from);<br> +<br> +static inline BOOL<br> +SocketHandleGetOverlappedResult(sockethandle_t sh, struct overlapped_io *io)<br> +{<br> + return sh.is_handle ?<br> + GetOverlappedResult(sh.h, &io->overlapped, &io->size, FALSE) :<br> + WSAGetOverlappedResult(sh.s, &io->overlapped, &io->size, FALSE, &io->flags);<br> +}<br> +<br> +static inline int<br> +SocketHandleGetLastError(sockethandle_t sh)<br> +{<br> + return sh.is_handle ? (int)GetLastError() : WSAGetLastError();<br> +}<br> +<br> +inline static void<br> +SocketHandleSetLastError(sockethandle_t sh, DWORD err)<br> +{<br> + sh.is_handle ? SetLastError(err) : WSASetLastError(err);<br></blockquote><div><br></div><div>To be consistent, one could add an explicit cast here too and avoid a potential compiler warning:</div><div><br></div><div>WSASetLastError((int) err); </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> +}<br> +<br> +static inline void<br> +SocketHandleSetInvalError(sockethandle_t sh)<br> +{<br> + sh.is_handle ? SetLastError(ERROR_INVALID_FUNCTION) : WSASetLastError(WSAEINVAL);<br> +}<br></blockquote><div><br></div><div>I'm not sure whether ERROR_INVALID_FUNCTION is the appropriate error code here. WSAEINVAL is more like invalid-argument (so ERROR_BAD_ARGUMENTS?). But this agrees with the original, so let it be.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> #else /* ifdef _WIN32 */<br> <br> @@ -1020,7 +1053,8 @@ link_socket_read_udp_win32(struct link_socket *sock,<br> struct buffer *buf,<br> struct link_socket_actual *from)<br> {<br> - return socket_finalize(sock->sd, &sock->reads, buf, from);<br> + sockethandle_t sh = { .s = sock->sd };<br> + return sockethandle_finalize(sh, &sock->reads, buf, from);<br> }<br> <br> #else /* ifdef _WIN32 */<br> @@ -1078,9 +1112,10 @@ link_socket_write_win32(struct link_socket *sock,<br> {<br> int err = 0;<br> int status = 0;<br> + sockethandle_t sh = { .s = sock->sd };<br> if (overlapped_io_active(&sock->writes))<br> {<br> - status = socket_finalize(sock->sd, &sock->writes, NULL, NULL);<br> + status = sockethandle_finalize(sh, &sock->writes, NULL, NULL);<br> if (status < 0)<br> {<br> err = WSAGetLastError();<br> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c<br> index 12bdd200..fe32127b 100644<br> --- a/src/openvpn/tun.c<br> +++ b/src/openvpn/tun.c<br> @@ -3561,87 +3561,29 @@ tun_write_queue(struct tuntap *tt, struct buffer *buf)<br> }<br> <br> int<br> -tun_finalize(<br> - HANDLE h,<br> - struct overlapped_io *io,<br> - struct buffer *buf)<br> +tun_write_win32(struct tuntap *tt, struct buffer *buf)<br> {<br> - int ret = -1;<br> - BOOL status;<br> -<br> - switch (io->iostate)<br> + int err = 0;<br> + int status = 0;<br> + if (overlapped_io_active(&tt->writes))<br> {<br> - case IOSTATE_QUEUED:<br> - status = GetOverlappedResult(<br> - h,<br> - &io->overlapped,<br> - &io->size,<br> - FALSE<br> - );<br> - if (status)<br> - {<br> - /* successful return for a queued operation */<br> - if (buf)<br> - {<br> - *buf = io->buf;<br> - }<br> - ret = io->size;<br> - io->iostate = IOSTATE_INITIAL;<br> - ASSERT(ResetEvent(io->overlapped.hEvent));<br> - dmsg(D_WIN32_IO, "WIN32 I/O: TAP Completion success [%d]", ret);<br> - }<br> - else<br> - {<br> - /* error during a queued operation */<br> - ret = -1;<br> - if (GetLastError() != ERROR_IO_INCOMPLETE)<br> - {<br> - /* if no error (i.e. just not finished yet),<br> - * then DON'T execute this code */<br> - io->iostate = IOSTATE_INITIAL;<br> - ASSERT(ResetEvent(io->overlapped.hEvent));<br> - msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: TAP Completion error");<br> - }<br> - }<br> - break;<br> -<br> - case IOSTATE_IMMEDIATE_RETURN:<br> - io->iostate = IOSTATE_INITIAL;<br> - ASSERT(ResetEvent(io->overlapped.hEvent));<br> - if (io->status)<br> - {<br> - /* error return for a non-queued operation */<br> - SetLastError(io->status);<br> - ret = -1;<br> - msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: TAP Completion non-queued error");<br> - }<br> - else<br> - {<br> - /* successful return for a non-queued operation */<br> - if (buf)<br> - {<br> - *buf = io->buf;<br> - }<br> - ret = io->size;<br> - dmsg(D_WIN32_IO, "WIN32 I/O: TAP Completion non-queued success [%d]", ret);<br> - }<br> - break;<br> -<br> - case IOSTATE_INITIAL: /* were we called without proper queueing? */<br> - SetLastError(ERROR_INVALID_FUNCTION);<br> - ret = -1;<br> - dmsg(D_WIN32_IO, "WIN32 I/O: TAP Completion BAD STATE");<br> - break;<br> -<br> - default:<br> - ASSERT(0);<br> + sockethandle_t sh = { .is_handle = true, .h = tt->hand };<br> + status = sockethandle_finalize(sh, &tt->writes, NULL, NULL);<br> + if (status < 0)<br> + {<br> + err = GetLastError();<br> + }<br> }<br> -<br> - if (buf)<br> + tun_write_queue(tt, buf);<br> + if (status < 0)<br> {<br> - buf->len = ret;<br> + SetLastError(err);<br> + return status;<br> + }<br> + else<br> + {<br> + return BLEN(buf);<br> }<br> - return ret;<br> }<br> <br> static const struct device_instance_id_interface *<br> diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h<br> index d4657537..a6661be0 100644<br> --- a/src/openvpn/tun.h<br> +++ b/src/openvpn/tun.h<br> @@ -437,8 +437,6 @@ int tun_read_queue(struct tuntap *tt, int maxsize);<br> <br> int tun_write_queue(struct tuntap *tt, struct buffer *buf);<br> <br> -int tun_finalize(HANDLE h, struct overlapped_io *io, struct buffer *buf);<br> -<br> static inline bool<br> tuntap_stop(int status)<br> {<br> @@ -466,36 +464,8 @@ tuntap_abort(int status)<br> return false;<br> }<br> <br> -static inline int<br> -tun_write_win32(struct tuntap *tt, struct buffer *buf)<br> -{<br> - int err = 0;<br> - int status = 0;<br> - if (overlapped_io_active(&tt->writes))<br> - {<br> - status = tun_finalize(tt->hand, &tt->writes, NULL);<br> - if (status < 0)<br> - {<br> - err = GetLastError();<br> - }<br> - }<br> - tun_write_queue(tt, buf);<br> - if (status < 0)<br> - {<br> - SetLastError(err);<br> - return status;<br> - }<br> - else<br> - {<br> - return BLEN(buf);<br> - }<br> -}<br> -<br> -static inline int<br> -read_tun_buffered(struct tuntap *tt, struct buffer *buf)<br> -{<br> - return tun_finalize(tt->hand, &tt->reads, buf);<br> -}<br> +int<br> +tun_write_win32(struct tuntap *tt, struct buffer *buf);<br></blockquote><div><br></div><div>Antonio wanted this to be in one line though we are not terribly consistent about this.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> static inline ULONG<br> wintun_ring_packet_align(ULONG size)<br></blockquote><div><br></div><div>In spite of those nits meant to annoy the author, I think this looks good.</div><div><br></div><div>Acked-by: Selva Nair <<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>></div></div></div></div>
Test compiled with MinGW. Not runtime tested.
Adjusted the prototype to be in-one-line, as instructed. And have
ignored the other nits :-)
Your patch has been applied to the master branch.
commit bcf04b0b8e3c0f142ab0ec97627ea140c80c7962
Author: Lev Stipakov
Date: Mon Jan 17 11:49:17 2022 +0200
tun: remove tun_finalize()
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Selva Nair <selva.nair@gmail.com>
Message-Id: <20220117094917.178-1-lstipakov@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23555.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index f82386a1..a905f993 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -1115,7 +1115,8 @@ read_incoming_tun(struct context *c) } else { - read_tun_buffered(c->c1.tuntap, &c->c2.buf); + sockethandle_t sh = { .is_handle = true, .h = c->c1.tuntap->hand }; + sockethandle_finalize(sh, &c->c1.tuntap->reads, &c->c2.buf, NULL); } #else /* ifdef _WIN32 */ ASSERT(buf_init(&c->c2.buf, FRAME_HEADROOM(&c->c2.frame))); diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c index df736746..780c5cb3 100644 --- a/src/openvpn/socket.c +++ b/src/openvpn/socket.c @@ -3198,7 +3198,8 @@ link_socket_read_tcp(struct link_socket *sock, if (!sock->stream_buf.residual_fully_formed) { #ifdef _WIN32 - len = socket_finalize(sock->sd, &sock->reads, buf, NULL); + sockethandle_t sh = { .s = sock->sd }; + len = sockethandle_finalize(sh, &sock->reads, buf, NULL); #else struct buffer frag; stream_buf_get_next(&sock->stream_buf, &frag); @@ -3664,10 +3665,10 @@ socket_send_queue(struct link_socket *sock, struct buffer *buf, const struct lin } int -socket_finalize(SOCKET s, - struct overlapped_io *io, - struct buffer *buf, - struct link_socket_actual *from) +sockethandle_finalize(sockethandle_t sh, + struct overlapped_io *io, + struct buffer *buf, + struct link_socket_actual *from) { int ret = -1; BOOL status; @@ -3675,13 +3676,7 @@ socket_finalize(SOCKET s, switch (io->iostate) { case IOSTATE_QUEUED: - status = WSAGetOverlappedResult( - s, - &io->overlapped, - &io->size, - FALSE, - &io->flags - ); + status = SocketHandleGetOverlappedResult(sh, io); if (status) { /* successful return for a queued operation */ @@ -3693,18 +3688,18 @@ socket_finalize(SOCKET s, io->iostate = IOSTATE_INITIAL; ASSERT(ResetEvent(io->overlapped.hEvent)); - dmsg(D_WIN32_IO, "WIN32 I/O: Socket Completion success [%d]", ret); + dmsg(D_WIN32_IO, "WIN32 I/O: Completion success [%d]", ret); } else { /* error during a queued operation */ ret = -1; - if (WSAGetLastError() != WSA_IO_INCOMPLETE) + if (SocketHandleGetLastError(sh) != ERROR_IO_INCOMPLETE) { /* if no error (i.e. just not finished yet), then DON'T execute this code */ io->iostate = IOSTATE_INITIAL; ASSERT(ResetEvent(io->overlapped.hEvent)); - msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: Socket Completion error"); + msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: Completion error"); } } break; @@ -3715,9 +3710,9 @@ socket_finalize(SOCKET s, if (io->status) { /* error return for a non-queued operation */ - WSASetLastError(io->status); + SocketHandleSetLastError(sh, io->status); ret = -1; - msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: Socket Completion non-queued error"); + msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: Completion non-queued error"); } else { @@ -3727,14 +3722,14 @@ socket_finalize(SOCKET s, *buf = io->buf; } ret = io->size; - dmsg(D_WIN32_IO, "WIN32 I/O: Socket Completion non-queued success [%d]", ret); + dmsg(D_WIN32_IO, "WIN32 I/O: Completion non-queued success [%d]", ret); } break; case IOSTATE_INITIAL: /* were we called without proper queueing? */ - WSASetLastError(WSAEINVAL); + SocketHandleSetInvalError(sh); ret = -1; - dmsg(D_WIN32_IO, "WIN32 I/O: Socket Completion BAD STATE"); + dmsg(D_WIN32_IO, "WIN32 I/O: Completion BAD STATE"); break; default: @@ -3742,7 +3737,7 @@ socket_finalize(SOCKET s, } /* return from address if requested */ - if (from) + if (!sh.is_handle && from) { if (ret >= 0 && io->addr_defined) { diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h index cc1e0c36..77c3d720 100644 --- a/src/openvpn/socket.h +++ b/src/openvpn/socket.h @@ -262,11 +262,44 @@ int socket_send_queue(struct link_socket *sock, struct buffer *buf, const struct link_socket_actual *to); -int socket_finalize( - SOCKET s, - struct overlapped_io *io, - struct buffer *buf, - struct link_socket_actual *from); +typedef struct { + union { + SOCKET s; + HANDLE h; + }; + bool is_handle; +} sockethandle_t; + +int sockethandle_finalize(sockethandle_t sh, + struct overlapped_io *io, + struct buffer *buf, + struct link_socket_actual *from); + +static inline BOOL +SocketHandleGetOverlappedResult(sockethandle_t sh, struct overlapped_io *io) +{ + return sh.is_handle ? + GetOverlappedResult(sh.h, &io->overlapped, &io->size, FALSE) : + WSAGetOverlappedResult(sh.s, &io->overlapped, &io->size, FALSE, &io->flags); +} + +static inline int +SocketHandleGetLastError(sockethandle_t sh) +{ + return sh.is_handle ? (int)GetLastError() : WSAGetLastError(); +} + +inline static void +SocketHandleSetLastError(sockethandle_t sh, DWORD err) +{ + sh.is_handle ? SetLastError(err) : WSASetLastError(err); +} + +static inline void +SocketHandleSetInvalError(sockethandle_t sh) +{ + sh.is_handle ? SetLastError(ERROR_INVALID_FUNCTION) : WSASetLastError(WSAEINVAL); +} #else /* ifdef _WIN32 */ @@ -1020,7 +1053,8 @@ link_socket_read_udp_win32(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *from) { - return socket_finalize(sock->sd, &sock->reads, buf, from); + sockethandle_t sh = { .s = sock->sd }; + return sockethandle_finalize(sh, &sock->reads, buf, from); } #else /* ifdef _WIN32 */ @@ -1078,9 +1112,10 @@ link_socket_write_win32(struct link_socket *sock, { int err = 0; int status = 0; + sockethandle_t sh = { .s = sock->sd }; if (overlapped_io_active(&sock->writes)) { - status = socket_finalize(sock->sd, &sock->writes, NULL, NULL); + status = sockethandle_finalize(sh, &sock->writes, NULL, NULL); if (status < 0) { err = WSAGetLastError(); diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index 12bdd200..fe32127b 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -3561,87 +3561,29 @@ tun_write_queue(struct tuntap *tt, struct buffer *buf) } int -tun_finalize( - HANDLE h, - struct overlapped_io *io, - struct buffer *buf) +tun_write_win32(struct tuntap *tt, struct buffer *buf) { - int ret = -1; - BOOL status; - - switch (io->iostate) + int err = 0; + int status = 0; + if (overlapped_io_active(&tt->writes)) { - case IOSTATE_QUEUED: - status = GetOverlappedResult( - h, - &io->overlapped, - &io->size, - FALSE - ); - if (status) - { - /* successful return for a queued operation */ - if (buf) - { - *buf = io->buf; - } - ret = io->size; - io->iostate = IOSTATE_INITIAL; - ASSERT(ResetEvent(io->overlapped.hEvent)); - dmsg(D_WIN32_IO, "WIN32 I/O: TAP Completion success [%d]", ret); - } - else - { - /* error during a queued operation */ - ret = -1; - if (GetLastError() != ERROR_IO_INCOMPLETE) - { - /* if no error (i.e. just not finished yet), - * then DON'T execute this code */ - io->iostate = IOSTATE_INITIAL; - ASSERT(ResetEvent(io->overlapped.hEvent)); - msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: TAP Completion error"); - } - } - break; - - case IOSTATE_IMMEDIATE_RETURN: - io->iostate = IOSTATE_INITIAL; - ASSERT(ResetEvent(io->overlapped.hEvent)); - if (io->status) - { - /* error return for a non-queued operation */ - SetLastError(io->status); - ret = -1; - msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: TAP Completion non-queued error"); - } - else - { - /* successful return for a non-queued operation */ - if (buf) - { - *buf = io->buf; - } - ret = io->size; - dmsg(D_WIN32_IO, "WIN32 I/O: TAP Completion non-queued success [%d]", ret); - } - break; - - case IOSTATE_INITIAL: /* were we called without proper queueing? */ - SetLastError(ERROR_INVALID_FUNCTION); - ret = -1; - dmsg(D_WIN32_IO, "WIN32 I/O: TAP Completion BAD STATE"); - break; - - default: - ASSERT(0); + sockethandle_t sh = { .is_handle = true, .h = tt->hand }; + status = sockethandle_finalize(sh, &tt->writes, NULL, NULL); + if (status < 0) + { + err = GetLastError(); + } } - - if (buf) + tun_write_queue(tt, buf); + if (status < 0) { - buf->len = ret; + SetLastError(err); + return status; + } + else + { + return BLEN(buf); } - return ret; } static const struct device_instance_id_interface * diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h index d4657537..a6661be0 100644 --- a/src/openvpn/tun.h +++ b/src/openvpn/tun.h @@ -437,8 +437,6 @@ int tun_read_queue(struct tuntap *tt, int maxsize); int tun_write_queue(struct tuntap *tt, struct buffer *buf); -int tun_finalize(HANDLE h, struct overlapped_io *io, struct buffer *buf); - static inline bool tuntap_stop(int status) { @@ -466,36 +464,8 @@ tuntap_abort(int status) return false; } -static inline int -tun_write_win32(struct tuntap *tt, struct buffer *buf) -{ - int err = 0; - int status = 0; - if (overlapped_io_active(&tt->writes)) - { - status = tun_finalize(tt->hand, &tt->writes, NULL); - if (status < 0) - { - err = GetLastError(); - } - } - tun_write_queue(tt, buf); - if (status < 0) - { - SetLastError(err); - return status; - } - else - { - return BLEN(buf); - } -} - -static inline int -read_tun_buffered(struct tuntap *tt, struct buffer *buf) -{ - return tun_finalize(tt->hand, &tt->reads, buf); -} +int +tun_write_win32(struct tuntap *tt, struct buffer *buf); static inline ULONG wintun_ring_packet_align(ULONG size)