Merge branch 'soranoba/fix-file-position/PR-646' into maint

* soranoba/fix-file-position/PR-646:
  Unify internal error handling
  Fix file:pread and :pwrite to use character encoding
  Clean up code for file:position/2
  Fix file:position (not raw mode)

OTP-13155

2 files changed, 139 insertions, 47 deletions

diff --git a/lib/kernel/src/file_io_server.erl b/lib/kernel/src/file_io_server.erl
index bf8b9e2747..deb7b315b1 100644
--- a/lib/kernel/src/file_io_server.erl
+++ b/lib/kernel/src/file_io_server.erl
@@ -1,7 +1,7 @@
 %%
 %% %CopyrightBegin%
 %%
-%% Copyright Ericsson AB 2000-2013. All Rights Reserved.
+%% Copyright Ericsson AB 2000-2015. All Rights Reserved.
 %%
 %% Licensed under the Apache License, Version 2.0 (the "License");
 %% you may not use this file except in compliance with the License.
@@ -206,8 +206,8 @@ io_reply(From, ReplyAs, Reply) ->
 file_request({advise,Offset,Length,Advise},
          #state{handle=Handle}=State) ->
     case ?PRIM_FILE:advise(Handle, Offset, Length, Advise) of
-    {error,_}=Reply ->
-        {stop,normal,Reply,State};
+    {error,Reason}=Reply ->
+        {stop,Reason,Reply,State};
     Reply ->
         {reply,Reply,State}
     end;
@@ -215,62 +215,91 @@ file_request({allocate, Offset, Length},
          #state{handle = Handle} = State) ->
     Reply = ?PRIM_FILE:allocate(Handle, Offset, Length),
     {reply, Reply, State};
+file_request({pread,At,Sz}, State)
+  when At =:= cur;
+       At =:= {cur,0} ->
+    case get_chars(Sz, latin1, State) of
+	{reply,Reply,NewState}
+	  when is_list(Reply);
+	       is_binary(Reply) ->
+	    {reply,{ok,Reply},NewState};
+	Other ->
+	    Other
+    end;
 file_request({pread,At,Sz}, 
-	     #state{handle=Handle,buf=Buf,read_mode=ReadMode}=State) ->
+	     #state{handle=Handle,buf=Buf}=State) ->
     case position(Handle, At, Buf) of
-	{ok,_Offs} ->
-	    case ?PRIM_FILE:read(Handle, Sz) of
-		{ok,Bin} when ReadMode =:= list ->
-		    std_reply({ok,binary_to_list(Bin)}, State);
-		Reply ->
-		    std_reply(Reply, State)
-	    end;
-	Reply ->
-	    std_reply(Reply, State)
+	{error,_} = Reply ->
+	    {error,Reply,State};
+	_ ->
+	    case get_chars(Sz, latin1, State#state{buf= <<>>}) of
+		{reply,Reply,NewState}
+		  when is_list(Reply);
+		       is_binary(Reply) ->
+		    {reply,{ok,Reply},NewState};
+		Other ->
+		    Other
+	    end
     end;
+file_request({pwrite,At,Data},
+	     #state{buf= <<>>}=State)
+  when At =:= cur;
+       At =:= {cur,0} ->
+    put_chars(Data, latin1, State);
 file_request({pwrite,At,Data}, 
 	     #state{handle=Handle,buf=Buf}=State) ->
     case position(Handle, At, Buf) of
-	{ok,_Offs} ->
-	    std_reply(?PRIM_FILE:write(Handle, Data), State);
-	Reply ->
-	    std_reply(Reply, State)
+	{error,_} = Reply ->
+	    {error,Reply,State};
+	_ ->
+	    put_chars(Data, latin1, State)
     end;
 file_request(datasync,
 	     #state{handle=Handle}=State) ->
     case ?PRIM_FILE:datasync(Handle) of
-	{error,_}=Reply ->
-	    {stop,normal,Reply,State};
+	{error,Reason}=Reply ->
+	    {stop,Reason,Reply,State};
 	Reply ->
 	    {reply,Reply,State}
     end;
 file_request(sync, 
 	     #state{handle=Handle}=State) ->
     case ?PRIM_FILE:sync(Handle) of
-	{error,_}=Reply ->
-	    {stop,normal,Reply,State};
+	{error,Reason}=Reply ->
+	    {stop,Reason,Reply,State};
 	Reply ->
 	    {reply,Reply,State}
     end;
 file_request(close, 
 	     #state{handle=Handle}=State) ->
-    {stop,normal,?PRIM_FILE:close(Handle),State#state{buf= <<>>}};
+    case ?PRIM_FILE:close(Handle) of
+	{error,Reason}=Reply ->
+	    {stop,Reason,Reply,State#state{buf= <<>>}};
+	Reply ->
+	    {stop,normal,Reply,State#state{buf= <<>>}}
+    end;
 file_request({position,At}, 
 	     #state{handle=Handle,buf=Buf}=State) ->
-    std_reply(position(Handle, At, Buf), State);
+    case position(Handle, At, Buf) of
+	{error,_} = Reply ->
+	    {error,Reply,State};
+	Reply ->
+	    std_reply(Reply, State)
+    end;
 file_request(truncate, 
 	     #state{handle=Handle}=State) ->
     case ?PRIM_FILE:truncate(Handle) of
-	{error,_Reason}=Reply ->
-	    {stop,normal,Reply,State#state{buf= <<>>}};
+	{error,Reason}=Reply ->
+	    {stop,Reason,Reply,State#state{buf= <<>>}};
 	Reply ->
-	    {reply,Reply,State}
+	    std_reply(Reply, State)
     end;
 file_request(Unknown, 
 	     #state{}=State) ->
     Reason = {request, Unknown},
     {error,{error,Reason},State}.
 
+%% Standard reply and clear buffer
 std_reply({error,_}=Reply, State) ->
     {error,Reply,State#state{buf= <<>>}};
 std_reply(Reply, State) ->
@@ -286,8 +315,8 @@ io_request({put_chars, Enc, Chars},
 io_request({put_chars, Enc, Chars}, 
 	   #state{handle=Handle,buf=Buf}=State) ->
     case position(Handle, cur, Buf) of
-	{error,_}=Reply ->
-	    {stop,normal,Reply,State#state{buf= <<>>}};
+	{error,Reason}=Reply ->
+	    {stop,Reason,Reply,State};
 	_ ->
 	    put_chars(Chars, Enc, State#state{buf= <<>>})
     end;
@@ -368,23 +397,27 @@ io_request_loop([Request|Tail],
 %% I/O request put_chars
 %%
 put_chars(Chars, latin1, #state{handle=Handle, unic=latin1}=State) ->
+    NewState = State#state{buf = <<>>},
     case ?PRIM_FILE:write(Handle, Chars) of
-	{error,_}=Reply ->
-	    {stop,normal,Reply,State};
+	{error,Reason}=Reply ->
+	    {stop,Reason,Reply,NewState};
 	Reply ->
-	    {reply,Reply,State}
+	    {reply,Reply,NewState}
     end;
 put_chars(Chars, InEncoding, #state{handle=Handle, unic=OutEncoding}=State) ->
+    NewState = State#state{buf = <<>>},
     case unicode:characters_to_binary(Chars,InEncoding,OutEncoding) of
 	Bin when is_binary(Bin) ->
 	    case ?PRIM_FILE:write(Handle, Bin) of
-		{error,_}=Reply ->
-		    {stop,normal,Reply,State};
+		{error,Reason}=Reply ->
+		    {stop,Reason,Reply,NewState};
 		Reply ->
-		    {reply,Reply,State}
+		    {reply,Reply,NewState}
 	    end;
 	{error,_,_} ->
-	    {stop,normal,{error,{no_translation, InEncoding, OutEncoding}},State}
+	    {stop,no_translation,
+	     {error,{no_translation, InEncoding, OutEncoding}},
+	     NewState}
     end.
 
 get_line(S, {<<>>, Cont}, OutEnc,
@@ -884,11 +917,14 @@ cbv({utf32,little},_) ->
 
 %% Compensates ?PRIM_FILE:position/2 for the number of bytes 
 %% we have buffered
-
-position(Handle, cur, Buf) ->
-    position(Handle, {cur, 0}, Buf);
-position(Handle, {cur, Offs}, Buf) when is_binary(Buf) ->
-    ?PRIM_FILE:position(Handle, {cur, Offs-byte_size(Buf)});
-position(Handle, At, _Buf) ->
-    ?PRIM_FILE:position(Handle, At).
-
+position(Handle, At, Buf) ->
+    ?PRIM_FILE:position(
+       Handle,
+       case At of
+	   cur ->
+	       {cur, -byte_size(Buf)};
+	   {cur, Offs} ->
+	       {cur, Offs-byte_size(Buf)};
+	   _ ->
+	       At
+       end).
diff --git a/lib/kernel/test/file_SUITE.erl b/lib/kernel/test/file_SUITE.erl
index 8856be31c2..09d9a45197 100644
--- a/lib/kernel/test/file_SUITE.erl
+++ b/lib/kernel/test/file_SUITE.erl
@@ -1,7 +1,7 @@
 %%
 %% %CopyrightBegin%
 %%
-%% Copyright Ericsson AB 1996-2013. All Rights Reserved.
+%% Copyright Ericsson AB 1996-2015. All Rights Reserved.
 %%
 %% Licensed under the Apache License, Version 2.0 (the "License");
 %% you may not use this file except in compliance with the License.
@@ -48,7 +48,7 @@
 -export([cur_dir_0/1, cur_dir_1/1, make_del_dir/1,
 	 list_dir/1,list_dir_error/1,
 	 untranslatable_names/1, untranslatable_names_error/1,
-	 pos1/1, pos2/1]).
+	 pos1/1, pos2/1, pos3/1]).
 -export([close/1, consult1/1, path_consult/1, delete/1]).
 -export([ eval1/1, path_eval/1, script1/1, path_script/1,
 	 open1/1,
@@ -80,6 +80,7 @@
 
 -export([interleaved_read_write/1]).
 
+-export([unicode/1]).
 -export([altname/1]).
 
 -export([large_file/1, large_write/1]).
@@ -114,7 +115,7 @@
 suite() -> [{ct_hooks,[ts_install_cth]}].
 
 all() -> 
-    [altname, read_write_file, {group, dirs},
+    [unicode, altname, read_write_file, {group, dirs},
      {group, files}, delete, rename, names, {group, errors},
      {group, compression}, {group, links}, copy,
      delayed_write, read_ahead, segment_read, segment_write,
@@ -136,7 +137,7 @@ groups() ->
       [open1, old_modes, new_modes, path_open, close, access,
        read_write, pread_write, append, open_errors,
        exclusive]},
-     {pos, [], [pos1, pos2]},
+     {pos, [], [pos1, pos2, pos3]},
      {file_info, [],
       [file_info_basic_file, file_info_basic_directory,
        file_info_bad, file_info_times, file_write_file_info]},
@@ -1370,6 +1371,27 @@ pos2(Config) when is_list(Config) ->
     ?line test_server:timetrap_cancel(Dog),
     ok.
 
+pos3(suite) -> [];
+pos3(doc) -> ["When it does not use raw mode, file:position had a bug."];
+pos3(Config) when is_list(Config) ->
+    ?line Dog = test_server:timetrap(test_server:seconds(5)),
+    ?line RootDir = ?config(data_dir, Config),
+    ?line Name = filename:join(RootDir, "realmen.html.gz"),
+
+    ?line {ok, Fd} = ?FILE_MODULE:open(Name, [read, binary]),
+    ?line {ok, _}  = ?FILE_MODULE:read(Fd, 5),
+    ?line {error, einval} = ?FILE_MODULE:position(Fd, {bof, -1}),
+
+    %% Here ok had returned =(
+    ?line {error, einval} = ?FILE_MODULE:position(Fd, {cur, -10}),
+    %% That test is actually questionable since file:position/2
+    %% is documented to leave the file position undefined after
+    %% it has returned an error.  But on Posix systems the position
+    %% is guaranteed to be unchanged after an error return.  On e.g
+    %% Windows there is nothing stated about this in the documentation.
+
+    ?line test_server:timetrap_cancel(Dog),
+    ok.
 
 file_info_basic_file(suite) -> [];
 file_info_basic_file(doc) -> [];
@@ -2761,6 +2783,40 @@ compress_async_crash_loop(N, Path, ExpectedData) ->
 
 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
 
+unicode(Config) when is_list(Config) ->
+    Dir = ?config(priv_dir, Config),
+    Name = filename:join(Dir, "data-utf8.txt"),
+    Txt = lists:seq(128, 255),
+    D = unicode:characters_to_binary(Txt, latin1, latin1),
+    {ok,Fd1} =
+	?FILE_MODULE:open(Name, [write,read,binary,{encoding,unicode}]),
+    ok = ?FILE_MODULE:truncate(Fd1),
+    ok = ?FILE_MODULE:write(Fd1, Txt),
+    {ok,0} = ?FILE_MODULE:position(Fd1, bof),
+    {ok,D} = ?FILE_MODULE:read(Fd1, 129),
+    {ok,0} = ?FILE_MODULE:position(Fd1, bof),
+    {ok,D1} = ?FILE_MODULE:read(Fd1, 64),
+    {ok,Pos} = ?FILE_MODULE:position(Fd1, cur),
+    {ok,D2} = ?FILE_MODULE:pread(Fd1, {cur,0}, 65),
+    D = <<D1/binary, D2/binary>>,
+    {ok,D1} = ?FILE_MODULE:pread(Fd1, bof, 64),
+    {ok,Pos} = ?FILE_MODULE:position(Fd1, Pos),
+    {ok,D2} = ?FILE_MODULE:read(Fd1, 64),
+    ok = ?FILE_MODULE:close(Fd1),
+    %%
+    RawD = unicode:characters_to_binary(Txt, latin1, unicode),
+    {ok,RawD} = ?FILE_MODULE:read_file(Name),
+    %%
+    {ok,Fd2} = ?FILE_MODULE:open(Name, [read,{encoding,unicode}]),
+    {ok,Txt} = ?FILE_MODULE:read(Fd2, 129),
+    {Txt1,Txt2} = lists:split(64, Txt),
+    {ok,Txt2} = ?FILE_MODULE:pread(Fd2, Pos, 65),
+    {ok,0} = ?FILE_MODULE:position(Fd2, bof),
+    {ok,Txt1} = ?FILE_MODULE:read(Fd2, 64),
+    ok = ?FILE_MODULE:close(Fd2).
+
+%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
+
 altname(doc) ->
     "Test the file:altname/1 function";
 altname(suite) ->