MPR#6120: Windows symlinks and corrected stat implementation

[dra27]

See also the discussion in Mantis

Implements Unix.readlink and Unix.symlink for Windows. Windows symbolic links are a little, both in their implementation and availability - the weirdness of both is documented in unix.mli both in Unix.symlink and the new Unix.has_symlink function.

Microsoft's implementation of stat is, politely put, flaky and there is no implementation of lstat. The stat functions are therefore completely re-implemented - though the algorithm for the Microsoft implementation is maintained in various places (e.g. setting the execute bit). The new implementation populates st_dev using the device's serial number rather than a simulated value and also populates st_ino (critical for ocamlbuild to be able to use the new symbolic link support)

[dbuenzli]

Here you should add that {!has_symlink} can be used to check that the process is able to create symbolic links.

[dra27]

Will do!

[damiendoligez]

You need to set the correct author here. Is this mostly copied from Xavier's code, or is it substantially new code?

[dra27]

Ah, OK - no, all the code's mine, beyond lifting the GC boilerplate macros from the UNIX version (which is how the header remained). I'll push amendments in a few mins...

[damiendoligez]

There is a lot of new code in this PR. Have you signed the CLA already? See #342 (comment) for the procedure.

I'm settting the milestone to 4.04 because changes in unix.mli are likely to break some libraries.

[dra27]

Ok, I'll get a CLA pinged to Xavier (will probably be tomorrow as I won't be near a scanner for today). I thought from the comment thread in Mantis that this was approved for 4.03 and just awaiting the fstat change (the patch has been in Mantis for a while) - is it definitely too late for 4.03? It will be useful as it enables support for symlinks in ocamlbuild which eases building for certain libraries in OPAM (but the functionality can of course be back-ported there, it needs be)

[alainfrisch]

because changes in unix.mli are likely to break some libraries.

I'm not sure about the "likely". Except taking the module type of Unix (but this same argument would argue against any change to the stdlib), or passing symlink as a first-class function, how could the changes break existing code? If you are really concerned by the addition of the optional argument, one could probably remove the optional argument for 4.03 (using the default heuristic only).

[dbuenzli]

FWIW I would also very like to have this in 4.03.

[damiendoligez]

I'm sorry, I forgot to look at the Mantis discussion before I set the milestone. Since we have several Windows stakeholders pushing for inclusion, let's take the risk.

[damiendoligez]

I'll approve as soon as we have the CLA and you fix the author (and date) in both file headers.

[dra27]

OK - given that I've almost totally rewritten stat.c, should the header in that be updated too?

[alainfrisch]

@damiendoligez Correct me if I'm wrong, but people contributing code through a CLA should not only update the code attribution but also the copyright holder. Is that right?

[damiendoligez]

@dra27 Yes, you'll have to update the header in stat.c. And @alainfrisch is correct, you need to put the year and your name in the sentence that starts with Copyright.

[damiendoligez]

@dra27 This is one of the last 3 things before I branch 4.03, so it would be good if you fixed the headers ASAP. We'll be fine if we have the CLA by Friday or even next Monday.

[dra27]

Sorry - had to pause for a few hours to do the day job! Should manage this all later this evening...

[dra27]

Trunk seems to be broken for Windows again, not sure how technically correct this is (given that Windows has FLAMBDA=false in config/Makefile), but I patched trunk thusly for the purposes of testing this:

diff --git a/Makefile.nt b/Makefile.nt
index cda7459..02d8847 100644
--- a/Makefile.nt
+++ b/Makefile.nt
@@ -295,6 +295,14 @@ ocamlc: compilerlibs/ocamlcommon.cma compilerlibs/ocamlbytecomp.cma $(BYTESTART)
 partialclean::
        rm -f ocamlc

