[Openvpn-devel,3/3,v2] Print format spec changes for tapctl and openvpnmscia

Message ID 20210525173838.3969-1-selva.nair@gmail.com
State Accepted
Headers show
Series None | expand

Commit Message

Selva Nair May 25, 2021, 7:38 a.m. UTC
From: Selva Nair <selva.nair@gmail.com>

The tapctl and openvpnmscia codebase is written with an intent of
supporting both unicode and ansi builds.  This patch does not attempt
to change that although non-unicode support looks untested
and buggy.

The main change is to replace %s by PRIsLPTSR that is defined
as %ls or %s depending on _UNICODE is defined ot not.

v2: add missing ')' and fix whitespace

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpnmsica/openvpnmsica.c |  6 +++--
 src/tapctl/main.c               | 39 +++++++++++++++++++++------------
 src/tapctl/tap.c                |  3 ++-
 3 files changed, 31 insertions(+), 17 deletions(-)

Comments

Lev Stipakov May 26, 2021, 9:05 p.m. UTC | #1
> -    _stprintf_s(szTitle, _countof(szTitle), TEXT("%s v%s"), szFunctionName, TEXT(PACKAGE_VERSION));
> +    _stprintf_s(szTitle, _countof(szTitle), TEXT("%") TEXT(PRIsLPTSTR) TEXT("v%") TEXT(PRIsLPTSTR),

Just a note that a whitespace is (still) missing before "v%", but it
is fixed in subsequent patch ("Replace TEXT(__FUNCTION__) by
__FUNCTION__ in openvpnmscia.c").

Looks good - compared with v1, compiled/tested with MSVC.

Acked-by: Lev Stipakov <lstipakov@gmail.com>
Gert Doering May 26, 2021, 9:11 p.m. UTC | #2
Have not tested this, just stared a bit at the code.  Did not "fix"
the "%s v%s" issue Lev pointed out as this will be fixed in the next
patch...  thanks.

(I do wonder if "support ansi builds" is really something we want, if
that makes the code so complicated - "always UNICODE" seems to be much
simpler.  What is the drawback of not supporting "ansi" builds?)

Your patch has been applied to the master branch.

commit a13f6b16b20d7cae7f1ff50d325e13e303c8d3b3
Author: Selva Nair
Date:   Tue May 25 13:38:38 2021 -0400

     Print format spec changes for tapctl and openvpnmscia

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Lev Stipakov <lstipakov@gmail.com>
     Message-Id: <20210525173838.3969-1-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22453.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Selva Nair May 27, 2021, 5:40 a.m. UTC | #3
Hi

On Thu, May 27, 2021 at 3:11 AM Gert Doering <gert@greenie.muc.de> wrote:
>
> Have not tested this, just stared a bit at the code.  Did not "fix"
> the "%s v%s" issue Lev pointed out as this will be fixed in the next
> patch...  thanks.
>
> (I do wonder if "support ansi builds" is really something we want, if
> that makes the code so complicated - "always UNICODE" seems to be much
> simpler.  What is the drawback of not supporting "ansi" builds?)

I prefer to write all new Windows code as unicode without any dependency on
the defines UNICODE and  _UNICODE --- i.e no TCHARs, explicit API calls,
no _tprintfs.

For old code, an easy option would be to retain the TCHARs, but assume
we only build with UNICODE (TCHAR=WCHAR) and going forward don't care
to support ANSI builds. As we did with the service.

Even for this part of the code, I suspect ansi build is broken -- we
never test it,
do we.


Selva
Gert Doering May 28, 2021, 11:44 p.m. UTC | #4
Hi,

On Thu, May 27, 2021 at 11:40:59AM -0400, Selva Nair wrote:
> > (I do wonder if "support ansi builds" is really something we want, if
> > that makes the code so complicated - "always UNICODE" seems to be much
> > simpler.  What is the drawback of not supporting "ansi" builds?)
> 
> I prefer to write all new Windows code as unicode without any dependency on
> the defines UNICODE and  _UNICODE --- i.e no TCHARs, explicit API calls,
> no _tprintfs.
> 
> For old code, an easy option would be to retain the TCHARs, but assume
> we only build with UNICODE (TCHAR=WCHAR) and going forward don't care
> to support ANSI builds. As we did with the service.

I'm not the person who writes and debugs most of the windows code, I just
see patches pass by, and some bits are "more readable" and others less
so :-)

So my "vote" would be towards what you propose - always build unicode,
and for older code, assume (or force-#define) UNICODE.

Plus, document this somewhere.  On our styleguide page, maybe?

gert
Selva Nair May 30, 2021, 10:12 a.m. UTC | #5
Hi

On Sat, May 29, 2021 at 5:44 AM Gert Doering <gert@greenie.muc.de> wrote:
>
> Hi,
>
> On Thu, May 27, 2021 at 11:40:59AM -0400, Selva Nair wrote:
> > > (I do wonder if "support ansi builds" is really something we want, if
> > > that makes the code so complicated - "always UNICODE" seems to be much
> > > simpler.  What is the drawback of not supporting "ansi" builds?)
> >
> > I prefer to write all new Windows code as unicode without any dependency on
> > the defines UNICODE and  _UNICODE --- i.e no TCHARs, explicit API calls,
> > no _tprintfs.
> >
> > For old code, an easy option would be to retain the TCHARs, but assume
> > we only build with UNICODE (TCHAR=WCHAR) and going forward don't care
> > to support ANSI builds. As we did with the service.
>
> I'm not the person who writes and debugs most of the windows code, I just
> see patches pass by, and some bits are "more readable" and others less
> so :-)
>
> So my "vote" would be towards what you propose - always build unicode,
> and for older code, assume (or force-#define) UNICODE.
>
> Plus, document this somewhere.  On our styleguide page, maybe?

I have added a section named "Windows specific" to the codestyle in
the wiki. Not very polished -- please take a look and feel free to
pull it apart.

https://community.openvpn.net/openvpn/wiki/CodeStyle#Windowsspecific

Selva
Gert Doering July 13, 2021, 10:36 p.m. UTC | #6
Hi,

On Sun, May 30, 2021 at 04:12:37PM -0400, Selva Nair wrote:
> > So my "vote" would be towards what you propose - always build unicode,
> > and for older code, assume (or force-#define) UNICODE.
> >
> > Plus, document this somewhere.  On our styleguide page, maybe?
> 
> I have added a section named "Windows specific" to the codestyle in
> the wiki. Not very polished -- please take a look and feel free to
> pull it apart.
> 
> https://community.openvpn.net/openvpn/wiki/CodeStyle#Windowsspecific

This looks good to me.  Short and concise.  (One typo - WHCAR - fixed).

Lev, anything else we might want to add there?

gert

Patch

diff --git a/src/openvpnmsica/openvpnmsica.c b/src/openvpnmsica/openvpnmsica.c
index 96652117..5eb2f3c8 100644
--- a/src/openvpnmsica/openvpnmsica.c
+++ b/src/openvpnmsica/openvpnmsica.c
@@ -108,7 +108,8 @@  _debug_popup(_In_z_ LPCTSTR szFunctionName)
 
     /* Compose pop-up title. The dialog title will contain function name to ease the process
      * locating. Mind that Visual Studio displays window titles on the process list. */
-    _stprintf_s(szTitle, _countof(szTitle), TEXT("%s v%s"), szFunctionName, TEXT(PACKAGE_VERSION));
+    _stprintf_s(szTitle, _countof(szTitle), TEXT("%") TEXT(PRIsLPTSTR) TEXT("v%") TEXT(PRIsLPTSTR),
+                szFunctionName, TEXT(PACKAGE_VERSION));
 
     /* Get process name. */
     GetModuleFileName(NULL, szProcessPath, _countof(szProcessPath));
@@ -118,7 +119,8 @@  _debug_popup(_In_z_ LPCTSTR szFunctionName)
     /* Compose the pop-up message. */
     _stprintf_s(
         szMessage, _countof(szMessage),
-        TEXT("The %s process (PID: %u) has started to execute the %s custom action.\r\n")
+        TEXT("The %") TEXT(PRIsLPTSTR) TEXT(" process (PID: %u) has started to execute the %")
+        TEXT(PRIsLPTSTR) TEXT(" custom action.\r\n")
         TEXT("\r\n")
         TEXT("If you would like to debug the custom action, attach a debugger to this process and set breakpoints before dismissing this dialog.\r\n")
         TEXT("\r\n")
diff --git a/src/tapctl/main.c b/src/tapctl/main.c
index 3350bf1f..81addaef 100644
--- a/src/tapctl/main.c
+++ b/src/tapctl/main.c
@@ -49,7 +49,7 @@  const TCHAR title_string[] =
 ;
 
 static const TCHAR usage_message[] =
-    TEXT("%s\n")
+    TEXT("%") TEXT(PRIsLPTSTR) TEXT("\n")
     TEXT("\n")
     TEXT("Usage:\n")
     TEXT("\n")
@@ -66,7 +66,7 @@  static const TCHAR usage_message[] =
 ;
 
 static const TCHAR usage_message_create[] =
-    TEXT("%s\n")
+    TEXT("%") TEXT(PRIsLPTSTR) TEXT("\n")
     TEXT("\n")
     TEXT("Creates a new TUN/TAP adapter\n")
     TEXT("\n")
@@ -91,7 +91,7 @@  static const TCHAR usage_message_create[] =
 ;
 
 static const TCHAR usage_message_list[] =
-    TEXT("%s\n")
+    TEXT("%") TEXT(PRIsLPTSTR) TEXT("\n")
     TEXT("\n")
     TEXT("Lists TUN/TAP adapters\n")
     TEXT("\n")
@@ -110,7 +110,7 @@  static const TCHAR usage_message_list[] =
 ;
 
 static const TCHAR usage_message_delete[] =
-    TEXT("%s\n")
+    TEXT("%") TEXT(PRIsLPTSTR) TEXT("\n")
     TEXT("\n")
     TEXT("Deletes the specified network adapter\n")
     TEXT("\n")
@@ -170,7 +170,8 @@  _tmain(int argc, LPCTSTR argv[])
         }
         else
         {
-            _ftprintf(stderr, TEXT("Unknown command \"%s\". Please, use \"tapctl help\" to list supported commands.\n"), argv[2]);
+            _ftprintf(stderr, TEXT("Unknown command \"%") TEXT(PRIsLPTSTR)
+                      TEXT("\". Please, use \"tapctl help\" to list supported commands.\n"), argv[2]);
         }
 
         return 1;
@@ -194,7 +195,9 @@  _tmain(int argc, LPCTSTR argv[])
             }
             else
             {
-                _ftprintf(stderr, TEXT("Unknown option \"%s\". Please, use \"tapctl help create\" to list supported options. Ignored.\n"), argv[i]);
+                _ftprintf(stderr, TEXT("Unknown option \"%") TEXT(PRIsLPTSTR)
+                          TEXT("\". Please, use \"tapctl help create\" to list supported options. Ignored.\n"),
+                          argv[i]);
             }
         }
 
@@ -230,7 +233,8 @@  _tmain(int argc, LPCTSTR argv[])
                 if (_tcsicmp(szName, pAdapter->szName) == 0)
                 {
                     StringFromIID((REFIID)&pAdapter->guid, &szAdapterId);
-                    _ftprintf(stderr, TEXT("Adapter \"%s\" already exists (GUID %") TEXT(PRIsLPOLESTR) TEXT(").\n"), pAdapter->szName, 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;
                 }
@@ -241,7 +245,9 @@  _tmain(int argc, LPCTSTR argv[])
             if (dwResult != ERROR_SUCCESS)
             {
                 StringFromIID((REFIID)&guidAdapter, &szAdapterId);
-                _ftprintf(stderr, TEXT("Renaming TUN/TAP adapter %") TEXT(PRIsLPOLESTR) TEXT(" to \"%s\" failed (error 0x%x).\n"), szAdapterId, szName, dwResult);
+                _ftprintf(stderr, TEXT("Renaming TUN/TAP adapter %") TEXT(PRIsLPOLESTR)
+                          TEXT(" to \"%") TEXT(PRIsLPTSTR) TEXT("\" failed (error 0x%x).\n"),
+                          szAdapterId, szName, dwResult);
                 CoTaskMemFree(szAdapterId);
                 iResult = 1; goto quit;
             }
@@ -289,7 +295,9 @@  create_delete_adapter:
             }
             else
             {
-                _ftprintf(stderr, TEXT("Unknown option \"%s\". Please, use \"tapctl help list\" to list supported options. Ignored.\n"), argv[i]);
+                _ftprintf(stderr, TEXT("Unknown option \"%") TEXT(PRIsLPTSTR)
+                          TEXT("\". Please, use \"tapctl help list\" to list supported options. Ignored.\n"),
+                          argv[i]);
             }
         }
 
