Re: [ovs-dev] [PATCH 1/4] util: Fix abs_file_name() bugs on Windows.

2018-08-03 Thread Alin Serdean 


On 3 Aug 2018, at 18:52, Ben Pfaff mailto:b...@ovn.org>> wrote:

On Fri, Aug 03, 2018 at 03:41:55PM +, Alin Serdean wrote:

abs_file_name() believed that a file name that begins with / or contains :
is absolute and that any other file name is relative.  On Windows, this is
wrong in at least the following ways:

  * / and \ are interchangeable on Windows.

  * A name that begins with \\ or // is also absolute.

  * A name that begins with X: but not X:\ is not absolute.

  * A name with : in some position other than the second position is
not absolute (although it might not be valid either?).

Furthermore, Windows has more than one current working directory (one
per volume letter), so trying to make a file name absolute by just prefixing
the current working directory for the current volume results in silliness.

This patch attempts to fix the problem.

Found by inspection.

CC: Alin Gabriel Serdean mailto:aserd...@ovn.org>>
Signed-off-by: Ben Pfaff mailto:b...@ovn.org>>
[Alin Serdean] Thanks a lot for the change Ben!
I was wondering if you can fold in the following so we can remove even more 
confusion:

That is nicer.  Thanks, I'll do that.

Are the changes to linker flags because PathIsRelative is in some
library not previously linked?

The changes are needed because PathIsRelative is included in shlwapi library 
which wasn’t previously included in the link step.

(https://docs.microsoft.com/en-us/windows/desktop/api/shlwapi/nf-shlwapi-pathisrelativea)

 Or are they independent changes?

Thanks,

Ben.

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/4] util: Fix abs_file_name() bugs on Windows.

2018-08-03 Thread Ben Pfaff 
On Fri, Aug 03, 2018 at 03:41:55PM +, Alin Serdean wrote:
> > 
> > abs_file_name() believed that a file name that begins with / or contains :
> > is absolute and that any other file name is relative.  On Windows, this is
> > wrong in at least the following ways:
> > 
> >* / and \ are interchangeable on Windows.
> > 
> >* A name that begins with \\ or // is also absolute.
> > 
> >* A name that begins with X: but not X:\ is not absolute.
> > 
> >* A name with : in some position other than the second position is
> >  not absolute (although it might not be valid either?).
> > 
> > Furthermore, Windows has more than one current working directory (one
> > per volume letter), so trying to make a file name absolute by just prefixing
> > the current working directory for the current volume results in silliness.
> > 
> > This patch attempts to fix the problem.
> > 
> > Found by inspection.
> > 
> > CC: Alin Gabriel Serdean 
> > Signed-off-by: Ben Pfaff 
> [Alin Serdean] Thanks a lot for the change Ben!
> I was wondering if you can fold in the following so we can remove even more 
> confusion:

That is nicer.  Thanks, I'll do that.

Are the changes to linker flags because PathIsRelative is in some
library not previously linked?  Or are they independent changes?

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/4] util: Fix abs_file_name() bugs on Windows.

2018-08-03 Thread Alin Serdean 
> 
> abs_file_name() believed that a file name that begins with / or contains :
> is absolute and that any other file name is relative.  On Windows, this is
> wrong in at least the following ways:
> 
>* / and \ are interchangeable on Windows.
> 
>* A name that begins with \\ or // is also absolute.
> 
>* A name that begins with X: but not X:\ is not absolute.
> 
>* A name with : in some position other than the second position is
>  not absolute (although it might not be valid either?).
> 
> Furthermore, Windows has more than one current working directory (one
> per volume letter), so trying to make a file name absolute by just prefixing
> the current working directory for the current volume results in silliness.
> 
> This patch attempts to fix the problem.
> 
> Found by inspection.
> 
> CC: Alin Gabriel Serdean 
> Signed-off-by: Ben Pfaff 
[Alin Serdean] Thanks a lot for the change Ben!
I was wondering if you can fold in the following so we can remove even more 
confusion:
diff --git a/lib/util.c b/lib/util.c
index d3c62988b..082bc7bb0 100644
--- a/lib/util.c
+++ b/lib/util.c
@@ -38,6 +38,9 @@
 #ifdef HAVE_PTHREAD_SET_NAME_NP
 #include 
 #endif
+#ifdef _WIN32
+#include 
+#endif

 VLOG_DEFINE_THIS_MODULE(util);