+# The middle end (whose .cma library is currently only used for linking
+# the "objinfo" program, since we cannot depend on the whole native code
+# compiler for "make world" and the list of dependencies for
+# asmcomp/export_info.cmo is long).
+
+compilerlibs/ocamlmiddleend.cma: $(MIDDLE_END)
+       $(CAMLC) -a -o $@ $(MIDDLE_END)
+
 # The native-code compiler

 compilerlibs/ocamloptcomp.cma: $(MIDDLE_END) $(ASMCOMP)
@@ -606,7 +614,7 @@ clean::
 # Tools

 ocamltools: ocamlc ocamlyacc ocamllex asmcomp/cmx_format.cmi \
-            asmcomp/printclambda.cmo
+            asmcomp/printclambda.cmo compilerlibs/ocamlmiddleend.cma
        cd tools ; $(MAKEREC) all

 ocamltoolsopt:

[dra27]

Headers fixed. The only other change, apart from a trunk-rebase, was to remove an obsolete branch of code the algorithm of which was taken from Microsoft's implementation of stat (their old implementation had somewhat weird semantics for UNC and drive roots - e.g. \server\share or C:\ but the new implementation with GetFileInformationByHandle doesn't require such trickery.

For ease of review of the copyright headers, here's the diff:

diff --git a/otherlibs/win32unix/readlink.c b/otherlibs/win32unix/readlink.c
index ad080c5..1bb9347 100644
--- a/otherlibs/win32unix/readlink.c
+++ b/otherlibs/win32unix/readlink.c
@@ -2,12 +2,12 @@
 /*                                                                     */
 /*                                OCaml                                */
 /*                                                                     */
-/*            Xavier Leroy, projet Cristal, INRIA Rocquencourt         */
+/*               David Allsopp, MetaStack Solutions Ltd.               */
 /*                                                                     */
-/*  Copyright 1996 Institut National de Recherche en Informatique et   */
-/*  en Automatique.  All rights reserved.  This file is distributed    */
-/*  under the terms of the GNU Library General Public License, with    */
-/*  the special exception on linking described in file ../../LICENSE.  */
+/*  Copyright 2015 MetaStack Solutions Ltd.  All rights reserved.      */
+/*  This file is distributed under the terms of the GNU Library        */
+/*  General Public License, with the special exception on linking      */
+/*  described in file ../../LICENSE.                                   */
 /*                                                                     */
 /***********************************************************************/

diff --git a/otherlibs/win32unix/stat.c b/otherlibs/win32unix/stat.c
index 432cf21..ed854b8 100644
--- a/otherlibs/win32unix/stat.c
+++ b/otherlibs/win32unix/stat.c
@@ -2,12 +2,12 @@
 /*                                                                     */
 /*                                OCaml                                */
 /*                                                                     */
-/*            Xavier Leroy, projet Cristal, INRIA Rocquencourt         */
+/*               David Allsopp, MetaStack Solutions Ltd.               */
 /*                                                                     */
-/*  Copyright 2002 Institut National de Recherche en Informatique et   */
-/*  en Automatique.  All rights reserved.  This file is distributed    */
-/*  under the terms of the GNU Library General Public License, with    */
-/*  the special exception on linking described in file ../../LICENSE.  */
+/*  Copyright 2015 MetaStack Solutions Ltd.  All rights reserved.      */
+/*  This file is distributed under the terms of the GNU Library        */
+/*  General Public License, with the special exception on linking      */
+/*  described in file ../../LICENSE.                                   */
 /*                                                                     */
 /***********************************************************************/

diff --git a/otherlibs/win32unix/symlink.c b/otherlibs/win32unix/symlink.c
index 37c6810..b684f34 100644
--- a/otherlibs/win32unix/symlink.c
+++ b/otherlibs/win32unix/symlink.c
@@ -2,12 +2,12 @@
 /*                                                                     */
 /*                                OCaml                                */
 /*                                                                     */
-/*            Xavier Leroy, projet Cristal, INRIA Rocquencourt         */
+/*               David Allsopp, MetaStack Solutions Ltd.               */
 /*                                                                     */
