search

OpenVPN / tap-windows6

Windows TAP driver (NDIS 6)
Other
785 stars 237 forks source link

Remove NCF_HAS_UI flag from Characteristics #94

Closed rozmansi closed 5 years ago

rozmansi commented 5 years ago

This flag indicates that the device has a supplemental DLL with a custom user interface. However, the TAP-Windows6 does not have any custom UI other than standard device registry properties.

Furthermore, HP Envy laptops have a built-in feature to turn off WiFi automatically when an Ethernet NIC gets connected. This feature actually checks if the Ethernet NIC is NCF_VIRTUAL to ignore it. Unfortunately, it literally checks Characteristics == NCF_VIRTUAL instead of (Characteristics & NCF_VIRTUAL) != 0. The end result is, an HP Envy laptop turns off WiFi when OpenVPN connects killing its only data link.

This commit remedies the HP Envy issue on the OpenVPN end. Thou it should be fixed at the source - namely HP. Preparations to report this issue to HP are underway.

Reference: https://docs.microsoft.com/en-us/windows-hardware/drivers/network/ddinstall-section-in-a-network-inf-file

selvanair commented 5 years ago

The docs say NCF_HAS_UI indicates "Component supports a user interface (for example, the Advanced Page or a custom properties sheet)." We do have an advanced tab in device properties but if this refers to custom property pages handled by helper dlls, the patch makes perfect sense. Also, considering viscosity's version works without this flag, it does seem unnecessary.

As for HP envy, in reply to my suggestion to try NCF_VIRTUAL, JJK replied that the change did not help the wifi disconnection problem. Have you tested this on that laptop? However, this is irrelevant, as the HP envy silliness is not a good argument, nor necessary, to justify such a change. We can't possibly alter settings to suit an obscure laptop which may not be even in the market by the time this version is released.

rozmansi commented 5 years ago

As for HP envy, in reply to my suggestion to try NCF_VIRTUAL, JJK replied that the change did not help the wifi disconnection problem. Have you tested this on that laptop?

JJK tried to change it in the registry after the driver was installed and TAP adapter created, AFAIK. I have changed it at the source: in the .inf file.

I have not tested it personally, as I do not possess any HP Envy laptop. I have compiled the driver using this commit and shipped it to an organization which uses HP Envy laptops exclusively. Because of this issue, the entire organization is unable to use OpenVPN nor eduVPN or any other solution based on TAP-Windows6 driver. They confirmed that replacing with this driver fixed the problem.

However, this is irrelevant, as the HP envy silliness is not a good argument, nor necessary, to justify such a change. We can't possibly alter settings to suit an obscure laptop which may not be even in the market by the time this version is released.

It is just one of the arguments to make a change. Please, do not forget the original argument that our settings are wrong: TAP-Windows6 is not using a custom UI DLL and the NCF_HAS_UI flag is redundant and misleading.

Besides, I do not agree with an attitude "We're not fixing our driver. You change your laptops!"

schwabe commented 5 years ago

I looked a bit a the documentation and it seems that the flag we set (NCF_HAS_UI, 0x80) is required for custom property pages. But we don't have those.

I think this fixes a bug/cosmetic issue in our driver. That it also fixes HP's broken driver stack is just a nice side effect.

selvanair commented 5 years ago

Besides, I do not agree with an attitude "We're not fixing our driver. You change your laptops!"

Sorry if it came out that way -- I had no intention of implying anything like that. If the flag is redundant or wrongly set, we should get rid of it.

mattock commented 5 years ago

I'll merge this soon if nobody objects.

cron2 commented 5 years ago

Hi,

On Sun, Aug 04, 2019 at 11:15:45PM -0700, Samuli Seppänen wrote:

I'll merge this soon if nobody objects.

From the discussion it seems nobody objects and this fixes a "wart" and in passing works around a HPE annoyance :-) - so double-win.

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

mattock commented 5 years ago

Everyone in today's community meeting agreed that we can merge this, so I'll do just that now :smile:.

cron2 commented 4 years ago

jjk confirmed that the test installer provided indeed fixes the HP Envy Wifi issue. So, merged, and actually tested as well :-)