[m-rev.] for review: io.make_temp now reterns a result rather than throw exceptions
Peter Wang
novalazy at gmail.com
Wed Apr 20 16:50:32 AEST 2016
On Wed, 20 Apr 2016 14:00:52 +1000, Paul Bone <paul at bone.id.au> wrote:
> On Wed, Apr 20, 2016 at 01:22:42PM +1000, Peter Wang wrote:
> > On Wed, 20 Apr 2016 12:44:38 +1000, Paul Bone <paul at bone.id.au> wrote:
> > > For review by Peter Wang who suggested this change.
> >
> > Hi Paul,
> >
> > Can you resend with diff -w? Some parts are hard to follow.
>
> Okay, I reflowed some things, especially comments, as indentation levels
> changed, so they'll still be part of the diff.
>
> From 6f03058105c31e75fdcb88e010ea50b427f304bd Mon Sep 17 00:00:00 2001
> From: Paul Bone <paul at bone.id.au>
> Date: Wed, 20 Apr 2016 12:23:23 +1000
> Subject: [PATCH] io.make_temp now reterns a result rather than throw
> exceptions
>
> The make_temp predicates would throw an exception if they were unsuccessful,
.
> New versions of make_temp/3 and make_temp/5 have been created (named
> make_temp_res) to avoid braking code. make_temp/3 and make_temp/5 have been
breaking
> marked as obsolete. make_temp_directory/3 and make_temp_directory/5 have
> been modified directly as they are very new. This closes bug #408
The new predicates can be called "make_temp_file" to parallel
make_temp_directory and avoid the artificial _res suffix.
>
> Some routines in compiler/compile_target_code.m used the name returned by
> make_temp and added a suffix before using the resulting file, this sould
could
> create unnecessary temporary files that I beleive were never cleaned up. So
> I've added a suffix argument to make_temp_res/5 and make_temp_directory/5
> (now they're make_temp_res/6 and make_temp_directory/6).
We could remove the weird "up to the first 5 characters of Prefix" thing
in the new predicates. The old make_temp could chop the Prefix argument
before forwarding to the new predicate, though I can't imagine anyone
needing that behaviour. You don't need to do it immediately.
> diff --git a/compiler/compile_target_code.m b/compiler/compile_target_code.m
> index 48453b4..ce11561 100644
> --- a/compiler/compile_target_code.m
> +++ b/compiler/compile_target_code.m
> @@ -1254,7 +1254,9 @@ invoke_mkinit(Globals, InitFileStream, Verbosity,
> % mkinit expects unquoted file names.
> join_string_list(FileNames, "", "\n", "", TargetFileNames),
>
> - io.make_temp(TmpFile, !IO),
> + io.make_temp_res(TmpFileResult, !IO),
> + (
> + TmpFileResult = ok(TmpFile),
> io.open_output(TmpFile, OpenResult, !IO),
> (
> OpenResult = ok(TmpStream),
> @@ -1274,7 +1276,15 @@ invoke_mkinit(Globals, InitFileStream, Verbosity,
> MkInitOK = no
> )
> ;
> - OpenResult = error(_),
> + OpenResult = error(Error),
> + io.format(io.stderr_stream, "%s: %s\n",
> + [s(TmpFile), s(error_message(Error))], !IO),
> + MkInitOK = no
> + )
> + ;
> + TmpFileResult = error(Error),
> + io.format(io.stderr_stream, "Could not create temporary file: %s\n",
> + [s(error_message(Error))], !IO),
> MkInitOK = no
> ).
I think invoke_mkinit should return the error to be presented by its
callers.
> @@ -1977,8 +1987,12 @@ link_exe_or_shared_lib(Globals, ErrorStream, LinkTargetType, ModuleName,
> % and thus overflow the command line, so in this case
> % we first create an archive of all of the object files.
> RestrictedCommandLine = yes,
> - io.make_temp(TmpFile, !IO),
> globals.lookup_string_option(Globals, library_extension, LibExt),
> + io.get_temp_directory(TempDir, !IO),
> + io.make_temp_res(TempDir, "", "." ++ LibExt, TmpFileResult, !IO),
> + (
> + TmpFileResult = ok(TmpFile),
> + % XXX: This is why my /tmp directory is always full of crap.
> TmpArchive = TmpFile ++ LibExt,
> % Only include actual object files in the temporary archive,
> % not other files such as other archives.
Delete the comment.
--restricted-command-line is only used on Windows so I doubt that's what
you're seeing. It is a bug, though.
> @@ -1990,6 +2004,15 @@ link_exe_or_shared_lib(Globals, ErrorStream, LinkTargetType, ModuleName,
> join_quoted_string_list([TmpArchive | NonObjectFiles],
> "", "", " ", Objects)
> ;
> + TmpFileResult = error(Error),
> + io.format(stderr_stream,
> + "Could not create temporary file: %s\n",
> + [s(error_message(Error))], !IO),
> + ArchiveSucceeded = no,
> + MaybeDeleteTmpArchive = no,
> + join_quoted_string_list(ObjectsList, "", "", " ", Objects)
> + )
> + ;
> RestrictedCommandLine = no,
> ArchiveSucceeded = yes,
> MaybeDeleteTmpArchive = no,
> @@ -3043,7 +3066,9 @@ create_java_exe_or_lib(Globals, ErrorStream, LinkTargetType, MainModuleName,
> % extremely long. We create the temporary file in the current directory to
> % avoid problems under Cygwin, where absolute paths will be interpreted
> % incorrectly when passed to a non-Cygwin jar program.
> - io.make_temp(".", "mtmp", TempFileName, !IO),
> + io.make_temp_res(".", "mtmp", "", TempFileNameResult, !IO),
> + (
> + TempFileNameResult = ok(TempFileName),
> io.open_output(TempFileName, OpenResult, !IO),
> (
> OpenResult = ok(Stream),
> @@ -3053,8 +3078,8 @@ create_java_exe_or_lib(Globals, ErrorStream, LinkTargetType, MainModuleName,
>
> Cmd = string.append_list(
> [Jar, " cf ", JarFileName, " @", TempFileName]),
> - invoke_system_command(Globals, ErrorStream, cmd_verbose_commands, Cmd,
> - Succeeded0, !IO),
> + invoke_system_command(Globals, ErrorStream,
> + cmd_verbose_commands, Cmd, Succeeded0, !IO),
> io.remove_file(TempFileName, _, !IO),
> (
> Succeeded0 = yes
> @@ -3064,9 +3089,14 @@ create_java_exe_or_lib(Globals, ErrorStream, LinkTargetType, MainModuleName,
> )
> ;
> OpenResult = error(Error),
> - io.error_message(Error, ErrorMsg),
> io.format(ErrorStream, "Error creating `%s': %s\n",
> - [s(TempFileName), s(ErrorMsg)], !IO),
> + [s(TempFileName), s(error_message(Error))], !IO),
> + Succeeded0 = no
> + )
> + ;
> + TempFileNameResult = error(Error),
> + io.format(ErrorStream, "Could not create temporary file: %s\n",
> + [s(error_message(Error))], !IO),
> Succeeded0 = no
> ),
> ( if
> @@ -3463,7 +3493,9 @@ invoke_long_system_command_maybe_filter_output(Globals, ErrorStream, Verbosity,
> RestrictedCommandLine = yes,
>
> % Avoid generating very long command lines by using @files.
> - io.make_temp(TmpFile, !IO),
> + io.make_temp_res(TmpFileResult, !IO),
> + (
> + TmpFileResult = ok(TmpFile),
> io.open_output(TmpFile, OpenResult, !IO),
> (
> OpenResult = ok(TmpStream),
> @@ -3504,7 +3536,15 @@ invoke_long_system_command_maybe_filter_output(Globals, ErrorStream, Verbosity,
> Succeeded = no
> )
> ;
> - OpenResult = error(_),
> + OpenResult = error(Error),
> + io.format(stderr_stream, "%s: %s\n",
> + [s(TmpFile), s(error_message(Error))], !IO),
> + Succeeded = no
> + )
> + ;
> + TmpFileResult = error(Error),
> + io.format(stderr_stream, "Could not create temporary file: %s\n",
> + [s(error_message(Error))], !IO),
> Succeeded = no
> )
> ;
> diff --git a/compiler/make.module_target.m b/compiler/make.module_target.m
> index 6d1f22f..0a9d037 100644
> --- a/compiler/make.module_target.m
> +++ b/compiler/make.module_target.m
> @@ -362,18 +362,30 @@ build_target(Globals, CompilationTask, TargetFile, Imports, TouchedTargetFiles,
> % We need a temporary file to pass the arguments to the mmc process
> % which will do the compilation. It is created here (not in invoke_mmc)
> % so it can be cleaned up by build_with_check_for_interrupt.
> - io.make_temp(ArgFileName, !IO),
> - MaybeArgFileName = yes(ArgFileName)
> + io.make_temp_res(ArgFileNameResult, !IO),
> + (
> + ArgFileNameResult = ok(ArgFileName),
> + MaybeArgFileName = yes(ArgFileName),
> + ArgFileNameSuccess = ok `with_type` io.res
> + ;
> + ArgFileNameResult = error(Error),
> + MaybeArgFileName = no,
> + ArgFileNameSuccess = error(Error)
> + )
> else
> - MaybeArgFileName = no
> + MaybeArgFileName = no,
> + ArgFileNameSuccess = ok
> ),
> - Cleanup =
> - ( pred(!.MakeInfo::in, !:MakeInfo::out, !.IO::di, !:IO::uo) is det :-
> +
> + (
> + ArgFileNameSuccess = ok,
It seems nicer to write this here instead of the with_type above:
ArgFileNameSuccess = ok : io.res,
> diff --git a/compiler/prog_event.m b/compiler/prog_event.m
> index 65a72c0..717c06c 100644
> --- a/compiler/prog_event.m
> +++ b/compiler/prog_event.m
> @@ -92,7 +92,9 @@ read_event_set(SpecsFileName, EventSetName, EventSpecMap, ErrorSpecs, !IO) :-
> % those tools are not yet mature enough. When they are, we should switch
> % to using them.
>
> - io.make_temp(TermFileName, !IO),
> + io.make_temp_res(TermFileNameResult, !IO),
> + (
> + TermFileNameResult = ok(TermFileName),
> read_specs_file(SpecsFileName, TermFileName, Problem, !IO),
> ( if Problem = "" then
> io.open_input(TermFileName, TermOpenRes, !IO),
> @@ -101,7 +103,8 @@ read_event_set(SpecsFileName, EventSetName, EventSpecMap, ErrorSpecs, !IO) :-
> io.read(TermStream, TermReadRes, !IO),
> (
> TermReadRes = ok(EventSetTerm),
> - EventSetTerm = event_set_spec(EventSetName, EventSpecsTerm),
> + EventSetTerm = event_set_spec(EventSetName,
> + EventSpecsTerm),
> convert_list_to_spec_map(SpecsFileName, EventSpecsTerm,
> map.init, EventSpecMap, [], ErrorSpecs)
> ;
> @@ -130,9 +133,12 @@ read_event_set(SpecsFileName, EventSetName, EventSpecMap, ErrorSpecs, !IO) :-
> TermOpenRes = error(TermOpenError),
> EventSetName = "",
> EventSpecMap = map.init,
> - Pieces = [words(io.error_message(TermOpenError)), nl],
> - ErrorSpec = error_spec(severity_error, phase_term_to_parse_tree,
> - [error_msg(no, do_not_treat_as_first, 0, [always(Pieces)])]),
> + Pieces = [words("Could not open"), quote(TermFileName),
> + words(":"), words_quote(error_message(TermOpenError)), nl],
> + ErrorSpec = error_spec(severity_error,
> + phase_term_to_parse_tree,
> + [error_msg(no, do_not_treat_as_first, 0,
> + [always(Pieces)])]),
> ErrorSpecs = [ErrorSpec]
> )
> else
> @@ -143,7 +149,17 @@ read_event_set(SpecsFileName, EventSetName, EventSpecMap, ErrorSpecs, !IO) :-
> [error_msg(no, do_not_treat_as_first, 0, [always(Pieces)])]),
> ErrorSpecs = [ErrorSpec]
> ),
> - io.remove_file(TermFileName, _RemoveRes, !IO).
> + io.remove_file(TermFileName, _RemoveRes, !IO)
> + ;
> + TermFileNameResult= error(TermFileNameError),
space
> + EventSetName = "",
> + EventSpecMap = map.init,
> + Pieces = [words("Could not create temporary file:"),
> + words_quote(error_message(TermFileNameError)), nl],
> + ErrorSpec = error_spec(severity_error, phase_term_to_parse_tree,
> + [error_msg(no, do_not_treat_as_first, 0, [always(Pieces)])]),
> + ErrorSpecs = [ErrorSpec]
> + ).
(maybe refactor read_event_set)
> diff --git a/compiler/write_deps_file.m b/compiler/write_deps_file.m
> index e661e06..caac87b 100644
> --- a/compiler/write_deps_file.m
> +++ b/compiler/write_deps_file.m
> @@ -159,8 +159,14 @@ write_dependency_file(Globals, ModuleAndImports, AllDeps,
> % parallel makes, we first create the file with a temporary name,
> % and then rename it to the desired name when we've finished.
>
> - io.make_temp(dir.dirname(DependencyFileName), "tmp_d",
> - TmpDependencyFileName, !IO),
> + io.make_temp_res(dir.dirname(DependencyFileName), "tmp_d",
> + "", TmpDependencyFileNameRes, !IO),
> + (
> + TmpDependencyFileNameRes = error(Error),
> + Message = "Could not create temporary file: " ++ error_message(Error),
> + report_error(Message, !IO)
> + ;
> + TmpDependencyFileNameRes = ok(TmpDependencyFileName),
> maybe_write_string(Verbose, "% Writing auto-dependency file `", !IO),
> maybe_write_string(Verbose, DependencyFileName, !IO),
> maybe_write_string(Verbose, "'...", !IO),
[snip!]
I think you should add a helper predicate (just in the compiler
directory) that does make_temp_file followed by open_output:
:- pred open_temp_output(io.res(pair(string, text_output_stream))::out,
io::di, io::uo) is det.
then this huge predicate does not need another level of indentation.
Other changes in this diff would benefit from using this predicate as
well.
> diff --git a/deep_profiler/conf.m b/deep_profiler/conf.m
> index dc42450..11bdd1c 100644
> --- a/deep_profiler/conf.m
> +++ b/deep_profiler/conf.m
> @@ -82,7 +82,8 @@ server_name(ServerName, !IO) :-
> :- pred server_name_2(string::out, io::di, io::uo) is det.
>
> server_name_2(ServerName, !IO) :-
> - io.make_temp(TmpFile, !IO),
> + io.make_temp_res(TmpFileResult, !IO),
> + ( TmpFileResult = ok(TmpFile),
Add newline.
> hostname_cmd(HostnameCmd),
> ServerRedirectCmd =
> string.format("%s > %s", [s(HostnameCmd), s(TmpFile)]),
> @@ -102,11 +103,13 @@ server_name_2(ServerName, !IO) :-
> then
> ServerName = ServerNamePrime
> else
> - unexpected($module, $pred, "malformed server name")
> + unexpected($module, $pred,
> + "malformed server name")
> )
> ;
> TmpReadRes = error(_, _),
> - unexpected($module, $pred, "cannot read server's name")
> + unexpected($module, $pred,
> + "cannot read server's name")
> ),
> io.close_input(TmpStream, !IO)
> ;
> @@ -123,6 +126,10 @@ server_name_2(ServerName, !IO) :-
> Res1 = error(_),
> unexpected($module, $pred,
> "cannot execute cmd to find the server's name")
> + )
> + ;
> + TmpFileResult = error(_),
> + unexpected($module, $pred, "Cannot create temporary file")
> ).
>
> :- pred maybe_server_port(maybe(string)::out, io::di, io::uo) is det.
> diff --git a/library/io.m b/library/io.m
> index 41a3f10..a944075 100644
> --- a/library/io.m
> +++ b/library/io.m
> @@ -1319,46 +1319,54 @@
> % File handling predicates.
> %
>
> - % make_temp(Name, !IO) creates an empty file whose name is different
> - % to the name of any existing file. Name is bound to the name of the file.
> - % It is the responsibility of the caller to delete the file when it
> - % is no longer required.
> + % make_temp(Result, !IO) creates an empty file whose name is different
new predicate name
> + % to the name of any existing file. If successful Result returns the name
> + % of the file. It is the responsibility of the caller to delete the file
> + % when it is no longer required.
> %
> % The file is placed in the directory returned by get_temp_directory/3.
> %
> - % Throws an io.error exception if the temporary file could not be created.
> - %
> % On the Erlang and Java backends, this does not attempt to create the file
> % with restrictive permissions (600 on Unix-like systems) and therefore
> % should not be used when security is required.
> %
> +:- pred make_temp_res(io.res(string)::out, io::di, io::uo) is det.
> +
> + % Like make_temp_res/3 except it throws an io.error exception if the
> + % temporary file could not be created.
> + %
> +:- pragma obsolete(make_temp/3).
> :- pred make_temp(string::out, io::di, io::uo) is det.
>
> - % make_temp(Dir, Prefix, Name, !IO) creates an empty file whose
> + % make_temp(Dir, Prefix, Suffix, Result, !IO) creates an empty file whose
new predicate name
> % name is different to the name of any existing file. The file will reside
> % in the directory specified by Dir and will have a prefix using up to
> - % the first 5 characters of Prefix. Name is bound to the name of the
> - % file. It is the responsibility of the caller to delete the file when it
> - % is no longer required.
> - %
> - % Throws an io.error exception if the temporary file could not be created.
> + % the first 5 characters of Prefix. If successful, Result returns the name
> + % of the file. It is the responsibility of the caller to delete the file
> + % when it is no longer required.
> %
> % The C# backend has the following limitations:
> % - Dir is ignored.
> % - Prefix is ignored.
> + % - Suffix is ignored.
> + %
> + % On the Erlang backend Suffix is ignored.
> %
> % On the Erlang and Java backends, this does not attempt to create the file
> % with restrictive permissions (600 on Unix-like systems) and therefore
> % should not be used when security is required.
> %
> -:- pred make_temp(string::in, string::in, string::out, io::di, io::uo)
> - is det.
> +:- pred make_temp_res(string::in, string::in, string::in, io.res(string)::out,
> + io::di, io::uo) is det.
The list of exceptions gets longer and longer :(
Ignoring the Suffix seems worse than ignoring the Dir and Prefix;
presumably the caller wants a specific file extension?
If Suffix can't be supported on C#, perhaps we should just drop it.
It's only used so far in that --restricted-command-line path, which
could be handled some other way (e.g. renaming the temp file).
Unfortunately the log message that added the ++ LibExt does not say why
it was added.
Peter
More information about the reviews
mailing list