| Message ID | 20230518162350.1633-1-lstipakov@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel,v2] tapctl: generate driver-specific adapter names | expand |
Hi, On Thu, May 18, 2023 at 12:25 PM Lev Stipakov <lstipakov@gmail.com> wrote: > From: Lev Stipakov <lev@openvpn.net> > > At the moment if --name is not specified, adapter names > are generated by Windows and they look a bit confusing > like "Local Area Connection 2". > > This is also behavior of "Add a new <driver-name> virtual network > adapter" shortcuts. > > This makes tapctl generate driver-specific names for adapters > if --name is missing, inclusing resolving duplicates. For instance > following commands: > > tapctl.exe create --hwid ovpn-dco > > will create an adapter named > > OpenVPN Data Channel Offload > > If the name is taken, the next one will be > > OpenVPN Data Channel Offload #1 > > and so on up to 100. > > Fixes https://github.com/OpenVPN/openvpn/issues/337 Looks good but some comments below -- mostly nitpicks. > > Change-Id: Ic5afb470d14ac7b231d91f0f5de0a0046043a7e0 > Signed-off-by: Lev Stipakov <lev@openvpn.net> > --- > src/tapctl/main.c | 133 +++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 107 insertions(+), 26 deletions(-) > > diff --git a/src/tapctl/main.c b/src/tapctl/main.c > index 1194036f..e4d44794 100644 > --- a/src/tapctl/main.c > +++ b/src/tapctl/main.c > @@ -126,6 +126,90 @@ usage(void) > title_string); > } > > +/** > + * Checks if adapter with given name doesn't already exist > + */ > +BOOL > I would make this static > +is_adapter_name_available(LPCTSTR name, struct tap_adapter_node > *adapter_list, BOOL log) > +{ > + for (struct tap_adapter_node *a = adapter_list; a; a = a->pNext) > + { > + if (_tcsicmp(name, a->szName) == 0) > + { > + if (log) > + { > + LPOLESTR adapter_id = NULL; > + StringFromIID((REFIID)&a->guid, &adapter_id); > + _ftprintf(stderr, TEXT("Adapter \"%") TEXT(PRIsLPTSTR) > TEXT("\" already exists (GUID %") > + TEXT(PRIsLPOLESTR) TEXT(").\n"), a->szName, > adapter_id); > + CoTaskMemFree(adapter_id); > + } > + > + return FALSE; > + } > + } > + > + return TRUE; > +} > + > +/** > + * Returns unique adapter name based on hwid or NULL if name cannot be > generated. > + * Caller is responsible for freeing it. > + */ > +LPTSTR > same static here too? > +get_unique_adapter_name(LPCTSTR hwid, struct tap_adapter_node > *adapter_list) > +{ > + if (hwid == NULL) > + { > + return NULL; > + } > + > + LPCTSTR base_name; > + if (_tcsicmp(hwid, TEXT("ovpn-dco")) == 0) > + { > + base_name = TEXT("OpenVPN Data Channel Offload"); > + } > + else if (_tcsicmp(hwid, TEXT("wintun")) == 0) > + { > + base_name = TEXT("OpenVPN Wintun"); > + } > + else if (_tcsicmp(hwid, TEXT("root\\") TEXT(TAP_WIN_COMPONENT_ID)) == > 0) > + { > + base_name = TEXT("OpenVPN TAP-Windows6"); > + } > + else > + { > + return NULL; > + } > + > + if (is_adapter_name_available(base_name, adapter_list, FALSE)) > + { > + return _tcsdup(base_name); > + } > + > + TCHAR itot_buf[10]; > + size_t name_len = (_tcslen(base_name) + sizeof(itot_buf) / > sizeof(TCHAR) + 2); > _countof(itot_buf) is what we normally use when SIZE() is not available. That said, do we need itot_buf[] at all? see below. > + LPTSTR name = malloc(name_len * sizeof(TCHAR)); > + if (name == NULL) > + { > + return NULL; > + } > + for (int i = 1; i < 100; ++i) > + { > + *name = TEXT('\0'); > + _tcscat_s(name, name_len, base_name); > + _tcscat_s(name, name_len, TEXT(" #")); > + _itot_s(i, itot_buf, sizeof(itot_buf) / sizeof(TCHAR), 10); > + _tcscat_s(name, name_len, itot_buf); > It looks much simpler to write the above 5 lines as _stprintf_s(name, name_len, TEXT("%ls #%d"), base_name, i) Well, the format string should be TEXT("%") TEXT(PRIsLPTSTR) TEXT(" #%d"), but we are not going to build this ever without UNICODE. + > + if (is_adapter_name_available(name, adapter_list, FALSE)) > + { > + return name; > + } > + } > + > + return NULL; > +} > > /** > * Program entry point > @@ -210,50 +294,47 @@ _tmain(int argc, LPCTSTR argv[]) > iResult = 1; goto quit; > } > > - if (szName) > + /* Get existing network adapters. */ > + struct tap_adapter_node *pAdapterList = NULL; > + dwResult = tap_list_adapters(NULL, NULL, &pAdapterList); > + if (dwResult != ERROR_SUCCESS) > { > - /* Get existing network adapters. */ > - struct tap_adapter_node *pAdapterList = NULL; > - dwResult = tap_list_adapters(NULL, NULL, &pAdapterList); > - if (dwResult != ERROR_SUCCESS) > - { > - _ftprintf(stderr, TEXT("Enumerating adapters failed > (error 0x%x).\n"), dwResult); > - iResult = 1; goto create_delete_adapter; > - } > + _ftprintf(stderr, TEXT("Enumerating adapters failed (error > 0x%x).\n"), dwResult); > + iResult = 1; goto create_delete_adapter; > Add a line-break to avoid multiple statements per line? I know it's as in the original, but we can "fix" at least the contexts that are touched.. > + } > > - /* Check for duplicates. */ > - for (struct tap_adapter_node *pAdapter = pAdapterList; > pAdapter; pAdapter = pAdapter->pNext) > + LPTSTR adapter_name = szName ? _tcsdup(szName) : > get_unique_adapter_name(szHwId, pAdapterList); > + if (adapter_name) > + { > + /* Check for duplicates when name was specified, > + * otherwise get_adapter_default_name() takes care of it */ > + if (szName && !is_adapter_name_available(adapter_name, > pAdapterList, TRUE)) > { > - if (_tcsicmp(szName, pAdapter->szName) == 0) > - { > - StringFromIID((REFIID)&pAdapter->guid, &szAdapterId); > - _ftprintf(stderr, TEXT("Adapter \"%") > TEXT(PRIsLPTSTR) TEXT("\" already exists (GUID %") > - TEXT(PRIsLPOLESTR) TEXT(").\n"), > pAdapter->szName, szAdapterId); > - CoTaskMemFree(szAdapterId); > - iResult = 1; goto create_cleanup_pAdapterList; > - } > + iResult = 1; goto create_cleanup_pAdapterList; > same here.. > } > > /* Rename the adapter. */ > - dwResult = tap_set_adapter_name(&guidAdapter, szName, FALSE); > + dwResult = tap_set_adapter_name(&guidAdapter, adapter_name, > FALSE); > if (dwResult != ERROR_SUCCESS) > { > StringFromIID((REFIID)&guidAdapter, &szAdapterId); > _ftprintf(stderr, TEXT("Renaming TUN/TAP adapter %") > TEXT(PRIsLPOLESTR) > TEXT(" to \"%") TEXT(PRIsLPTSTR) TEXT("\" > failed (error 0x%x).\n"), > - szAdapterId, szName, dwResult); > + szAdapterId, adapter_name, dwResult); > CoTaskMemFree(szAdapterId); > iResult = 1; goto quit; > .. > } > > iResult = 0; > + } > If (adapter_name) is false, we reach here with iResult not set, but it gets referenced below. Add an else { iResult = 1; } or initialize iResult to 1 at top? > create_cleanup_pAdapterList: > - tap_free_adapter_list(pAdapterList); > - if (iResult) > - { > - goto create_delete_adapter; > - } > + free(adapter_name); > + > + tap_free_adapter_list(pAdapterList); > + if (iResult) > + { > + goto create_delete_adapter; > } > > /* Output adapter GUID. */ Selva
Hi, > It looks much simpler to write the above 5 lines as > > _stprintf_s(name, name_len, TEXT("%ls #%d"), base_name, i) Agreed. > If (adapter_name) is false, we reach here with iResult not set, but it gets referenced below. Add an else { iResult = 1; } or initialize iResult to 1 at top? Normally this should not happen, adapter_name is currently set either by --name or using a hwid-generated name. But surely let's handle this case too. I think iResult should be set to 0, since if we don't set the name this is not an error - same behavior has been before. Agreed with the rest, V3 is coming.
diff --git a/src/tapctl/main.c b/src/tapctl/main.c index 1194036f..e4d44794 100644 --- a/src/tapctl/main.c +++ b/src/tapctl/main.c @@ -126,6 +126,90 @@ usage(void) title_string); } +/** + * Checks if adapter with given name doesn't already exist + */ +BOOL +is_adapter_name_available(LPCTSTR name, struct tap_adapter_node *adapter_list, BOOL log) +{ + for (struct tap_adapter_node *a = adapter_list; a; a = a->pNext) + { + if (_tcsicmp(name, a->szName) == 0) + { + if (log) + { + LPOLESTR adapter_id = NULL; + StringFromIID((REFIID)&a->guid, &adapter_id); + _ftprintf(stderr, TEXT("Adapter \"%") TEXT(PRIsLPTSTR) TEXT("\" already exists (GUID %") + TEXT(PRIsLPOLESTR) TEXT(").\n"), a->szName, adapter_id); + CoTaskMemFree(adapter_id); + } + + return FALSE; + } + } + + return TRUE; +} + +/** + * Returns unique adapter name based on hwid or NULL if name cannot be generated. + * Caller is responsible for freeing it. + */ +LPTSTR +get_unique_adapter_name(LPCTSTR hwid, struct tap_adapter_node *adapter_list) +{ + if (hwid == NULL) + { + return NULL; + } + + LPCTSTR base_name; + if (_tcsicmp(hwid, TEXT("ovpn-dco")) == 0) + { + base_name = TEXT("OpenVPN Data Channel Offload"); + } + else if (_tcsicmp(hwid, TEXT("wintun")) == 0) + { + base_name = TEXT("OpenVPN Wintun"); + } + else if (_tcsicmp(hwid, TEXT("root\\") TEXT(TAP_WIN_COMPONENT_ID)) == 0) + { + base_name = TEXT("OpenVPN TAP-Windows6"); + } + else + { + return NULL; + } + + if (is_adapter_name_available(base_name, adapter_list, FALSE)) + { + return _tcsdup(base_name); + } + + TCHAR itot_buf[10]; + size_t name_len = (_tcslen(base_name) + sizeof(itot_buf) / sizeof(TCHAR) + 2); + LPTSTR name = malloc(name_len * sizeof(TCHAR)); + if (name == NULL) + { + return NULL; + } + for (int i = 1; i < 100; ++i) + { + *name = TEXT('\0'); + _tcscat_s(name, name_len, base_name); + _tcscat_s(name, name_len, TEXT(" #")); + _itot_s(i, itot_buf, sizeof(itot_buf) / sizeof(TCHAR), 10); + _tcscat_s(name, name_len, itot_buf); + + if (is_adapter_name_available(name, adapter_list, FALSE)) + { + return name; + } + } + + return NULL; +} /** * Program entry point @@ -210,50 +294,47 @@ _tmain(int argc, LPCTSTR argv[]) iResult = 1; goto quit; } - if (szName) + /* Get existing network adapters. */ + struct tap_adapter_node *pAdapterList = NULL; + dwResult = tap_list_adapters(NULL, NULL, &pAdapterList); + if (dwResult != ERROR_SUCCESS) { - /* Get existing network adapters. */ - struct tap_adapter_node *pAdapterList = NULL; - dwResult = tap_list_adapters(NULL, NULL, &pAdapterList); - if (dwResult != ERROR_SUCCESS) - { - _ftprintf(stderr, TEXT("Enumerating adapters failed (error 0x%x).\n"), dwResult); - iResult = 1; goto create_delete_adapter; - } + _ftprintf(stderr, TEXT("Enumerating adapters failed (error 0x%x).\n"), dwResult); + iResult = 1; goto create_delete_adapter; + } - /* Check for duplicates. */ - for (struct tap_adapter_node *pAdapter = pAdapterList; pAdapter; pAdapter = pAdapter->pNext) + LPTSTR adapter_name = szName ? _tcsdup(szName) : get_unique_adapter_name(szHwId, pAdapterList); + if (adapter_name) + { + /* Check for duplicates when name was specified, + * otherwise get_adapter_default_name() takes care of it */ + if (szName && !is_adapter_name_available(adapter_name, pAdapterList, TRUE)) { - if (_tcsicmp(szName, pAdapter->szName) == 0) - { - StringFromIID((REFIID)&pAdapter->guid, &szAdapterId); - _ftprintf(stderr, TEXT("Adapter \"%") TEXT(PRIsLPTSTR) TEXT("\" already exists (GUID %") - TEXT(PRIsLPOLESTR) TEXT(").\n"), pAdapter->szName, szAdapterId); - CoTaskMemFree(szAdapterId); - iResult = 1; goto create_cleanup_pAdapterList; - } + iResult = 1; goto create_cleanup_pAdapterList; } /* Rename the adapter. */ - dwResult = tap_set_adapter_name(&guidAdapter, szName, FALSE); + dwResult = tap_set_adapter_name(&guidAdapter, adapter_name, FALSE); if (dwResult != ERROR_SUCCESS) { StringFromIID((REFIID)&guidAdapter, &szAdapterId); _ftprintf(stderr, TEXT("Renaming TUN/TAP adapter %") TEXT(PRIsLPOLESTR) TEXT(" to \"%") TEXT(PRIsLPTSTR) TEXT("\" failed (error 0x%x).\n"), - szAdapterId, szName, dwResult); + szAdapterId, adapter_name, dwResult); CoTaskMemFree(szAdapterId); iResult = 1; goto quit; } iResult = 0; + } create_cleanup_pAdapterList: - tap_free_adapter_list(pAdapterList); - if (iResult) - { - goto create_delete_adapter; - } + free(adapter_name); + + tap_free_adapter_list(pAdapterList); + if (iResult) + { + goto create_delete_adapter; } /* Output adapter GUID. */