-/*  Copyright 1996 Institut National de Recherche en Informatique et   */
-/*  en Automatique.  All rights reserved.  This file is distributed    */
-/*  under the terms of the GNU Library General Public License, with    */
-/*  the special exception on linking described in file ../../LICENSE.  */
+/*  Copyright 2015 MetaStack Solutions Ltd.  All rights reserved.      */
+/*  This file is distributed under the terms of the GNU Library        */
+/*  General Public License, with the special exception on linking      */
+/*  described in file ../../LICENSE.                                   */
 /*                                                                     */
 /***********************************************************************/

[DemiMarie]

Was anything taken from the Microsoft CRT? That code is proprietary, and nothing based on it can be used in OCaml.

[dra27]

No - the old code in question was mine which allowed an earlier iteration of my do_stat to match the behaviour of Microsoft's (given that there are not many ways of coding 1+1, it was similar) in a particular corner case. Microsoft's freely available (if not libre) source code for the CRT was used to determine the algorithm for corner cases only.
For the avoidance of any copyright doubt, all of the code in this patch is mine with one exception: the REPARSE_DATA_BUFFER structure in otherlibs/win32unix/unixsupport.h which is taken from one of the mingw headers (also see the full structure definition in MSDN) for reasons documented in the comment above it.

[damiendoligez]

Thanks a lot!

[gasche]

I noticed while working on upgrading Batteries for 4.03 that there are some glitches in the unix.mli documentation; for example, readlink is still marked as unsupported on Windows, while I understand that @dra27 added support for it as part of this patch. Would it be possible for someone to fix it?

[damiendoligez]

readlink is still marked as unsupported on Windows

Fixed in 4.03 branch (525799b).

[fdopen]

Yes, I know it's late. I'm not sure, if anyone will read this, but.
In readlink/stat/symlink, the path is never copied to the c heap, before caml_enter_blocking_section is called, instead a simple pointer to the caml heap is used. Couldn't caml_enter_blocking_section trigger the garbage collector in case of multiple threads? Or do I miss something obvious?
e.g:

ocaml/otherlibs/win32unix/symlink.c

Line 50 in d86aa4c

result = pCreateSymbolicLink(String_val(dest), String_val(source), flags);

,

ocaml/otherlibs/win32unix/stat.c

Line 304 in d86aa4c

if (!do_stat(0, 0, String_val(path), caml_string_length(path), NULL, &st_ino, &buf)) {

,

ocaml/otherlibs/win32unix/stat.c

Line 156 in d86aa4c

h = CreateFile(path,

,

ocaml/otherlibs/win32unix/stat.c

Line 210 in d86aa4c

if ((h = CreateFile(path,

, etc.

[alainfrisch]

I believe you are right, and this should be fixed. Would you like to propose a patch?

[dra27]

Gah - good spot. In all those cases, there's no choice but duplicate the string, I guess. Is caml_enter_blocking_section a no-op when threading isn't in use, or can garbage collection take place between caml_enter_blocking_section...caml_leave_blocking_section even in single-threaded code?
There's at least one other instance, looking at https://github.com/ocaml/ocaml/blob/trunk/otherlibs/win32unix/sleep.c#L23-L25, I think?
Happy to put a patch together (it's my mess...!)

[xavierleroy]

@dra27: between enter_blocking_section/leave_blocking_section, both context switches and signal handling can take place, so you should assume that a GC can occur. Copying the Caml string is indeed necessary. The run-time cost of the copy is negligible compared with the cost of the system call.

For Unix.sleep, you're correct that Double_val(t) should be computed before caml_enter_blocking_section.

[damiendoligez]

I've opened a pair of Mantis issues to track these bugs:

• http://caml.inria.fr/mantis/view.php?id=7299
• http://caml.inria.fr/mantis/view.php?id=7300

[dra27]

@xavierleroy - thanks for the confirmation!
@damiendoligez - I'll submit PRs using those reference numbers. Should manage them both before I go on holiday next week (and therefore in time for 4.04?)