Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unix fsync #1839

Merged
merged 16 commits into from Jul 31, 2018

Conversation

Projects
None yet
10 participants
@UnixJunkie
Copy link
Contributor

commented Jun 15, 2018

No description provided.

@UnixJunkie

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2018

@UnixJunkie

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2018

I have no idea on how to do the windows part.
I hope a good will fill that bit.

@@ -182,6 +182,8 @@ type file_perm = int
external openfile : string -> open_flag list -> file_perm -> file_descr
= "unix_open"
external close : file_descr -> unit = "unix_close"
external fsync : file_descr -> unit =
invalid_arg "Unix.fsync not implemented"

This comment has been minimized.

Copy link
@yallop

yallop Jun 15, 2018

Member

externallet

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Jun 15, 2018

Contributor

Isn't there a compatibility equivalent on windows ? If not please indicate that in the API docs and in the manual table listing platform differences in Unix that that fsync is not implemented on windows.

@nojb

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2018

@UnixJunkie

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2018

added, thanks for the link, and thanks to @rwmjones for the code?

external close : file_descr -> unit = "unix_close"
external close : file_descr -> unit = "fsync"
external fsync : file_descr -> unit =
invalid_arg "Unix.fsync not implemented"

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Jun 15, 2018

Contributor

?????

This library is free software; you can redistribute it and/or
modify it under the terms of the GNU Lesser General Public
License as published by the Free Software Foundation; either
version 2.1 of the License, or (at your option) any later version.

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Jun 15, 2018

Contributor

This may not be compatible with the compiler's license (has no static linking exception).

This comment has been minimized.

Copy link
@dra27

dra27 Jun 15, 2018

Contributor

I can make that stronger - this code is not compatible with the compiler's contribution requirements. See https://github.com/ocaml/ocaml/blob/trunk/CONTRIBUTING.md#contributor-license-agreement. @rwmjones may be willing to contribute it to the compiler sources.

@nojb

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2018

The link was just for inspiration, to see how the function is called; you still need to write a stub function under Windows, with signature CAMLprim value unix_fsync(value fd).

@UnixJunkie

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2018

I don't have access to a windows computer, if you know better, feel free to improve the PR.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2018

@UnixJunkie - contributions are always welcome, so please don't take this in the wrong way. Contributions which move one platform forwards while leaving another behind necessarily create work and, in my opinion at least, are better worked on elsewhere - Discuss, caml-list and so forth can be used to draw developers in to help.

The Windows version of fsync.c will need adapting from that version you've used, as that's only been designed for mingw (i.e. GCC). There are various pointers which can get around your lack of a Windows machine:

  • I have blogged on setting up AppVeyor on one's own OCaml fork.
  • https://github.com/ocaml/opam/blob/master/appveyor.yml#L43-L46 contains a sample of how to get AppVeyor to pause at the end of a build (failed or otherwise) which allows you to connect to the build machine using an RDP client. Do not use this setting on OCaml's AppVeyor - please set it up on your own.
  • On a private branch, you can of course disable parts of the AppVeyor build so that you get to the bit of the build you want as quickly as possible (although msvc64 runs early in the process anyhow).
  • Surely your university provides Microsoft Imagine, and therefore easy and free access to a Windows 10 Licence which you could virtualise?
@nojb

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2018

I would like to emphasize @dra27's message. OCaml takes Windows compatibility seriously, and any improvement should ideally apply to all platforms (if at all possible).

With regards to this PR, I went ahead and pushed a commit with a possible implementation of fsync for Windows (FWIW, it is written from scratch without using any existing code).

@dra27

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2018

Reading up on MSDN's entry for FlushFileBuffers, I'm slightly concerned at the behaviour mentioned for pipes. fsync(2) should return EINVAL for a pipe, but according to MSDN, FlushFileBuffers will block until the pipe has been fully read.

It's belt-and-braces, as we don't support named pipes in the Unix library itself, but should the call be guarded with a call to GetFileType?

@nojb

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2018

I don't have a strong opinion either way - I will add the check.

@pmetzger

This comment has been minimized.

Copy link
Member

commented Jun 15, 2018

Side comment: I didn't even realize we lacked fsync, which is Very Very Important at times.

@dra27
Copy link
Contributor

left a comment

A few minor tweaks - and it needs a Changes entry.

/* */
/* OCaml */
/* */
/* Xavier Leroy, projet Cristal, INRIA Rocquencourt */

This comment has been minimized.

Copy link
@dra27

dra27 Jun 15, 2018

Contributor

This copyright header is wrong!

This comment has been minimized.

Copy link
@UnixJunkie

UnixJunkie Jun 18, 2018

Author Contributor

Where can I get a correct one then?

This comment has been minimized.

Copy link
@dra27

dra27 Jun 18, 2018

Contributor

Your name isn’t Xavier, you’re not at Inria, and I hope you haven’t been sitting on the patch for 22 years? 🙂

