You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2013/02/14 21:52:58 UTC
[patch] Don't discard sqlite's error code
Someone on IRC got "E200030: I/O error", which prompted me to write
a patch that exposes the sqlite integer error code via the error string
(the err->apr_err value remains unchanged, 200030 SVN_ERR_SQLITE_ERROR).
Any objections in principle, or should I go ahead and see how many tests
this breaks?
Daniel
(I would have liked an "E%06d"-like syntax for this, but didn't come
up with one)
Index: subversion/libsvn_subr/sqlite.c
===================================================================
--- subversion/libsvn_subr/sqlite.c (revision 1446314)
+++ subversion/libsvn_subr/sqlite.c (working copy)
@@ -140,8 +140,9 @@ struct svn_sqlite__value_t
int sqlite_err__temp = (x); \
if (sqlite_err__temp != SQLITE_OK) \
return svn_error_createf(SQLITE_ERROR_CODE(sqlite_err__temp), \
- NULL, "sqlite: %s", \
- sqlite3_errmsg((db)->db3)); \
+ NULL, "sqlite: %s (%d)", \
+ sqlite3_errmsg((db)->db3), \
+ sqlite_err__temp); \
} while (0)
#define SQLITE_ERR_MSG(x, msg) do \
@@ -149,7 +150,8 @@ struct svn_sqlite__value_t
int sqlite_err__temp = (x); \
if (sqlite_err__temp != SQLITE_OK) \
return svn_error_createf(SQLITE_ERROR_CODE(sqlite_err__temp), \
- NULL, "sqlite: %s", (msg)); \
+ NULL, "sqlite: %s (%d)", (msg), \
+ sqlite_err__temp); \
} while (0)
@@ -171,8 +173,9 @@ exec_sql2(svn_sqlite__db_t *db, const char *sql, i
if (sqlite_err != SQLITE_OK && sqlite_err != ignored_err)
{
svn_error_t *err = svn_error_createf(SQLITE_ERROR_CODE(sqlite_err), NULL,
- _("sqlite: %s, executing statement '%s'"),
- err_msg, sql);
+ _("sqlite: %s (%d),"
+ " executing statement '%s'"),
+ err_msg, sqlite_err, sql);
sqlite3_free(err_msg);
return err;
}
@@ -289,7 +292,8 @@ svn_sqlite__step(svn_boolean_t *got_row, svn_sqlit
svn_error_t *err1, *err2;
err1 = svn_error_createf(SQLITE_ERROR_CODE(sqlite_result), NULL,
- "sqlite: %s", sqlite3_errmsg(stmt->db->db3));
+ "sqlite: %s (%d)",
+ sqlite3_errmsg(stmt->db->db3), sqlite_result);
err2 = svn_sqlite__reset(stmt);
return svn_error_compose_create(err1, err2);
}
@@ -739,8 +743,8 @@ init_sqlite(void *baton, apr_pool_t *pool)
{
int err = sqlite3_config(SQLITE_CONFIG_MULTITHREAD);
if (err != SQLITE_OK && err != SQLITE_MISUSE)
- return svn_error_create(SQLITE_ERROR_CODE(err), NULL,
- _("Could not configure SQLite"));
+ return svn_error_createf(SQLITE_ERROR_CODE(err), NULL,
+ _("Could not configure SQLite (%d)"), err);
}
SQLITE_ERR_MSG(sqlite3_initialize(), _("Could not initialize SQLite"));
@@ -793,7 +797,8 @@ internal_open(sqlite3 **db3, const char *path, svn
sqlite3_close(*db3);
return svn_error_createf(SQLITE_ERROR_CODE(err_code), NULL,
- "sqlite: %s: '%s'", msg, path);
+ "sqlite: %s (%d): '%s'",
+ msg, err_code, path);
}
}
}
@@ -863,7 +868,7 @@ close_apr(void *data)
}
if (result != SQLITE_OK)
- return SQLITE_ERROR_CODE(result);
+ return SQLITE_ERROR_CODE(result); /* ### lossy */
db->db3 = NULL;
Re: [patch] Don't discard sqlite's error code
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Fri, Feb 15, 2013 at 00:07:21 +0000:
> The problem is that sqlite3_errstr() seems to be rather recent -- it doesn't exist in my installed version which is sqlite v3.7.7.
>
We require 3.7.12.
> Feel free to commit what you have and we could re-visit this part later or never.
>
Okay, committted r1446410. We can improve this (potentially along the
lines of the snipped context) subsequently.
Thanks
Daniel
> - Julian
Re: [patch] Don't discard sqlite's error code
Posted by Julian Foad <ju...@btopenworld.com>.
--
Certified & Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
----- Original Message -----
> From: Daniel Shahaf <d....@daniel.shahaf.name>
> To: Julian Foad <ju...@btopenworld.com>
> Cc: "dev@subversion.apache.org" <de...@subversion.apache.org>
> Sent: Thursday, 14 February 2013, 18:45
> Subject: Re: [patch] Don't discard sqlite's error code
>
> Julian Foad wrote on Thu, Feb 14, 2013 at 22:11:13 +0000:
>> Daniel Shahaf wrote:
>>
>> > Julian Foad wrote on Thu, Feb 14, 2013 at 21:17:03 +0000:
>> >> Daniel Shahaf wrote:
>> >> > @@ -739,8 +743,8 @@ init_sqlite(void *baton, apr_pool_t
> *pool)
>> >> > {
>> >> > int err = sqlite3_config(SQLITE_CONFIG_MULTITHREAD);
>> >> > if (err != SQLITE_OK && err != SQLITE_MISUSE)
>> >> > - return svn_error_create(SQLITE_ERROR_CODE(err), NULL,
>> >> > - _("Could not configure
>> > SQLite"));
>> >> > + return svn_error_createf(SQLITE_ERROR_CODE(err),
> NULL,
>> >> > + _("Could not configure
> SQLite
>> > (%d)"),
>> >> > err);
>> >>
>> >> In cases like this one, it seems we should be using one of the
> above sqlite
>> > -> svn error converters so that we get the full SQLite description,
> and then
>> > wrapping the resulting svn error object with "Could not configure
>
>> > SQLite".
>> >>
>> >
>> > As in:
>> >
>> > return svn_error_quick_wrap(svn_error_create(SQLITE_ERROR_CODE(err),
>> > NULL, NULL)
>> > _("Could not configure SQLite
> (%d)"));
>> >
>> > ?
>>
>> No. I mean a chain of two svn error objects, one containing SQLite's
> expansion of 'err' (including both its number and its message), which
> you can get from e.g. the SQLITE_ERR() macro; and one containing the wrapper
> message "Could not configure SQLite". Such as:
>>
>> return svn_error_quick_wrap(SQLITE_ERR(err, db),
>> _("Could not configure SQLite"));
>>
>
> That won't compile because SQLITE_ERR() is a statement, not an
> expression.
>
> I suppose I could write a function wrapper around SQLITE_ERR, but maybe
> there's a better option...
Huh... And there's no 'db' here anyway. I believe, on delving a little deeper, that what we really want is:
[[[
/* If SQLite has been already initialized, sqlite3_config() returns
SQLITE_MISUSE. */
{
int err = sqlite3_config(SQLITE_CONFIG_MULTITHREAD);
if (err != SQLITE_OK && err != SQLITE_MISUSE)
- return svn_error_create(SQLITE_ERROR_CODE(err), NULL,
- _("Could not configure SQLite"));
+ return svn_error_quick_wrap(
+ svn_error_createf(SQLITE_ERROR_CODE(err), NULL,
+ "sqlite: %s", sqlite3_errstr(err))
+ _("Could not configure SQLite to multi-thread mode"));
}
SQLITE_ERR_MSG(sqlite3_initialize(), _("Could not initialize SQLite"));
]]]
The problem is that sqlite3_errstr() seems to be rather recent -- it doesn't exist in my installed version which is sqlite v3.7.7.
Feel free to commit what you have and we could re-visit this part later or never.
- Julian
Re: [patch] Don't discard sqlite's error code
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Thu, Feb 14, 2013 at 22:11:13 +0000:
> Daniel Shahaf wrote:
>
> > Julian Foad wrote on Thu, Feb 14, 2013 at 21:17:03 +0000:
> >> Daniel Shahaf wrote:
> >> > @@ -739,8 +743,8 @@ init_sqlite(void *baton, apr_pool_t *pool)
> >> > {
> >> > int err = sqlite3_config(SQLITE_CONFIG_MULTITHREAD);
> >> > if (err != SQLITE_OK && err != SQLITE_MISUSE)
> >> > - return svn_error_create(SQLITE_ERROR_CODE(err), NULL,
> >> > - _("Could not configure
> > SQLite"));
> >> > + return svn_error_createf(SQLITE_ERROR_CODE(err), NULL,
> >> > + _("Could not configure SQLite
> > (%d)"),
> >> > err);
> >>
> >> In cases like this one, it seems we should be using one of the above sqlite
> > -> svn error converters so that we get the full SQLite description, and then
> > wrapping the resulting svn error object with "Could not configure
> > SQLite".
> >>
> >
> > As in:
> >
> > return svn_error_quick_wrap(svn_error_create(SQLITE_ERROR_CODE(err),
> > NULL, NULL)
> > _("Could not configure SQLite (%d)"));
> >
> > ?
>
> No. I mean a chain of two svn error objects, one containing SQLite's expansion of 'err' (including both its number and its message), which you can get from e.g. the SQLITE_ERR() macro; and one containing the wrapper message "Could not configure SQLite". Such as:
>
> return svn_error_quick_wrap(SQLITE_ERR(err, db),
> _("Could not configure SQLite"));
>
That won't compile because SQLITE_ERR() is a statement, not an
expression.
I suppose I could write a function wrapper around SQLITE_ERR, but maybe
there's a better option...
> - Julian
Re: [patch] Don't discard sqlite's error code
Posted by Julian Foad <ju...@btopenworld.com>.
Daniel Shahaf wrote:
> Julian Foad wrote on Thu, Feb 14, 2013 at 21:17:03 +0000:
>> Daniel Shahaf wrote:
>> > @@ -739,8 +743,8 @@ init_sqlite(void *baton, apr_pool_t *pool)
>> > {
>> > int err = sqlite3_config(SQLITE_CONFIG_MULTITHREAD);
>> > if (err != SQLITE_OK && err != SQLITE_MISUSE)
>> > - return svn_error_create(SQLITE_ERROR_CODE(err), NULL,
>> > - _("Could not configure
> SQLite"));
>> > + return svn_error_createf(SQLITE_ERROR_CODE(err), NULL,
>> > + _("Could not configure SQLite
> (%d)"),
>> > err);
>>
>> In cases like this one, it seems we should be using one of the above sqlite
> -> svn error converters so that we get the full SQLite description, and then
> wrapping the resulting svn error object with "Could not configure
> SQLite".
>>
>
> As in:
>
> return svn_error_quick_wrap(svn_error_create(SQLITE_ERROR_CODE(err),
> NULL, NULL)
> _("Could not configure SQLite (%d)"));
>
> ?
No. I mean a chain of two svn error objects, one containing SQLite's expansion of 'err' (including both its number and its message), which you can get from e.g. the SQLITE_ERR() macro; and one containing the wrapper message "Could not configure SQLite". Such as:
return svn_error_quick_wrap(SQLITE_ERR(err, db),
_("Could not configure SQLite"));
- Julian
Re: [patch] Don't discard sqlite's error code
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Thu, Feb 14, 2013 at 21:17:03 +0000:
> Daniel Shahaf wrote:
> > @@ -739,8 +743,8 @@ init_sqlite(void *baton, apr_pool_t *pool)
> > {
> > int err = sqlite3_config(SQLITE_CONFIG_MULTITHREAD);
> > if (err != SQLITE_OK && err != SQLITE_MISUSE)
> > - return svn_error_create(SQLITE_ERROR_CODE(err), NULL,
> > - _("Could not configure SQLite"));
> > + return svn_error_createf(SQLITE_ERROR_CODE(err), NULL,
> > + _("Could not configure SQLite (%d)"),
> > err);
>
> In cases like this one, it seems we should be using one of the above sqlite -> svn error converters so that we get the full SQLite description, and then wrapping the resulting svn error object with "Could not configure SQLite".
>
As in:
return svn_error_quick_wrap(svn_error_create(SQLITE_ERROR_CODE(err), NULL, NULL)
_("Could not configure SQLite (%d)"));
?
> - Julian
Re: [patch] Don't discard sqlite's error code
Posted by Julian Foad <ju...@btopenworld.com>.
Daniel Shahaf wrote:
> Someone on IRC got "E200030: I/O error", which prompted me to write
> a patch that exposes the sqlite integer error code via the error string
> (the err->apr_err value remains unchanged, 200030 SVN_ERR_SQLITE_ERROR).
>
> Any objections in principle, or should I go ahead and see how many tests
> this breaks?
No objections, just an improvement:
> @@ -739,8 +743,8 @@ init_sqlite(void *baton, apr_pool_t *pool)
> {
> int err = sqlite3_config(SQLITE_CONFIG_MULTITHREAD);
> if (err != SQLITE_OK && err != SQLITE_MISUSE)
> - return svn_error_create(SQLITE_ERROR_CODE(err), NULL,
> - _("Could not configure SQLite"));
> + return svn_error_createf(SQLITE_ERROR_CODE(err), NULL,
> + _("Could not configure SQLite (%d)"),
> err);
In cases like this one, it seems we should be using one of the above sqlite -> svn error converters so that we get the full SQLite description, and then wrapping the resulting svn error object with "Could not configure SQLite".
- Julian
Re: [patch] Don't discard sqlite's error code
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 02/14/2013 03:52 PM, Daniel Shahaf wrote:
> Someone on IRC got "E200030: I/O error", which prompted me to write
> a patch that exposes the sqlite integer error code via the error string
> (the err->apr_err value remains unchanged, 200030 SVN_ERR_SQLITE_ERROR).
>
> Any objections in principle, or should I go ahead and see how many tests
> this breaks?
None here. Seems like a beneficial change to me. +1.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development