On Mon, Mar 19, 2012 at 6:33 PM, Randall Leeds <randall.le...@gmail.com> wrote: > On Mon, Mar 19, 2012 at 04:48, Filipe David Manana <fdman...@apache.org>wrote: > >> Any special reason why couch_log and couch_file don't get the same >> treatment (properly formatted error message) as couch_config and >> couch_config_writer? >> > > Hmm. I think maybe I should add it to couch_file because that makes a ton > of sense. At the time, I was thinking I would just let it bubble and it > would be caught above, as it certainly is in many cases. > > For couch_log, how can we log anything? It felt futile to try to invoke log > calls when couch_log fails to start.
For couch log just throw an exception/error containing the results of file:format_error/1 and/or io:format it to stdout/stderr like it's done in couch_server_sup.erl. Either approach will produce a much more user friendly error. > > >> >> Seems like a small (incidental) regression to me. >> >> thanks >> >> On Mon, Mar 19, 2012 at 3:56 AM, <rand...@apache.org> wrote: >> > remove unnecessary eaccess special casing >> > >> > It's better to let these errors bubble and/or not give them special >> > treatment when file:format_error/1 can do a better job of describing >> > the failure. >> > >> > >> > Project: http://git-wip-us.apache.org/repos/asf/couchdb/repo >> > Commit: http://git-wip-us.apache.org/repos/asf/couchdb/commit/48371335 >> > Tree: http://git-wip-us.apache.org/repos/asf/couchdb/tree/48371335 >> > Diff: http://git-wip-us.apache.org/repos/asf/couchdb/diff/48371335 >> > >> > Branch: refs/heads/COUCHDB-1445 >> > Commit: 483713352cb397510111798c4b076afad83f6c46 >> > Parents: 29eac04 >> > Author: Randall Leeds <rand...@apache.org> >> > Authored: Sun Mar 18 20:19:27 2012 -0700 >> > Committer: Randall Leeds <rand...@apache.org> >> > Committed: Sun Mar 18 20:41:42 2012 -0700 >> > >> > ---------------------------------------------------------------------- >> > src/couchdb/couch_config.erl | 11 ++++------- >> > src/couchdb/couch_config_writer.erl | 8 +++++--- >> > src/couchdb/couch_file.erl | 5 +---- >> > src/couchdb/couch_log.erl | 2 -- >> > src/couchdb/couch_server_sup.erl | 14 +++++++------- >> > 5 files changed, 17 insertions(+), 23 deletions(-) >> > ---------------------------------------------------------------------- >> > >> > >> > >> http://git-wip-us.apache.org/repos/asf/couchdb/blob/48371335/src/couchdb/couch_config.erl >> > ---------------------------------------------------------------------- >> > diff --git a/src/couchdb/couch_config.erl b/src/couchdb/couch_config.erl >> > index f669853..44a102d 100644 >> > --- a/src/couchdb/couch_config.erl >> > +++ b/src/couchdb/couch_config.erl >> > @@ -187,13 +187,10 @@ parse_ini_file(IniFile) -> >> > case file:read_file(IniFilename) of >> > {ok, IniBin0} -> >> > IniBin0; >> > - {error, eacces} -> >> > - throw({file_permission_error, IniFile}); >> > - {error, enoent} -> >> > - Fmt = "Couldn't find server configuration file ~s.", >> > - Msg = ?l2b(io_lib:format(Fmt, [IniFilename])), >> > - ?LOG_ERROR("~s~n", [Msg]), >> > - throw({startup_error, Msg}) >> > + {error, Reason} = Error -> >> > + ?LOG_ERROR("Couldn't read server configuration file ~s: ~s", >> > + [IniFilename, file:format_error(Reason)]), >> > + throw(Error) >> > end, >> > >> > Lines = re:split(IniBin, "\r\n|\n|\r|\032", [{return, list}]), >> > >> > >> http://git-wip-us.apache.org/repos/asf/couchdb/blob/48371335/src/couchdb/couch_config_writer.erl >> > ---------------------------------------------------------------------- >> > diff --git a/src/couchdb/couch_config_writer.erl >> b/src/couchdb/couch_config_writer.erl >> > index decd269..3a65c37 100644 >> > --- a/src/couchdb/couch_config_writer.erl >> > +++ b/src/couchdb/couch_config_writer.erl >> > @@ -22,6 +22,8 @@ >> > >> > -export([save_to_file/2]). >> > >> > +-include("couch_db.hrl"). >> > + >> > %% @spec save_to_file( >> > %% Config::{{Section::string(), Option::string()}, >> Value::string()}, >> > %% File::filename()) -> ok >> > @@ -38,9 +40,9 @@ save_to_file({{Section, Key}, Value}, File) -> >> > case file:write_file(File, NewFileContents) of >> > ok -> >> > ok; >> > - {error, eacces} -> >> > - {file_permission_error, File}; >> > - Error -> >> > + {error, Reason} = Error -> >> > + ?LOG_ERROR("Couldn't write config file ~s: ~s", >> > + [File, file:format_error(Reason)]), >> > Error >> > end. >> > >> > >> > >> http://git-wip-us.apache.org/repos/asf/couchdb/blob/48371335/src/couchdb/couch_file.erl >> > ---------------------------------------------------------------------- >> > diff --git a/src/couchdb/couch_file.erl b/src/couchdb/couch_file.erl >> > index e195db0..5e476af 100644 >> > --- a/src/couchdb/couch_file.erl >> > +++ b/src/couchdb/couch_file.erl >> > @@ -58,10 +58,7 @@ open(Filepath, Options) -> >> > {trap_exit, true} -> receive {'EXIT', Pid, _} -> ok end; >> > {trap_exit, false} -> ok >> > end, >> > - case Error of >> > - {error, eacces} -> {file_permission_error, Filepath}; >> > - _ -> Error >> > - end >> > + Error >> > end; >> > Error -> >> > Error >> > >> > >> http://git-wip-us.apache.org/repos/asf/couchdb/blob/48371335/src/couchdb/couch_log.erl >> > ---------------------------------------------------------------------- >> > diff --git a/src/couchdb/couch_log.erl b/src/couchdb/couch_log.erl >> > index 8e24cab..7fb95a7 100644 >> > --- a/src/couchdb/couch_log.erl >> > +++ b/src/couchdb/couch_log.erl >> > @@ -89,8 +89,6 @@ init([]) -> >> > case file:open(Filename, [append]) of >> > {ok, Fd} -> >> > {ok, #state{fd = Fd, level = Level, sasl = Sasl}}; >> > - {error, eacces} -> >> > - {stop, {file_permission_error, Filename}}; >> > Error -> >> > {stop, Error} >> > end. >> > >> > >> http://git-wip-us.apache.org/repos/asf/couchdb/blob/48371335/src/couchdb/couch_server_sup.erl >> > ---------------------------------------------------------------------- >> > diff --git a/src/couchdb/couch_server_sup.erl >> b/src/couchdb/couch_server_sup.erl >> > index 7baede3..be3c3a3 100644 >> > --- a/src/couchdb/couch_server_sup.erl >> > +++ b/src/couchdb/couch_server_sup.erl >> > @@ -46,7 +46,9 @@ start_server(IniFiles) -> >> > {ok, [PidFile]} -> >> > case file:write_file(PidFile, os:getpid()) of >> > ok -> ok; >> > - Error -> io:format("Failed to write PID file ~s, error: ~p", >> [PidFile, Error]) >> > + {error, Reason} -> >> > + io:format("Failed to write PID file ~s: ~s", >> > + [PidFile, file:format_error(Reason)]) >> > end; >> > _ -> ok >> > end, >> > @@ -121,12 +123,10 @@ start_server(IniFiles) -> >> > end end || Uri <- Uris], >> > case file:write_file(UriFile, Lines) of >> > ok -> ok; >> > - {error, eacces} -> >> > - ?LOG_ERROR("Permission error when writing to URI file ~s", >> [UriFile]), >> > - throw({file_permission_error, UriFile}); >> > - Error2 -> >> > - ?LOG_ERROR("Failed to write to URI file ~s: ~p~n", >> [UriFile, Error2]), >> > - throw(Error2) >> > + {error, Reason2} = Error -> >> > + ?LOG_ERROR("Failed to write to URI file ~s: ~s", >> > + [UriFile, file:format_error(Reason2)]), >> > + throw(Error) >> > end >> > end, >> > >> > >> >> >> >> -- >> Filipe David Manana, >> >> "Reasonable men adapt themselves to the world. >> Unreasonable men adapt the world to themselves. >> That's why all progress depends on unreasonable men." >> -- Filipe David Manana, "Reasonable men adapt themselves to the world. Unreasonable men adapt the world to themselves. That's why all progress depends on unreasonable men."
