You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@couchdb.apache.org by Filipe David Manana <fd...@apache.org> on 2012/03/19 12:48:44 UTC
Re: [3/3] git commit: remove unnecessary eaccess special casing
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?
Seems like a small (incidental) regression to me.
thanks
On Mon, Mar 19, 2012 at 3:56 AM, <ra...@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 <ra...@apache.org>
> Authored: Sun Mar 18 20:19:27 2012 -0700
> Committer: Randall Leeds <ra...@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."
Re: [3/3] git commit: remove unnecessary eaccess special casing
Posted by Filipe David Manana <fd...@apache.org>.
On Mon, Mar 19, 2012 at 6:33 PM, Randall Leeds <ra...@gmail.com> wrote:
> On Mon, Mar 19, 2012 at 04:48, Filipe David Manana <fd...@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, <ra...@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 <ra...@apache.org>
>> > Authored: Sun Mar 18 20:19:27 2012 -0700
>> > Committer: Randall Leeds <ra...@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."
Re: [3/3] git commit: remove unnecessary eaccess special casing
Posted by Randall Leeds <ra...@gmail.com>.
On Mon, Mar 19, 2012 at 04:48, Filipe David Manana <fd...@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.
>
> Seems like a small (incidental) regression to me.
>
> thanks
>
> On Mon, Mar 19, 2012 at 3:56 AM, <ra...@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 <ra...@apache.org>
> > Authored: Sun Mar 18 20:19:27 2012 -0700
> > Committer: Randall Leeds <ra...@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."
>