@@ -307,6 +307,9 @@ val openfile : string -> open_flag list -> file_perm -> file_descr
val close : file_descr -> unit
(** Close a file descriptor. *)

val fsync : file_descr -> unit
(** Synchronize a file's in-core state with the storage device. *)

This comment has been minimized.

Copy link
@dra27

dra27 Jun 15, 2018

Contributor

Does fsync ever cause anything to be read? Synchronize to me implies both directions: how about "Flushes all in-core data of the file associated with the file_descr to the storage device."?

This comment has been minimized.

Copy link
@dra27

dra27 Jun 15, 2018

Contributor

(I realise the man page for fsync begins by using the word synchronisation - it's just that it then goes on to describe flushing only AFAICT! FlushFileBuffers definitely does only what it says on the tin...)

This comment has been minimized.

Copy link
@pmetzger

pmetzger Jun 18, 2018

Member

The phrasing was taken straight from the summary line of the man page in *BSD and OS X.

I agree, though, that "flush in-memory file buffers to disk" or some such is better.

@@ -34,7 +34,7 @@ UNIX_FILES = access.c addrofstr.c chdir.c chmod.c cst2constr.c \
exit.c getaddrinfo.c getcwd.c gethost.c gethostname.c \
getnameinfo.c getproto.c \
getserv.c gmtime.c mmap_ba.c putenv.c rmdir.c \
socketaddr.c strofaddr.c time.c unlink.c
socketaddr.c strofaddr.c time.c unlink.c fsync.c

This comment has been minimized.

Copy link
@dra27

dra27 Jun 15, 2018

Contributor

This shouldn't be here.

This comment has been minimized.

Copy link
@UnixJunkie

UnixJunkie Jun 18, 2018

Author Contributor

where should it be then?

This comment has been minimized.

Copy link
@dra27

dra27 Jun 18, 2018

Contributor

Deleted. Files in this list are intended to be copied from the Unix version of the library.

This comment has been minimized.

Copy link
@UnixJunkie

UnixJunkie Jun 18, 2018

Author Contributor

edits from maintainers are allowed...

@UnixJunkie

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2018

Isn't _commit the equivalent of fsync on windows?

UnixJunkie added some commits Jul 9, 2018

@UnixJunkie

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2018

Maybe good to go.
I put @nojb as the first contributor, since he did the most work.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2018

You learn a new thing every morning - it does indeed look as though _commit is equivalent to fsync (a quick look online suggest Postgres just #ifdefs it)

The Windows code here is doing the same thing - @nojb, the code could be shared via fsync_os. What do you think?

@nojb

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2018

Yes, it looks equivalent to what we are doing - but indeed it would be better since we could share the Unix/Windows code. Will amend.

@nojb

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2018

I made a commit replacing FlushFileBuffers with _commit; @dra27 do you mind taking a look?

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

I'm not sure, but we might need a CLA for this contribution. (cc @xavierleroy)

@UnixJunkie

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

doesn't the windows code needs

#include <io.h>

just under

#ifdef _WIN32 

?

@nojb nojb force-pushed the UnixJunkie:unix_fsync branch from 2f9ee3e to 4ec5f22 Jul 10, 2018

@nojb

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2018

doesn't the windows code needs

Since the CI passes, I guess it gets included via some other header, but anyway I went ahead and added it for extra safety.

@UnixJunkie

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2018

I think this PR is ready to merge.
Thanks to all those who contributed!

@dra27

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

LGTM - the CLA question needs resolving before it can be squashed. You have left the copyright on fsync.c as being with Inria, which I presume you don't care about.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

The <io.h> thing is bothering me (mainly as to why it was working before - I can't see how that header was being included, at least just looking at the code).

@fdopen

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

mingw includes io.h through direct.h (included in unixsupport.h). msvc probably won't notify you, because the warning level is too low and _commit returns int (default return type).

@dra27

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

Thanks @fdopen!

@UnixJunkie

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2018

Yes, I wanted to leave the copyright to INRIA.

@UnixJunkie

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2018

@gasche I think it is ready to merge.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 31, 2018

I haven't participated in this discussion nor reviewed the code, I'll let other maintainers who did make decisions.

@nojb nojb merged commit 5b884d6 into ocaml:trunk Jul 31, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nojb

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2018

Merged on the basis of reviews by myself and @dra27.

@UnixJunkie

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2018

thanks!

@UnixJunkie UnixJunkie deleted the UnixJunkie:unix_fsync branch Jul 31, 2018


#ifdef _WIN32
#include <io.h>
#define fsync(fd) _commit(win_CRT_fd_of_filedescr(fd))

This comment has been minimized.

Copy link
@ygrek

ygrek Jul 31, 2018

Contributor

is this safe? fd is custom block and cannot be accessed outside of blocking section

This comment has been minimized.

Copy link
@nojb

nojb Jul 31, 2018

Contributor

Good catch, thanks! Fix in #1949.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.