@@ -1053,9 +1056,8 @@ bool
 is_file_name_absolute(const char *fn)
 {
 #ifdef _WIN32
-/* An absolute path begins with X:\ or \\. */
-return ((fn[0] && fn[1] == ':' && strchr("/\\", fn[2]))
-|| (strchr("/\\", fn[0]) && strchr("/\\", fn[1])));
+/* Use platform specific API */
+return !PathIsRelative(fn);
 #else
 /* An absolute path begins with /. */
 return fn[0] == '/';

diff --git a/Documentation/intro/install/windows.rst 
b/Documentation/intro/install/windows.rst
index 6d8c746c5..f696d2c9b 100644
--- a/Documentation/intro/install/windows.rst
+++ b/Documentation/intro/install/windows.rst
@@ -156,7 +156,7 @@ component installation directories, etc. For example:
 ::

$ ./configure CC=./build-aux/cccl LD="$(which link)" \
-   LIBS="-lws2_32 -liphlpapi -lwbemuuid -lole32 -loleaut32" \
+   LIBS="-lws2_32 -lShlwapi -liphlpapi -lwbemuuid -lole32 -loleaut32" \
--prefix="C:/openvswitch/usr" \
--localstatedir="C:/openvswitch/var" \
--sysconfdir="C:/openvswitch/etc" \
@@ -172,7 +172,7 @@ To configure with SSL support, add the requisite additional 
options:
 ::

$ ./configure CC=./build-aux/cccl LD="`which link`"  \
-   LIBS="-lws2_32 -liphlpapi -lwbemuuid -lole32 -loleaut32" \
+   LIBS="-lws2_32 -lShlwapi -liphlpapi -lwbemuuid -lole32 -loleaut32" \
--prefix="C:/openvswitch/usr" \
--localstatedir="C:/openvswitch/var"
--sysconfdir="C:/openvswitch/etc" \
@@ -184,7 +184,7 @@ Finally, to the kernel module also:
 ::

$ ./configure CC=./build-aux/cccl LD="`which link`" \
-   LIBS="-lws2_32 -liphlpapi -lwbemuuid -lole32 -loleaut32" \
+   LIBS="-lws2_32 -lShlwapi -liphlpapi -lwbemuuid -lole32 -loleaut32" \
--prefix="C:/openvswitch/usr" \
--localstatedir="C:/openvswitch/var" \
--sysconfdir="C:/openvswitch/etc" \

diff --git a/appveyor.yml b/appveyor.yml
index 49fcae4b5..2e5c37a37 100644
--- a/appveyor.yml
+++ b/appveyor.yml
@@ -41,6 +41,6 @@ build_script:
 - C:\MinGW\msys\1.0\bin\bash -lc "cp 
/c/pthreads-win32/Pre-built.2/dll/x86/*.dll /c/openvswitch/."
 - C:\MinGW\msys\1.0\bin\bash -lc "mv /bin/link.exe /bin/link_copy.exe"
 - C:\MinGW\msys\1.0\bin\bash -lc "cd /c/openvswitch && ./boot.sh"
-- C:\MinGW\msys\1.0\bin\bash -lc "cd /c/openvswitch && ./configure 
CC=build-aux/cccl LD=\"`which link`\" LIBS=\"-lws2_32 -liphlpapi -lwbemuuid 
-lole32 -loleaut32\" --with-pthread=C:/pthreads-win32/Pre-built.2 
--with-openssl=C:/OpenSSL-Win32 --with-vstudiotarget=\"Debug\"
+- C:\MinGW\msys\1.0\bin\bash -lc "cd /c/openvswitch && ./configure 
CC=build-aux/cccl LD=\"`which link`\" LIBS=\"-lws2_32 -lShlwapi -liphlpapi 
-lwbemuuid -lole32 -loleaut32\" --with-pthread=C:/pthreads-win32/Pre-built.2 
--with-openssl=C:/OpenSSL-Win32 --with-vstudiotarget=\"Debug\"
 - C:\MinGW\msys\1.0\bin\bash -lc "cd /c/openvswitch && make"
 - C:\MinGW\msys\1.0\bin\bash -lc "cd /c/openvswitch && make 
datapath_windows_analyze"

Either way:
Acked-by: Alin Gabriel Serdean 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 1/4] util: Fix abs_file_name() bugs on Windows.

2018-07-24 Thread Ben Pfaff 
abs_file_name() believed that a file name that begins with / or contains :
is absolute and that any other file name is relative.  On Windows, this is
wrong in at least the following ways:

   * / and \ are interchangeable on Windows.

   * A name that begins with \\ or // is also absolute.

   * A name that begins with X: but not X:\ is not absolute.

   * A name with : in some position other than the second position is
 not absolute (although it might not be valid either?).

Furthermore, Windows has more than one current working directory (one per
volume letter), so trying to make a file name absolute by just prefixing
the current working directory for the current volume results in silliness.

This patch attempts to fix the problem.

Found by inspection.

CC: Alin Gabriel Serdean 
Signed-off-by: Ben Pfaff 
---
 lib/util.c | 58 +++---
 lib/util.h |  1 +
 2 files changed, 40 insertions(+), 19 deletions(-)

diff --git a/lib/util.c b/lib/util.c
index 7152b55392be..d3c62988bf83 100644
--- a/lib/util.c
+++ b/lib/util.c
@@ -1049,37 +1049,57 @@ base_name(const char *file_name)
 }
 #endif /* _WIN32 */
 
-/* If 'file_name' starts with '/', returns a copy of 'file_name'.  Otherwise,
+bool
+is_file_name_absolute(const char *fn)
+{
+#ifdef _WIN32
+/* An absolute path begins with X:\ or \\. */
+return ((fn[0] && fn[1] == ':' && strchr("/\\", fn[2]))
+|| (strchr("/\\", fn[0]) && strchr("/\\", fn[1])));
+#else
+/* An absolute path begins with /. */
+return fn[0] == '/';
+#endif
+}
+
+/* If 'file_name' is absolute, returns a copy of 'file_name'.  Otherwise,
  * returns an absolute path to 'file_name' considering it relative to 'dir',
  * which itself must be absolute.  'dir' may be null or the empty string, in
  * which case the current working directory is used.
  *
- * Additionally on Windows, if 'file_name' has a ':', returns a copy of
- * 'file_name'
- *
  * Returns a null pointer if 'dir' is null and getcwd() fails. */
 char *
 abs_file_name(const char *dir, const char *file_name)
 {
-if (file_name[0] == '/') {
+/* If it's already absolute, return a copy. */
+if (is_file_name_absolute(file_name)) {
 return xstrdup(file_name);
-#ifdef _WIN32
-} else if (strchr(file_name, ':')) {
-return xstrdup(file_name);
-#endif
-} else if (dir && dir[0]) {
+}
+
+/* If a base dir was supplied, use it.  We assume, without checking, that
+ * the base dir is absolute.*/
+if (dir && dir[0]) {
 char *separator = dir[strlen(dir) - 1] == '/' ? "" : "/";
 return xasprintf("%s%s%s", dir, separator, file_name);
-} else {
-char *cwd = get_cwd();
-if (cwd) {
-char *abs_name = xasprintf("%s/%s", cwd, file_name);
-free(cwd);
-return abs_name;
-} else {
-return NULL;
-}
 }
+
+#if _WIN32
+/* It's a little complicated to make an absolute path on Windows because a
+ * relative path might still specify a drive letter.  The OS has a function
+ * to do the job for us, so use it. */
+char abs_path[MAX_PATH];
+DWORD n = GetFullPathName(file_name, sizeof abs_path, abs_path, NULL);
+return n > 0 && n <= sizeof abs_path ? xmemdup0(abs_path, n) : NULL;
+#else
+/* Outside Windows, do the job ourselves. */
+char *cwd = get_cwd();
+if (!cwd) {
+return NULL;
+}
+char *abs_name = xasprintf("%s/%s", cwd, file_name);
+free(cwd);
+return abs_name;
+#endif
 }
 
 /* Like readlink(), but returns the link name as a null-terminated string in
diff --git a/lib/util.h b/lib/util.h
index 0061bb986520..fd414489c7fe 100644
--- a/lib/util.h
+++ b/lib/util.h
@@ -231,6 +231,7 @@ char *dir_name(const char *file_name);
 char *base_name(const char *file_name);
 #endif
 char *abs_file_name(const char *dir, const char *file_name);
+bool is_file_name_absolute(const char *);
 
 char *follow_symlinks(const char *filename);
 
-- 
2.16.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev