On Tue, 16 Jan 2018 23:49:30 +0100, Guus Sliepen wrote:
> Hm, there's indeed some differences between the two versions, even apart
> from the fact that one uses a thread and the other the event system. It
> seems that in write_packet(), tinc 1.0 does a ResetEvent() if a
> WriteFile() succeeded immediately, but 1.1 doesn't. Also, in tinc 1.1,
> for reads, ResetEvent() is called right before GetOverlappedResult(),
> and that looks very suspicious... with tinc 1.0 it is called right
> before ReadFile().
>
> Overlapped IO feels alien to me.
I think the 1.1 device.c should call ResetEvent() before calling
WriteFile(). This is simpler than resetting the event after a
successful WriteFile() or error.
Another difference between 1.0 and 1.1 is what happens if the write
hasn't completed when it needs to perform a further write. In 1.0,
write_packet() returns true when the packet is dropped, whereas in
1.1 it returns false.
I suppose it doesn't make much difference since the return value
is never checked but returning false seems wrong since it was not
a write error.
The diff below resets the write event before WriteFile(), changes
the return value on packet drop to match 1.0, and adds a missing
"device_write_packet.len = 0" on an error path.
Does this make sense to you?
- todd
diff --git a/src/mingw/device.c b/src/mingw/device.c
index d90c69b..315c08d 100644
--- a/src/mingw/device.c
+++ b/src/mingw/device.c
@@ -311,9 +311,13 @@ static bool write_packet(vpn_packet_t *packet) {
which according to MSDN is a no-no. */
if(!GetOverlappedResult(device_handle,
&device_write_overlapped, &outlen, FALSE)) {
- int log_level = (GetLastError() == ERROR_IO_INCOMPLETE)
? DEBUG_TRAFFIC : DEBUG_ALWAYS;
- logger(log_level, LOG_ERR, "Error while checking
previous write to %s %s: %s", device_info, device, winerror(GetLastError()));
- return false;
+ if(GetLastError() != ERROR_IO_INCOMPLETE) {
+ logger(DEBUG_ALWAYS, LOG_ERR, "Error completing
previously queued write to %s %s: %s", device_info, device,
winerror(GetLastError));
+ } else {
+ logger(DEBUG_TRAFFIC, LOG_ERR, "Previous
overlapped write to %s %s still in progress", device_info, device);
+ // drop this packet
+ return true;
+ }
}
}
@@ -321,10 +325,14 @@ static bool write_packet(vpn_packet_t *packet) {
memcpy(&device_write_packet, packet, sizeof(*packet));
+ ResetEvent(device_write_overlapped.hEvent);
+
if(WriteFile(device_handle, DATA(&device_write_packet),
device_write_packet.len, &outlen, &device_write_overlapped)) {
+ // Write was completed immediately.
device_write_packet.len = 0;
} else if(GetLastError() != ERROR_IO_PENDING) {
logger(DEBUG_ALWAYS, LOG_ERR, "Error while writing to %s %s:
%s", device_info, device, winerror(GetLastError()));
+ device_write_packet.len = 0;
return false;
}
_______________________________________________
tinc mailing list
[email protected]
https://www.tinc-vpn.org/cgi-bin/mailman/listinfo/tinc
