Move away from openvpn_basename() over to platform provided basename()

This kicks out the openvpn_basename() function from misc.[ch] andputs it into compat.[ch]. This is to provide the same functionality on platforms not having a native basename() function available.

In addition this patch adds dirname() which commit 0f2bc0dd92f43c91e depends. Without dirname(), openvpn won't build in Visual Studio.

---
 Makefile.am  |    1 +
 compat.c     |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 compat.h     |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 configure.ac |    2 +-
 init.c       |    6 ++++--
 misc.c       |   23 +++--------------------
 misc.h       |    3 ---
 options.c    |    1 -
 syshead.h    |    2 ++
 9 files changed, 126 insertions(+), 27 deletions(-)
 create mode 100644 compat.c
 create mode 100644 compat.h

index 075270f..437f939 100644
@@ -84,6 +84,7 @@ openvpn_SOURCES = \
 	circ_list.h \
 	clinat.c clinat.h \
 	common.h \
+	compat.h compat.c \
 	config-win32.h \
 	crypto.c crypto.h crypto_backend.h \
 	crypto_openssl.c crypto_openssl.h \

index a4d68e6..173d6e2 100644
@@ -515,7 +515,7 @@ AC_CHECK_FUNCS(daemon chroot getpwnam setuid nice system getpid dup dup2 dnl
 	       getpass strerror syslog openlog mlockall getgrnam setgid dnl
 	       setgroups stat flock readv writev time dnl
 	       setsid chdir putenv getpeername unlink dnl
-	       chsize ftruncate execve getpeereid umask)
+	       chsize ftruncate execve getpeereid umask basename dirname)
 
 # Windows use stdcall for winsock so we cannot auto detect these
 m4_define([SOCKET_FUNCS], [socket recv recvfrom send sendto listen dnl

index 82c1000..368adb4 100644
@@ -862,8 +862,10 @@ init_verb_mute (struct context *c, unsigned int flags)
 void
 init_options_dev (struct options *options)
 {
-  if (!options->dev)
-    options->dev = openvpn_basename (options->dev_node);
+  if (!options->dev && options->dev_node) {
+    char *dev_node = strdup(options->dev_node); /* POSIX basename() implementaions may modify its arguments */
+    options->dev = basename (dev_node);
+  }
 }
 
 bool

index 99e5bc5..aec12b8 100644
@@ -2022,13 +2022,15 @@ argv_extract_cmd_name (const char *path)
 {
   if (path)
     {
-      const char *bn = openvpn_basename (path);
+      char *path_cp = strdup(path); /* POSIX basename() implementaions may modify its arguments */
+      const char *bn = basename (path_cp);
       if (bn)
 	{
 	  char *ret = string_alloc (bn, NULL);
 	  char *dot = strrchr (ret, '.');
 	  if (dot)
 	    *dot = '\0';
+	  free(path_cp);
 	  if (ret[0] != '\0')
 	    return ret;
 	}

@@ -2370,25 +2372,6 @@ argv_test (void)
 }
 #endif
 
-const char *
-openvpn_basename (const char *path)
-{
-  const char *ret;
-  const int dirsep = OS_SPECIFIC_DIRSEP;
-
-  if (path)
-    {
-      ret = strrchr (path, dirsep);
-      if (ret && *ret)
-	++ret;
-      else
-	ret = path;
-      if (*ret)
-	return ret;
-    }
-  return NULL;
-}
-
 /*
  * Remove security-sensitive strings from control message
  * so that they will not be output to log file.

index b2e6f91..b1badd9 100644
@@ -381,9 +381,6 @@ extern int script_method; /* GLOBAL */
 /* return the next largest power of 2 */
 size_t adjust_power_of_2 (size_t u);
 
-/* return the basename of path */
-const char *openvpn_basename (const char *path);
-
 /*
  * A printf-like function (that only recognizes a subset of standard printf
  * format operators) that prints arguments to an argv list instead

index 3a819c9..2280e9b 100644
@@ -53,7 +53,6 @@
 #include "forward.h"
 #include <ctype.h>
 #include <unistd.h>
-#include <libgen.h>
 
 #include "memdbg.h"
 

index f3c0ac9..020b81f 100644
@@ -36,6 +36,8 @@
 #include "config.h"
 #endif
 
+#include "compat.h"
+
 /* branch prediction hints */
 #if defined(__GNUC__)
 # define likely(x)       __builtin_expect((x),1)

1.7.4.4

Hello David,

The idea of compat.[hc] is to have system compatible layer. There should be no openvpn specific code in these. Just alternate implementation of missing functionality. It should be at the end of the chain in term of linkage.

So the "openvpn_dirbasename" should ether be copied to this file and declared as static or provide a different simpler approach, (strrchr('\\') or strrchr('/')).

Regards,
Alon.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Alon,

I'm not sure I understand the critique too well now. As this is the approach I tried to do. However, I put the basename() and dirname() implementations in compat.h, calling the combined openvpn_dirbasename() function.

The reason for the combined version is basically to keep as much generic code as possible. And the old openvpn_basename() has been tested and tried for many years, so that code base can be considered solid. As you know, the difference between dirname() and basename() is basically if you return the string from the beginning to the split point, or from the split point and to the end of the string.

But I can move the static inline functions from the compat.h file over to the compat.c file, and declare openvpn_dirbasename() static instead. If that is a better solution.

kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk7gkQ0ACgkQDC186MBRfrrK7wCfZUZbCF+8bJXApFpOipFsJ4xt Tz0AoJH8GJV/XyirtQklBzYKOHOAXD7o
=XiV3
-----END PGP SIGNATURE-----

Yes, this what I meant... :)

Best not to put code in headers, unless absolutely required... when? if you prove by profiler
that you can optimize anything... Most likely you won't call basename in 1000000000 loop.

However, you should be very careful from relying of the "solid" openvpn_dirbasename
as it won't be used at most systems... as most have basename/dirname...

This is the code from glibc, which most probably be linked into your code, the only difference
in windows it to check both '/' and '\'.... I like to use '/' also in windows. But if you don't like it
we can do AC_DEFINE([PATH_SEPARATOR], ...) in configure.ac, if not already there,
then use PATH_SEPARATOR constant in code.
---
char *
basename (filename)

     const char *filename;

{
char *p = strrchr (filename, '/');
return p ? p + 1 : (char *) filename;
}

char *
dirname (char *path)
{
static const char dot[] = ".";
char *last_slash;

  /* Find last '/'.  */
  last_slash = path != NULL ? strrchr (path, '/') : NULL;

  if (last_slash != NULL && last_slash != path && last_slash[1] == '\0')
    {
      /* Determine whether all remaining characters are slashes.  */
      char *runp;

      for (runp = last_slash; runp != path; --runp)
        if (runp[-1] != '/')
          break;

      /* The '/' is the last character, we have to look further.  */
      if (runp != path)
        last_slash = __memrchr (path, '/', runp - path);
    }

if (last_slash != NULL)

    {
      /* Determine whether all remaining characters are slashes.  */
      char *runp;

      for (runp = last_slash; runp != path; --runp)
        if (runp[-1] != '/')
          break;

      /* Terminate the path.  */
      if (runp == path)
        {
          /* The last slash is the first character in the string.  We have to
             return "/".  As a special case we have to return "//" if there
             are exactly two slashes at the beginning of the string.  See
             XBD 4.10 Path Name Resolution for more information.  */
          if (last_slash == path + 1)
            ++last_slash;
          else
            last_slash = path + 1;
        }
      else
        last_slash = runp;

      last_slash[0] = '\0';
    }

else
/* This assignment is ill-designed but the XPG specs require to
return a string containing "." in any case no directory part is found and so a static and constant string is required. */
path = (char *) dot;

return path;
}
---

Alon.

Hi,

Agree with this in principle, but...

[..]

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

This kicks out the openvpn_basename() function from misc.[ch] andputs it into compat.[ch]. This is to provide the same functionality on platforms not having a native basename() function available.

In addition this patch adds dirname() which commit 0f2bc0dd92f43c91e depends. Without dirname(), openvpn won't build in Visual Studio.

v2: Move all functions from compat.h to compat.c

---
 Makefile.am  |    1 +
 compat.c     |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 compat.h     |   42 +++++++++++++++++++++++++++++
 configure.ac |    2 +-
 init.c       |    6 +++-
 misc.c       |   23 ++--------------
 misc.h       |    3 --
 options.c    |    1 -
 syshead.h    |    2 +
 9 files changed, 135 insertions(+), 27 deletions(-)
 create mode 100644 compat.c
 create mode 100644 compat.h

index 075270f..437f939 100644
@@ -84,6 +84,7 @@ openvpn_SOURCES = \
 	circ_list.h \
 	clinat.c clinat.h \
 	common.h \
+	compat.h compat.c \
 	config-win32.h \
 	crypto.c crypto.h crypto_backend.h \
 	crypto_openssl.c crypto_openssl.h \

index a4d68e6..173d6e2 100644
@@ -515,7 +515,7 @@ AC_CHECK_FUNCS(daemon chroot getpwnam setuid nice system getpid dup dup2 dnl
 	       getpass strerror syslog openlog mlockall getgrnam setgid dnl
 	       setgroups stat flock readv writev time dnl
 	       setsid chdir putenv getpeername unlink dnl
-	       chsize ftruncate execve getpeereid umask)
+	       chsize ftruncate execve getpeereid umask basename dirname)
 
 # Windows use stdcall for winsock so we cannot auto detect these
 m4_define([SOCKET_FUNCS], [socket recv recvfrom send sendto listen dnl

index 82c1000..368adb4 100644
@@ -862,8 +862,10 @@ init_verb_mute (struct context *c, unsigned int flags)
 void
 init_options_dev (struct options *options)
 {
-  if (!options->dev)
-    options->dev = openvpn_basename (options->dev_node);
+  if (!options->dev && options->dev_node) {
+    char *dev_node = strdup(options->dev_node); /* POSIX basename() implementaions may modify its arguments */
+    options->dev = basename (dev_node);
+  }
 }
 
 bool

index 99e5bc5..aec12b8 100644
@@ -2022,13 +2022,15 @@ argv_extract_cmd_name (const char *path)
 {
   if (path)
     {
-      const char *bn = openvpn_basename (path);
+      char *path_cp = strdup(path); /* POSIX basename() implementaions may modify its arguments */
+      const char *bn = basename (path_cp);
       if (bn)
 	{
 	  char *ret = string_alloc (bn, NULL);
 	  char *dot = strrchr (ret, '.');
 	  if (dot)
 	    *dot = '\0';
+	  free(path_cp);
 	  if (ret[0] != '\0')
 	    return ret;
 	}

@@ -2370,25 +2372,6 @@ argv_test (void)
 }
 #endif
 
-const char *
-openvpn_basename (const char *path)
-{
-  const char *ret;
-  const int dirsep = OS_SPECIFIC_DIRSEP;
-
-  if (path)
-    {
-      ret = strrchr (path, dirsep);
-      if (ret && *ret)
-	++ret;
-      else
-	ret = path;
-      if (*ret)
-	return ret;
-    }
-  return NULL;
-}
-
 /*
  * Remove security-sensitive strings from control message
  * so that they will not be output to log file.

index b2e6f91..b1badd9 100644
@@ -381,9 +381,6 @@ extern int script_method; /* GLOBAL */
 /* return the next largest power of 2 */
 size_t adjust_power_of_2 (size_t u);
 
-/* return the basename of path */
-const char *openvpn_basename (const char *path);
-
 /*
  * A printf-like function (that only recognizes a subset of standard printf
  * format operators) that prints arguments to an argv list instead

index 3a819c9..2280e9b 100644
@@ -53,7 +53,6 @@
 #include "forward.h"
 #include <ctype.h>
 #include <unistd.h>
-#include <libgen.h>
 
 #include "memdbg.h"
 

index f3c0ac9..020b81f 100644
@@ -36,6 +36,8 @@
 #include "config.h"
 #endif
 
+#include "compat.h"
+
 /* branch prediction hints */
 #if defined(__GNUC__)
 # define likely(x)       __builtin_expect((x),1)

1.7.4.4

This kicks out the openvpn_basename() function from misc.[ch] andputs it into compat.[ch]. This is to provide the same functionality on platforms not having a native basename() function available.

In addition this patch adds dirname() which commit 0f2bc0dd92f43c91e depends. Without dirname(), openvpn won't build in Visual Studio.

v2: Move all functions from compat.h to compat.c

---
 Makefile.am  |    1 +
 compat.c     |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 compat.h     |   42 +++++++++++++++++++++++++++++
 configure.ac |    2 +-
 init.c       |    6 +++-
 misc.c       |   23 ++--------------
 misc.h       |    3 --
 options.c    |    1 -
 syshead.h    |    2 +
 9 files changed, 135 insertions(+), 27 deletions(-)
 create mode 100644 compat.c
 create mode 100644 compat.h

index 075270f..437f939 100644
@@ -84,6 +84,7 @@ openvpn_SOURCES = \
 	circ_list.h \
 	clinat.c clinat.h \
 	common.h \
+	compat.h compat.c \
 	config-win32.h \
 	crypto.c crypto.h crypto_backend.h \
 	crypto_openssl.c crypto_openssl.h \

index a4d68e6..173d6e2 100644
@@ -515,7 +515,7 @@ AC_CHECK_FUNCS(daemon chroot getpwnam setuid nice system getpid dup dup2 dnl
 	       getpass strerror syslog openlog mlockall getgrnam setgid dnl
 	       setgroups stat flock readv writev time dnl
 	       setsid chdir putenv getpeername unlink dnl
-	       chsize ftruncate execve getpeereid umask)
+	       chsize ftruncate execve getpeereid umask basename dirname)
 
 # Windows use stdcall for winsock so we cannot auto detect these
 m4_define([SOCKET_FUNCS], [socket recv recvfrom send sendto listen dnl

index 82c1000..368adb4 100644
@@ -862,8 +862,10 @@ init_verb_mute (struct context *c, unsigned int flags)
 void
 init_options_dev (struct options *options)
 {
-  if (!options->dev)
-    options->dev = openvpn_basename (options->dev_node);
+  if (!options->dev && options->dev_node) {
+    char *dev_node = strdup(options->dev_node); /* POSIX basename() implementaions may modify its arguments */
+    options->dev = basename (dev_node);
+  }
 }
 
 bool

index 99e5bc5..aec12b8 100644
@@ -2022,13 +2022,15 @@ argv_extract_cmd_name (const char *path)
 {
   if (path)
     {
-      const char *bn = openvpn_basename (path);
+      char *path_cp = strdup(path); /* POSIX basename() implementaions may modify its arguments */
+      const char *bn = basename (path_cp);
       if (bn)
 	{
 	  char *ret = string_alloc (bn, NULL);
 	  char *dot = strrchr (ret, '.');
 	  if (dot)
 	    *dot = '\0';
+	  free(path_cp);
 	  if (ret[0] != '\0')
 	    return ret;
 	}

@@ -2370,25 +2372,6 @@ argv_test (void)
 }
 #endif
 
-const char *
-openvpn_basename (const char *path)
-{
-  const char *ret;
-  const int dirsep = OS_SPECIFIC_DIRSEP;
-
-  if (path)
-    {
-      ret = strrchr (path, dirsep);
-      if (ret && *ret)
-	++ret;
-      else
-	ret = path;
-      if (*ret)
-	return ret;
-    }
-  return NULL;
-}
-
 /*
  * Remove security-sensitive strings from control message
  * so that they will not be output to log file.

index b2e6f91..b1badd9 100644
@@ -381,9 +381,6 @@ extern int script_method; /* GLOBAL */
 /* return the next largest power of 2 */
 size_t adjust_power_of_2 (size_t u);
 
-/* return the basename of path */
-const char *openvpn_basename (const char *path);
-
 /*
  * A printf-like function (that only recognizes a subset of standard printf
  * format operators) that prints arguments to an argv list instead

index 3a819c9..2280e9b 100644
@@ -53,7 +53,6 @@
 #include "forward.h"
 #include <ctype.h>
 #include <unistd.h>
-#include <libgen.h>
 
 #include "memdbg.h"
 

index f3c0ac9..020b81f 100644
@@ -36,6 +36,8 @@
 #include "config.h"
 #endif
 
+#include "compat.h"
+
 /* branch prediction hints */
 #if defined(__GNUC__)
 # define likely(x)       __builtin_expect((x),1)

1.7.4.4

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On second thought, after having discussed things with Gert on IRC, I'm withdrawing this version. I'll make use of the glibc variants Alon pointed me at as well.

kind regards,

David Sommerseth

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk7g1kIACgkQDC186MBRfrqD1QCeLiJP7Vbsf9Gy1YZqF1OU1A7i xAMAoKk8nDWe1+yuPa+Gj1VGbJuU/s6y
=tCIC
-----END PGP SIGNATURE-----

This kicks out the openvpn_basename() function from misc.[ch] and puts glibc equivalents into compat.[ch]. This is to provide the same functionality on platforms not having a native basename() function available.

In addition this patch adds dirname() which commit 0f2bc0dd92f43c91e depends. Without dirname(), openvpn won't build in Visual Studio.

v2: Move all functions from compat.h to compat.c
v3: Use glibc versions of basename() and dirname() instead

---
 Makefile.am  |    1 +
 compat.c     |  129 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 compat.h     |   42 +++++++++++++++++++
 configure.ac |    2 +-
 init.c       |    6 ++-
 misc.c       |   23 +---------
 misc.h       |    3 -
 options.c    |    1 -
 syshead.h    |    2 +
 9 files changed, 182 insertions(+), 27 deletions(-)
 create mode 100644 compat.c
 create mode 100644 compat.h

index 075270f..437f939 100644
@@ -84,6 +84,7 @@ openvpn_SOURCES = \
 	circ_list.h \
 	clinat.c clinat.h \
 	common.h \
+	compat.h compat.c \
 	config-win32.h \
 	crypto.c crypto.h crypto_backend.h \
 	crypto_openssl.c crypto_openssl.h \

index a4d68e6..173d6e2 100644
@@ -515,7 +515,7 @@ AC_CHECK_FUNCS(daemon chroot getpwnam setuid nice system getpid dup dup2 dnl
 	       getpass strerror syslog openlog mlockall getgrnam setgid dnl
 	       setgroups stat flock readv writev time dnl
 	       setsid chdir putenv getpeername unlink dnl
-	       chsize ftruncate execve getpeereid umask)
+	       chsize ftruncate execve getpeereid umask basename dirname)
 
 # Windows use stdcall for winsock so we cannot auto detect these
 m4_define([SOCKET_FUNCS], [socket recv recvfrom send sendto listen dnl

index 82c1000..368adb4 100644
@@ -862,8 +862,10 @@ init_verb_mute (struct context *c, unsigned int flags)
 void
 init_options_dev (struct options *options)
 {
-  if (!options->dev)
-    options->dev = openvpn_basename (options->dev_node);
+  if (!options->dev && options->dev_node) {
+    char *dev_node = strdup(options->dev_node); /* POSIX basename() implementaions may modify its arguments */
+    options->dev = basename (dev_node);
+  }
 }
 
 bool

index 99e5bc5..aec12b8 100644
@@ -2022,13 +2022,15 @@ argv_extract_cmd_name (const char *path)
 {
   if (path)
     {
-      const char *bn = openvpn_basename (path);
+      char *path_cp = strdup(path); /* POSIX basename() implementaions may modify its arguments */
+      const char *bn = basename (path_cp);
       if (bn)
 	{
 	  char *ret = string_alloc (bn, NULL);
 	  char *dot = strrchr (ret, '.');
 	  if (dot)
 	    *dot = '\0';
+	  free(path_cp);
 	  if (ret[0] != '\0')
 	    return ret;
 	}

@@ -2370,25 +2372,6 @@ argv_test (void)
 }
 #endif
 
-const char *
-openvpn_basename (const char *path)
-{
-  const char *ret;
-  const int dirsep = OS_SPECIFIC_DIRSEP;
-
-  if (path)
-    {
-      ret = strrchr (path, dirsep);
-      if (ret && *ret)
-	++ret;
-      else
-	ret = path;
-      if (*ret)
-	return ret;
-    }
-  return NULL;
-}
-
 /*
  * Remove security-sensitive strings from control message
  * so that they will not be output to log file.

index b2e6f91..b1badd9 100644
@@ -381,9 +381,6 @@ extern int script_method; /* GLOBAL */
 /* return the next largest power of 2 */
 size_t adjust_power_of_2 (size_t u);
 
-/* return the basename of path */
-const char *openvpn_basename (const char *path);
-
 /*
  * A printf-like function (that only recognizes a subset of standard printf
  * format operators) that prints arguments to an argv list instead

index 3a819c9..2280e9b 100644
@@ -53,7 +53,6 @@
 #include "forward.h"
 #include <ctype.h>
 #include <unistd.h>
-#include <libgen.h>
 
 #include "memdbg.h"
 

index f3c0ac9..020b81f 100644
@@ -36,6 +36,8 @@
 #include "config.h"
 #endif
 
+#include "compat.h"
+
 /* branch prediction hints */
 #if defined(__GNUC__)
 # define likely(x)       __builtin_expect((x),1)

1.7.4.4

Yes, this is valid compat now :)

Alon.

Hi,

ACK for the dirname() part.

I'm not particularily happy with the basename() changes, though.

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

(resent - go get it into the mailing list too)

For the record, I did some valgrind checks on this change. And for some odd reason, this doesn't leak. Even though --dev-node is explicitly set.
But it might be that the gc_* magic functions somehow catches this
allocation as well.

Also note that init_options_dev() is only called once during the execution, and this allocation *only* happens if --dev-node is used.

And the only reason I say this is to try to avoid posting a v4 patch ;-)

The original openvpn_basename() is based upon the GNU basename() which is not modifying any arguments, so this is definitely cleaner.

If there's a vote for a v4 version of this patch, I would then move this one out of compat.c and back to misc.c ... and add the code which tackles both / and \ in the same code.

*BUT* this will cause a nasty feature breakage, with tackling both / and \. As this will then work everywhere openvpn_basename() is used. But wherever dirname() is used in the code, it will be platform defined / *or* \ ... or in case of lack of dirname() it will use both (Windows).

Which again, in my head, speaks for openvpn_dirname() as well ...

However, Windows is the platform which first of all needs to tackle both / and \, as it is possible to use both variants, afaik.

David S.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk7iKJsACgkQDC186MBRfrq0VQCcC8P5Um4RkZbXwGH4ENMbJugs MIkAn2ta/cC7ufmiSEMO1JOYBW1L0Jp0
=C+WB
-----END PGP SIGNATURE-----

In my opinion, in order to avoid confusion there can be two options:

1. Use library functions as much as possible, providing the minimum common ground
in compatibility layer if missing. This ensure applying fixes and functionality from
various upstreams.

2. Use own implementation everywhere, taking responsibility and control over the implementation
and maintenance.

Mix is only confusing, and leads to even more errors.

This also applied to related functions such as dirname/basename in this case, choosing own
implementation of basename yield confusing as you do not expect that dirname taken from
different implementation.

Regards,
Alon.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

I've been pondering on this a lot today as well, after also having had an IRC chat with Gert. And I'm leaning towards what Alon says here. Even though, in 1) the drawback here is that compat functions will be less tested than in 2).

However, in this case, the base/dirname functions are taken straight out of glibc, and should have been more than well tested. The code has also not been modified in several years. This speaks for alternative 1).

I'm also seriously concerned about the / and \ parsing, as that will, naturally, be different from *nix and Windows platforms. But the current implementation in compat.[ch] tackles both automatically. So if the code base doesn't have its own base/dirname functions, a more flexible and gracious feature will be available. Otherwise, you're dependent on the libc library the running platform gives you. And this speaks for alternative 2).

I agree that mixing is unfortunate, however, we need to draw a line somewhere. Otherwise we're ending up implementing a unified libc, blending all supported platforms. We can't do that, that's just insane.
So we need to fix what's needed to fix. Which speaks more for 1).

Then there's the next problem. Vague specifications and different platforms interpreting standards and specifications differently. Sometimes you really wonder what POSIX people were smoking when deciding on standards. And base/dirname() is one of those ... the vast majority of man pages I've read lately states this:

Both dirname() and basename() may modify the contents of path, so it may be desirable to pass a copy when calling one of these functions.

These functions may return pointers to statically allocated
memory which may be overwritten by subsequent calls. Alter‐
natively, they may return a pointer to some part of path, so that the string referred to by path should not be modified or freed until the pointer returned by the function is no longer required.

How the he** is it possible for a developer to predict what different platforms will do, when these functions *may* modify the input or *may* or may not return pointers to static memory buffers? (They had to be smoking /something/). Anyway, this speaks in favour of 2).

And we can go on like this forever and ever and ever ... it will always be one argument which kills the other, both ways. So the conclusion? We really need to draw a line _somewhere_.

So ... Since the rest of the world are able to tackle such ambiguities like base/dirname() already, so should we. Seriously. We're a small project (in active people), but with a broad range of users - and amazingly many users and use cases. OpenVPN basically runs on everything which can run Linux - which means from simple embedded systems to powerful servers, and more. And as we're not that many developers involved, having less code to carry is way better in my eyes. Copying compat code from safe code bases, like glibc, will then not be such a big issue for those exceptions we might hit - the maintenance burden should be somewhat lower.

To be able to support the lower end, code size matters. Reducing the number of code we ship, reduces the footprint OpenVPN takes. In this case, it's not much at all, but in other cases this might be way bigger.
This matters more for me right now. The more we can depend on external
code, the cleaner our code can become. But we need a compat approach to support the broadness of our platforms. And then the needed compat code only is activated where absolutely needed.

My conclusion is that we need to choose 1) here. So that's my pragmatic long-term point of view on this.

In regards to this current patch. I have done valgrind tests, I even tried to free() the strdup()ed string in init_options_dev() ... and glibc complained instantly about double freed memory. So somehow, there are some clever memory management involved. Which seems to be covered by the gc_*() calls on the options buffer. I therefore don't see any immediate risks here, but I also don't say it is fail-safe as I don't see how this gc_*() magic really works. But initial tests looks very promising.

I will refrain from implementing the current patch in the current shape until after the weekend, so that more people may have their say. We may even ask James for an explicit opinion on this topic as well. But for now, I consider the patch ACKed (accepted) by Alon and NACKed (rejected) by Gert.

We haven't had any democratic system on the ACKs earlier, and I don't have any strong reasons to start with that now. But unless there are more people NACKing this code, it will be applied as-is during next week.

Any objections?

kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk7idHgACgkQDC186MBRfrqtMgCfRiLjhNgx5gTifGHOpX3/SV85 HLAAnRFBO2+B94ABOG8fskxKEaAI4FPy
=3HIB
-----END PGP SIGNATURE-----

Hi,

cutting off most of this long mail, and pointing to a few - IMHO - important aspects.

On Fri, Dec 09, 2011 at 09:50:02PM +0100, David Sommerseth wrote:
[..]

[..]

Keep that in mind...

This is why I opt for "use our own openvpn_basename()", based on the (very short) glibc implementation. Why? Because to cope with this messy specification, the caller of libc basename() has to jump through a number of hoops, which your v3 patch doesn't completely do yet:

- strdup() the string before calling basename() (in case the implementation modifies the string)

- strdup() the *returned* string (in case the next call to basename() overwrites the static buffer that we've just received a pointer to, thus garbling a string that we might need to use at some unknown point in time late ron)

- free() the first strdup()

- eventually free() the second strdup()

[..]

Not in this case. The actual basename() implementation has less footprint than the extra hoops required to *savely* call any possible POSIX-compliant basename() implementation.

[..]

Well, the valgrind tests are nice to know *what happens if using glibc*, but we already know that it's basename() implementation is really sane - it doesn't tell us anything about basename()'s with static buffers, etc.

I'm a bit surprised regarding the "double free" - that is something that this code and the implementation of basename() in glibc shouldn't be able to trigger - it will free() memory that other pointers still point to (so valgrind is happy, as there are still references), but it's not a *double* free.

This would very much surprise me, as strdup() isn't gc_*() aware.

[..]

Let me point you to your own code size argument :-) - the extra code to make POSIX basename() safe to use in all the useless-but-permitted cases is more than carrying a sane implementation, as it is now...

(Now for dirname(), things are different, as it's not used for store-and- use-later strings, so the "static buffer" thing is no risk here, and we know that most implementations will modify the path name passed, and have to cope with it anyway)

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Hi,

In this two either-or extremes, this doesn't make sense.

Of course nobody wants to reimplement all library functions (except djb, maybe, but we do not want to go there), but given some functions with sufficiently vague or messy calling semantics, having a better defined local implementation can improvide the code a lot.

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

I'm sorry if this is injecting noise into the conversation but I cannot resist commenting.

I don't know the answer, but I do know the question: Improve the code when? The old adage is "standard is better than better" comes to mind. If the code is calling basename all the time then it makes sense to improve it. If we're talking about saving a few microseconds here or there on startup or shutdown I'd question whether yet another interface, better or not, makes sense. Likewise, if there's a problem that needs to be fixed, say, people are introducing bugs, then by all means it needs fixing. Otherwise, if it ain't broke....

Regards,

Karl <[email protected]...>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Applied to master branches on -testing and -stable

commit ec302f7061b7ab4dd21bdac77dd115e75b50cbc0

Author: David Sommerseth <[email protected]...>
Date:   Thu Dec 8 00:40:45 2011 +0100

    Signed-off-by: David Sommerseth <[email protected]...>
    Acked-by: Alon Bar-Lev <[email protected]...>
    URL:

<http://thread.gmane.org/gmane.network.openvpn.devel/5178/focus=5215>

kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk8Mh0UACgkQDC186MBRfrpzKACZAbyca0UVkxkdtwBn5OF6bqEx CB4AnA7tpFZsvACpTjHkTYr7HTUCNiGw
=qewL
-----END PGP SIGNATURE-----