@@ -306,7 +314,8 @@  create_delete_adapter:
         {
             LPOLESTR szAdapterId = NULL;
             StringFromIID((REFIID)&pAdapter->guid, &szAdapterId);
-            _ftprintf(stdout, TEXT("%") TEXT(PRIsLPOLESTR) TEXT("\t%") TEXT(PRIsLPTSTR) TEXT("\n"), szAdapterId, pAdapter->szName);
+            _ftprintf(stdout, TEXT("%") TEXT(PRIsLPOLESTR) TEXT("\t%")
+                      TEXT(PRIsLPTSTR) TEXT("\n"), szAdapterId, pAdapter->szName);
             CoTaskMemFree(szAdapterId);
         }
 
@@ -337,7 +346,7 @@  create_delete_adapter:
             {
                 if (pAdapter == NULL)
                 {
-                    _ftprintf(stderr, TEXT("\"%s\" adapter not found.\n"), argv[2]);
+                    _ftprintf(stderr, TEXT("\"%") TEXT(PRIsLPTSTR) TEXT("\" adapter not found.\n"), argv[2]);
                     iResult = 1; goto delete_cleanup_pAdapterList;
                 }
                 else if (_tcsicmp(argv[2], pAdapter->szName) == 0)
@@ -364,7 +373,8 @@  delete_cleanup_pAdapterList:
             &bRebootRequired);
         if (dwResult != ERROR_SUCCESS)
         {
-            _ftprintf(stderr, TEXT("Deleting adapter \"%s\" failed (error 0x%x).\n"), argv[2], dwResult);
+            _ftprintf(stderr, TEXT("Deleting adapter \"%") TEXT(PRIsLPTSTR)
+                      TEXT("\" failed (error 0x%x).\n"), argv[2], dwResult);
             iResult = 1; goto quit;
         }
 
