| Message ID | 20200902213643.401-1-lstipakov@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v2] openvpnmsica: make adapter renaming non-fatal | expand |
Hi, > -----Original Message----- > From: Lev Stipakov <lstipakov@gmail.com> > Sent: Wednesday, September 2, 2020 11:37 PM > To: openvpn-devel@lists.sourceforge.net > Cc: Lev Stipakov <lev@openvpn.net> > Subject: [Openvpn-devel] [PATCH v2] openvpnmsica: make adapter renaming > non-fatal > > From: Lev Stipakov <lev@openvpn.net> > > For some users renaming adapter > > Local Area Connection > OpenVPN TAP-Windows6 > > mysteriously fails, see https://github.com/OpenVPN/openvpn- > build/issues/187 > > Since renaming is just a "nice to have", make it non-fatal > and, in case of error, only log message and don't display messagebox. > > Signed-off-by: Lev Stipakov <lev@openvpn.net> > --- > > v2: only log error, don't display messagebox > > src/openvpnmsica/dllmain.c | 2 +- > src/openvpnmsica/openvpnmsica.c | 9 +++------ > src/tapctl/main.c | 2 +- > src/tapctl/tap.c | 11 +++++++---- > src/tapctl/tap.h | 6 +++++- > 5 files changed, 17 insertions(+), 13 deletions(-) > > diff --git a/src/openvpnmsica/dllmain.c b/src/openvpnmsica/dllmain.c > index 201fd9af..34946ed8 100644 > --- a/src/openvpnmsica/dllmain.c > +++ b/src/openvpnmsica/dllmain.c > @@ -193,6 +193,6 @@ x_msg_va(const unsigned int flags, const char > *format, va_list arglist) > } > } > > - MsiProcessMessage(s->hInstall, INSTALLMESSAGE_ERROR, hRecordProg); > + MsiProcessMessage(s->hInstall, (flags & M_WARN) ? > INSTALLMESSAGE_INFO : INSTALLMESSAGE_ERROR, hRecordProg); > MsiCloseHandle(hRecordProg); > } > diff --git a/src/openvpnmsica/openvpnmsica.c > b/src/openvpnmsica/openvpnmsica.c > index 31e90bd2..f203f736 100644 > --- a/src/openvpnmsica/openvpnmsica.c > +++ b/src/openvpnmsica/openvpnmsica.c > @@ -1096,12 +1096,9 @@ ProcessDeferredAction(_In_ MSIHANDLE hInstall) > dwResult = tap_create_adapter(NULL, NULL, szHardwareId, > &bRebootRequired, &guidAdapter); > if (dwResult == ERROR_SUCCESS) > { > - /* Set adapter name. */ > - dwResult = tap_set_adapter_name(&guidAdapter, szName); > - if (dwResult != ERROR_SUCCESS) > - { > - tap_delete_adapter(NULL, &guidAdapter, > &bRebootRequired); > - } > + /* Set adapter name. May fail on some machines, but > that is not critical - use silent > + flag to mute messagebox and print error only to log > */ > + tap_set_adapter_name(&guidAdapter, szName, TRUE); > } > } > else if (wcsncmp(szArg[i], L"deleteN=", 8) == 0) > diff --git a/src/tapctl/main.c b/src/tapctl/main.c > index 31bb2ec7..d5bc7290 100644 > --- a/src/tapctl/main.c > +++ b/src/tapctl/main.c > @@ -237,7 +237,7 @@ _tmain(int argc, LPCTSTR argv[]) > } > > /* Rename the adapter. */ > - dwResult = tap_set_adapter_name(&guidAdapter, szName); > + dwResult = tap_set_adapter_name(&guidAdapter, szName, > FALSE); > if (dwResult != ERROR_SUCCESS) > { > StringFromIID((REFIID)&guidAdapter, &szAdapterId); > diff --git a/src/tapctl/tap.c b/src/tapctl/tap.c > index 7cb3dedc..0dfe239f 100644 > --- a/src/tapctl/tap.c > +++ b/src/tapctl/tap.c > @@ -1140,9 +1140,12 @@ ExecCommand(const WCHAR* cmdline) > DWORD > tap_set_adapter_name( > _In_ LPCGUID pguidAdapter, > - _In_ LPCTSTR szName) > + _In_ LPCTSTR szName, > + _In_ BOOL bSilent) > { > DWORD dwResult; > + int msg_flag = bSilent ? M_WARN : M_NONFATAL; > + msg_flag |= M_ERRNO; > > if (pguidAdapter == NULL || szName == NULL) > { > @@ -1176,7 +1179,7 @@ tap_set_adapter_name( > if (dwResult != ERROR_SUCCESS) > { > SetLastError(dwResult); /* MSDN does not mention RegOpenKeyEx() > to set GetLastError(). But we do have an error code. Set last error > manually. */ > - msg(M_NONFATAL | M_ERRNO, "%s: RegOpenKeyEx(HKLM, \"%" > PRIsLPTSTR "\") failed", __FUNCTION__, szRegKey); > + msg(msg_flag, "%s: RegOpenKeyEx(HKLM, \"%" PRIsLPTSTR "\") > failed", __FUNCTION__, szRegKey); > goto cleanup_szAdapterId; > } > > @@ -1185,7 +1188,7 @@ tap_set_adapter_name( > if (dwResult != ERROR_SUCCESS) > { > SetLastError(dwResult); > - msg(M_NONFATAL | M_ERRNO, "%s: Error reading adapter name", > __FUNCTION__); > + msg(msg_flag, "%s: Error reading adapter name", __FUNCTION__); > goto cleanup_hKey; > } > > @@ -1203,7 +1206,7 @@ tap_set_adapter_name( > if (dwResult != ERROR_SUCCESS) > { > SetLastError(dwResult); > - msg(M_NONFATAL | M_ERRNO, "%s: Error renaming adapter", > __FUNCTION__); > + msg(msg_flag, "%s: Error renaming adapter", __FUNCTION__); > goto cleanup_hKey; > } > > diff --git a/src/tapctl/tap.h b/src/tapctl/tap.h > index 102de32d..1f531cf2 100644 > --- a/src/tapctl/tap.h > +++ b/src/tapctl/tap.h > @@ -117,13 +117,17 @@ tap_enable_adapter( > * @param pguidAdapter A pointer to GUID that contains network adapter > ID. > * > * @param szName New adapter name - must be unique > + * > + * @param bSilent If true, MSI installer won't display message > box and > + * only print error to log. > * > * @return ERROR_SUCCESS on success; Win32 error code otherwise > **/ > DWORD > tap_set_adapter_name( > _In_ LPCGUID pguidAdapter, > - _In_ LPCTSTR szName); > + _In_ LPCTSTR szName, > + _In_ BOOL bSilent); > > > /** > -- > 2.17.1 > > > > _______________________________________________ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel I have reviewed the code, compiled it, built MSI, tested it. Although I could not create a phantom TUN adapter to reproduce the issue. Renaming "Ethernet" adapter to "OpenVPN TAP-Windows6" and trying to install MSI next displayed an error in evaluation phase that a foreign adapter with the same name already exists - which is expected. This patch is about solving the situation when: 1. There are no apparent TAP-Windows6 or Wintun adapters present => installation decides to create one. 2. Once adapter is created, the renaming to the desired name fails (as if the name is already taken). I agree with everybody that having a consistently named adapter after initial setup is nice, but not that essential to make the installation fail. So... Acked-by: Simon Rozman <simon@rozman.si> Regards, Simon
Your patch has been applied to the master and release/2.5 branch.
Thanks for coming up with the solution & testing & reviewing it!
The comment line is a bit long... uncrustify will catch it next time.
I have not tested anything, trusting Lev and Simon here :-) (but
from a cursory glance, the change looks nice and reasonable)
commit b341b1c596931f8a1f77f054ba505b5c1e3547be (master)
commit 567e277f1cbaa8059319a8feea4c08edf77b6a5a (release/2.5)
Author: Lev Stipakov
Date: Thu Sep 3 00:36:43 2020 +0300
openvpnmsica: make adapter renaming non-fatal
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Simon Rozman <simon@rozman.si>
Message-Id: <20200902213643.401-1-lstipakov@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20875.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpnmsica/dllmain.c b/src/openvpnmsica/dllmain.c index 201fd9af..34946ed8 100644 --- a/src/openvpnmsica/dllmain.c +++ b/src/openvpnmsica/dllmain.c @@ -193,6 +193,6 @@ x_msg_va(const unsigned int flags, const char *format, va_list arglist) } } - MsiProcessMessage(s->hInstall, INSTALLMESSAGE_ERROR, hRecordProg); + MsiProcessMessage(s->hInstall, (flags & M_WARN) ? INSTALLMESSAGE_INFO : INSTALLMESSAGE_ERROR, hRecordProg); MsiCloseHandle(hRecordProg); } diff --git a/src/openvpnmsica/openvpnmsica.c b/src/openvpnmsica/openvpnmsica.c index 31e90bd2..f203f736 100644 --- a/src/openvpnmsica/openvpnmsica.c +++ b/src/openvpnmsica/openvpnmsica.c @@ -1096,12 +1096,9 @@ ProcessDeferredAction(_In_ MSIHANDLE hInstall) dwResult = tap_create_adapter(NULL, NULL, szHardwareId, &bRebootRequired, &guidAdapter); if (dwResult == ERROR_SUCCESS) { - /* Set adapter name. */ - dwResult = tap_set_adapter_name(&guidAdapter, szName); - if (dwResult != ERROR_SUCCESS) - { - tap_delete_adapter(NULL, &guidAdapter, &bRebootRequired); - } + /* Set adapter name. May fail on some machines, but that is not critical - use silent + flag to mute messagebox and print error only to log */ + tap_set_adapter_name(&guidAdapter, szName, TRUE); } } else if (wcsncmp(szArg[i], L"deleteN=", 8) == 0) diff --git a/src/tapctl/main.c b/src/tapctl/main.c index 31bb2ec7..d5bc7290 100644 --- a/src/tapctl/main.c +++ b/src/tapctl/main.c @@ -237,7 +237,7 @@ _tmain(int argc, LPCTSTR argv[]) } /* Rename the adapter. */ - dwResult = tap_set_adapter_name(&guidAdapter, szName); + dwResult = tap_set_adapter_name(&guidAdapter, szName, FALSE); if (dwResult != ERROR_SUCCESS) { StringFromIID((REFIID)&guidAdapter, &szAdapterId); diff --git a/src/tapctl/tap.c b/src/tapctl/tap.c index 7cb3dedc..0dfe239f 100644 --- a/src/tapctl/tap.c +++ b/src/tapctl/tap.c @@ -1140,9 +1140,12 @@ ExecCommand(const WCHAR* cmdline) DWORD tap_set_adapter_name( _In_ LPCGUID pguidAdapter, - _In_ LPCTSTR szName) + _In_ LPCTSTR szName, + _In_ BOOL bSilent) { DWORD dwResult; + int msg_flag = bSilent ? M_WARN : M_NONFATAL; + msg_flag |= M_ERRNO; if (pguidAdapter == NULL || szName == NULL) { @@ -1176,7 +1179,7 @@ tap_set_adapter_name( if (dwResult != ERROR_SUCCESS) { SetLastError(dwResult); /* MSDN does not mention RegOpenKeyEx() to set GetLastError(). But we do have an error code. Set last error manually. */ - msg(M_NONFATAL | M_ERRNO, "%s: RegOpenKeyEx(HKLM, \"%" PRIsLPTSTR "\") failed", __FUNCTION__, szRegKey); + msg(msg_flag, "%s: RegOpenKeyEx(HKLM, \"%" PRIsLPTSTR "\") failed", __FUNCTION__, szRegKey); goto cleanup_szAdapterId; } @@ -1185,7 +1188,7 @@ tap_set_adapter_name( if (dwResult != ERROR_SUCCESS) { SetLastError(dwResult); - msg(M_NONFATAL | M_ERRNO, "%s: Error reading adapter name", __FUNCTION__); + msg(msg_flag, "%s: Error reading adapter name", __FUNCTION__); goto cleanup_hKey; } @@ -1203,7 +1206,7 @@ tap_set_adapter_name( if (dwResult != ERROR_SUCCESS) { SetLastError(dwResult); - msg(M_NONFATAL | M_ERRNO, "%s: Error renaming adapter", __FUNCTION__); + msg(msg_flag, "%s: Error renaming adapter", __FUNCTION__); goto cleanup_hKey; } diff --git a/src/tapctl/tap.h b/src/tapctl/tap.h index 102de32d..1f531cf2 100644 --- a/src/tapctl/tap.h +++ b/src/tapctl/tap.h @@ -117,13 +117,17 @@ tap_enable_adapter( * @param pguidAdapter A pointer to GUID that contains network adapter ID. * * @param szName New adapter name - must be unique + * + * @param bSilent If true, MSI installer won't display message box and + * only print error to log. * * @return ERROR_SUCCESS on success; Win32 error code otherwise **/ DWORD tap_set_adapter_name( _In_ LPCGUID pguidAdapter, - _In_ LPCTSTR szName); + _In_ LPCTSTR szName, + _In_ BOOL bSilent); /**