You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@lyra.org> on 2001/03/10 18:35:30 UTC
fs may not be opened
I just hit a problem where I called svn_fs_list_transactions() without a
prior call to svn_fs_open_berkeley(). It segfaulted within DB because we
tried to create a transaction (in begin_trail) using fs->env which was NULL.
I'm totally fine with allowing segfaults due to caller bogosity (and prefer
it rather than litter my code with arg validation), but figured that I'd
raise it in case somebody disagreed.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: fs may not be opened
Posted by Jim Blandy <ji...@zwingli.cygnus.com>.
Yoshiki Hayashi <yo...@xemacs.org> writes:
> Jim Blandy <ji...@zwingli.cygnus.com> writes:
>
> > The plan was to have every function in svn_fs.h call svn_fs__check_fs
> > (see err.h) before it does anything else. Franklin should go and add
> > all those calls.
>
> I didn't know Franklin does something good, but here's the
> patch anyway. :-)
Franklin is the one who left them out in the first place.
The patch looks great --- thanks!
Re: fs may not be opened
Posted by Jim Blandy <ji...@zwingli.cygnus.com>.
The only tweak I made to the patch was that it's okay to call
svn_fs_close_fs on an unopened filesystem.
Re: fs may not be opened
Posted by Yoshiki Hayashi <yo...@xemacs.org>.
Jim Blandy <ji...@zwingli.cygnus.com> writes:
> The plan was to have every function in svn_fs.h call svn_fs__check_fs
> (see err.h) before it does anything else. Franklin should go and add
> all those calls.
I didn't know Franklin does something good, but here's the
patch anyway. :-)
* fs.c (svn_fs_close_fs): Check fs.
* rev-table.c (svn_fs_youngest_rev): Ditto.
* rev-table.c (svn_fs_revision_prop): Ditto.
* rev-table.c (svn_fs_revision_proplist): Ditto.
* rev-table.c (svn_fs_change_rev_prop): Ditto.
* tree.c (svn_fs_revision_root): Ditto.
* txn.c (svn_fs_begin_txn): Ditto.
* txn.c (svn_fs_open_txn): Ditto.
* txn.c (svn_fs_list_transactions): Ditto.
* editor.c (svn_fs_get_editor): Ditto. Initialize eb->fs.
* fs-test.c (call_functions_with_unopend_fs): New test case.
Call functions with not opened and check returned errors.
Index: libsvn_fs/editor.c
===================================================================
RCS file: /cvs/subversion/subversion/libsvn_fs/editor.c,v
retrieving revision 1.24
diff -u -r1.24 editor.c
--- libsvn_fs/editor.c 2001/03/08 16:21:12 1.24
+++ libsvn_fs/editor.c 2001/03/12 08:09:29
@@ -20,8 +20,8 @@
#include "svn_delta.h"
#include "svn_fs.h"
#include "dag.h"
+#include "err.h"
-
/*** Editor batons. ***/
@@ -411,6 +411,8 @@
apr_pool_t *subpool = svn_pool_create (pool);
struct edit_baton *eb = apr_pcalloc (subpool, sizeof (*eb));
+ SVN_ERR (svn_fs__check_fs (fs));
+
/* Set up the editor. */
e->replace_root = replace_root;
e->delete_entry = delete_entry;
@@ -432,6 +434,7 @@
eb->hook_baton = hook_baton;
eb->base_rev = base_revision;
eb->base_path = svn_string_dup (base_path, subpool);
+ eb->fs = fs;
*edit_baton = eb;
*editor = e;
Index: libsvn_fs/fs.c
===================================================================
RCS file: /cvs/subversion/subversion/libsvn_fs/fs.c,v
retrieving revision 1.38
diff -u -r1.38 fs.c
--- libsvn_fs/fs.c 2001/03/02 23:07:36 1.38
+++ libsvn_fs/fs.c 2001/03/12 08:09:29
@@ -224,6 +224,7 @@
{
svn_error_t *svn_err = 0;
+ SVN_ERR (svn_fs__check_fs (fs));
/* We've registered cleanup_fs_apr as a cleanup function for this
pool, so just freeing the pool should shut everything down
nicely. But do catch an error, if one occurs. */
Index: libsvn_fs/rev-table.c
===================================================================
RCS file: /cvs/subversion/subversion/libsvn_fs/rev-table.c,v
retrieving revision 1.9
diff -u -r1.9 rev-table.c
--- libsvn_fs/rev-table.c 2001/03/06 00:11:00 1.9
+++ libsvn_fs/rev-table.c 2001/03/12 08:09:29
@@ -233,6 +233,9 @@
{
svn_revnum_t youngest;
struct youngest_rev_args args;
+
+ SVN_ERR (svn_fs__check_fs (fs));
+
args.youngest_p = &youngest;
args.fs = fs;
@@ -298,6 +301,8 @@
struct revision_prop_args args;
svn_string_t *value;
+ SVN_ERR (svn_fs__check_fs (fs));
+
args.value_p = &value;
args.fs = fs;
args.rev = rev;
@@ -356,6 +361,8 @@
struct revision_proplist_args args;
apr_hash_t *table;
+ SVN_ERR (svn_fs__check_fs (fs));
+
args.table_p = &table;
args.fs = fs;
args.rev = rev;
@@ -488,6 +495,8 @@
apr_pool_t *pool)
{
struct change_rev_prop_args args;
+
+ SVN_ERR (svn_fs__check_fs (fs));
args.fs = fs;
args.rev = rev;
Index: libsvn_fs/tree.c
===================================================================
RCS file: /cvs/subversion/subversion/libsvn_fs/tree.c,v
retrieving revision 1.41
diff -u -r1.41 tree.c
--- libsvn_fs/tree.c 2001/03/11 01:17:22 1.41
+++ libsvn_fs/tree.c 2001/03/12 08:09:30
@@ -1770,6 +1770,8 @@
struct revision_root_args args;
svn_fs_root_t *root;
+ SVN_ERR (svn_fs__check_fs (fs));
+
args.root_p = &root;
args.fs = fs;
args.rev = rev;
Index: libsvn_fs/txn.c
===================================================================
RCS file: /cvs/subversion/subversion/libsvn_fs/txn.c,v
retrieving revision 1.33
diff -u -r1.33 txn.c
--- libsvn_fs/txn.c 2001/03/11 02:40:36 1.33
+++ libsvn_fs/txn.c 2001/03/12 08:09:30
@@ -109,6 +109,8 @@
svn_fs_txn_t *txn;
struct begin_txn_args args;
+ SVN_ERR (svn_fs__check_fs (fs));
+
args.txn_p = &txn;
args.fs = fs;
args.rev = rev;
@@ -443,6 +445,9 @@
{
svn_fs_txn_t *txn;
struct open_txn_args args;
+
+ SVN_ERR (svn_fs__check_fs (fs));
+
args.txn_p = &txn;
args.fs = fs;
args.name = name;
@@ -477,6 +482,8 @@
{
char **names;
struct list_transactions_args args;
+
+ SVN_ERR (svn_fs__check_fs (fs));
args.names_p = &names;
args.fs = fs;
Index: tests/libsvn_fs/fs-test.c
===================================================================
RCS file: /cvs/subversion/subversion/tests/libsvn_fs/fs-test.c,v
retrieving revision 1.45
diff -u -r1.45 fs-test.c
--- tests/libsvn_fs/fs-test.c 2001/03/11 02:40:40 1.45
+++ tests/libsvn_fs/fs-test.c 2001/03/12 08:09:30
@@ -268,6 +268,95 @@
}
+static svn_error_t *
+check_no_fs_error (svn_error_t *err)
+{
+ if (err && (err->apr_err != SVN_ERR_FS_NOT_OPEN))
+ return svn_error_create
+ (SVN_ERR_FS_GENERAL, 0, NULL, pool,
+ "checking not opened filesystem got wrong error");
+ else if (! err)
+ return svn_error_create
+ (SVN_ERR_FS_GENERAL, 0, NULL, pool,
+ "checking not opened filesytem failed to get error");
+ else
+ return SVN_NO_ERROR;
+}
+
+
+/* Call functions with not yet opened filesystem and see it returns
+ correct error. */
+static svn_error_t *
+call_functions_with_unopened_fs (const char **msg)
+{
+ svn_error_t *err;
+ svn_fs_t *fs = svn_fs_new (pool);
+
+ *msg = "Call functions with not opened filesystem and check errors";
+ err = svn_fs_close_fs (fs);
+ SVN_ERR (check_no_fs_error (err));
+ err = svn_fs_set_berkeley_errcall (fs, berkeley_error_handler);
+ SVN_ERR (check_no_fs_error (err));
+
+ {
+ svn_fs_txn_t *ignored;
+ err = svn_fs_begin_txn (&ignored, fs, 0, pool);
+ SVN_ERR (check_no_fs_error (err));
+ err = svn_fs_open_txn (&ignored, fs, "0", pool);
+ SVN_ERR (check_no_fs_error (err));
+ }
+
+ {
+ char **ignored;
+ err = svn_fs_list_transactions (&ignored, fs, pool);
+ SVN_ERR (check_no_fs_error (err));
+ }
+
+ {
+ svn_fs_root_t *ignored;
+ err = svn_fs_revision_root (&ignored, fs, 0, pool);
+ SVN_ERR (check_no_fs_error (err));
+ }
+
+ {
+ svn_revnum_t ignored;
+ err = svn_fs_youngest_rev (&ignored, fs, pool);
+ SVN_ERR (check_no_fs_error (err));
+ }
+
+ {
+ svn_string_t *ignored, *unused;
+ err = svn_fs_revision_prop (&ignored, fs, 0, unused, pool);
+ SVN_ERR (check_no_fs_error (err));
+ }
+
+ {
+ apr_hash_t *ignored;
+ err = svn_fs_revision_proplist (&ignored, fs, 0, pool);
+ SVN_ERR (check_no_fs_error (err));
+ }
+
+ {
+ svn_string_t *unused1, *unused2;
+ err = svn_fs_change_rev_prop (fs, 0, unused1, unused2, pool);
+ SVN_ERR (check_no_fs_error (err));
+ }
+
+ {
+ void *edit_baton, *hook_baton;
+ svn_delta_edit_fns_t *editor;
+ svn_string_t *base_path, *log_msg;
+ svn_fs_commit_hook_t *hook;
+
+ err = svn_fs_get_editor (&editor, &edit_baton, fs, 0, base_path,
+ log_msg, hook, hook_baton, pool);
+ SVN_ERR (check_no_fs_error (err));
+ }
+
+ return SVN_NO_ERROR;
+}
+
+
/* Make sure we get txn lists correctly. */
static svn_error_t *
verify_txn_list (const char **msg)
@@ -1712,6 +1801,7 @@
reopen_trivial_transaction,
create_file_transaction,
verify_txn_list,
+ call_functions_with_unopened_fs,
write_and_read_file,
create_mini_tree_transaction,
create_greek_tree_transaction,
--
Yoshiki Hayashi
Re: fs may not be opened
Posted by Jim Blandy <ji...@zwingli.cygnus.com>.
Greg Stein <gs...@lyra.org> writes:
> I just hit a problem where I called svn_fs_list_transactions() without a
> prior call to svn_fs_open_berkeley(). It segfaulted within DB because we
> tried to create a transaction (in begin_trail) using fs->env which was NULL.
>
> I'm totally fine with allowing segfaults due to caller bogosity (and prefer
> it rather than litter my code with arg validation), but figured that I'd
> raise it in case somebody disagreed.
It would be nice if the library handled this a little more gracefully.
It's kind of odd that one can have an FS that doesn't actually refer
to anything yet, and we should help people out as much as we can.
The plan was to have every function in svn_fs.h call svn_fs__check_fs
(see err.h) before it does anything else. Franklin should go and add
all those calls.