@@ -372,7 +382,8 @@  delete_cleanup_pAdapterList:
     }
     else
     {
-        _ftprintf(stderr, TEXT("Unknown command \"%s\". Please, use \"tapctl help\" to list supported commands.\n"), argv[1]);
+        _ftprintf(stderr, TEXT("Unknown command \"%") TEXT(PRIsLPTSTR)
+                  TEXT("\". Please, use \"tapctl help\" to list supported commands.\n"), argv[1]);
         return 1;
     }
 
@@ -434,7 +445,7 @@  x_msg_va(const unsigned int flags, const char *format, va_list arglist)
             }
 
             /* Output error message. */
-            _ftprintf(stderr, TEXT("Error 0x%x: %s\n"), dwResult, szErrMessage);
+            _ftprintf(stderr, TEXT("Error 0x%x: %") TEXT(PRIsLPTSTR) TEXT("\n"), dwResult, szErrMessage);
 
             LocalFree(szErrMessage);
         }
diff --git a/src/tapctl/tap.c b/src/tapctl/tap.c
index 563c07f6..b8d63b0f 100644
--- a/src/tapctl/tap.c
+++ b/src/tapctl/tap.c
@@ -1117,7 +1117,8 @@  tap_set_adapter_name(
     }
 
     /* rename adapter via netsh call */
-    const TCHAR* szFmt = _T("netsh interface set interface name=\"%s\" newname=\"%s\"");
+    const TCHAR* szFmt = TEXT("netsh interface set interface name=\"%")
+                         TEXT(PRIsLPTSTR) TEXT("\" newname=\"%") TEXT(PRIsLPTSTR) TEXT("\"");
     size_t ncmdline = _tcslen(szFmt) + _tcslen(szOldName) + _tcslen(szName) + 1;
     WCHAR* szCmdLine = malloc(ncmdline * sizeof(TCHAR));
     _stprintf_s(szCmdLine, ncmdline, szFmt, szOldName, szName);