On Mon, Dec 21, 2009 at 05:38:28PM +0100, Bert Huijben wrote:
>
>
> > -----Original Message-----
> > From: Julian Foad [mailto:julianf...@btopenworld.com]
> > Sent: maandag 21 december 2009 16:51
> > To: Bert Huijben
> > Cc: 'Stefan Sperling'; 'Philip Martin'; 'Mark Phippard';
> > dev@subversion.apache.org
> > Subject: RE: svn commit: r884250 -
> > /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> >
> > Bert Huijben wrote:
> > >
> > > > -----Original Message-----
> > > > From: Julian Foad [mailto:julian.f...@wandisco.com]
> > > > Sent: maandag 21 december 2009 16:05
> > > > To: Stefan Sperling
> > > > Cc: Philip Martin; Mark Phippard; dev@subversion.apache.org
> > > > Subject: Re: svn commit: r884250 -
> > > > /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> > > >
> > > > Stefan Sperling wrote:
>
> > > Maybe assertions and aborts are easier for Subversion developers, but
> > they are not for our users.
> >
> > That's just stating what happens if there is no user-friendly handling
> > defined. The whole point of SVN_ERR_ASSERT() is to make an internal
> > consistency check brief to code, easy to handle consistently in
> > high-level code (convert them into regular error messages at some place
> > if appropriate) and still certain to be caught (in a crude way - abort)
> > if we forget or choose not to write handler code.
>
> One problem with this is that we convert all assertions to
> SVN_ERR_ASSERTION_FAIL, so we can never convert them to anything more
> detailed without processing the file and line number. Which you can't
> expect any ordinary user will do.
I'll stay out of the whole SVN_ERR_ASSERT discussion.
If we boil down the diff as below is this OK?
Stefan
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 892649)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -34,6 +34,7 @@
#include <apr_lib.h>
#include <apr_md5.h>
#include <apr_sha1.h>
+#include <apr_strings.h>
#include <apr_thread_mutex.h>
#include "svn_pools.h"
@@ -457,8 +458,7 @@ get_shared_txn(svn_fs_t *fs, const char *txn_id, s
}
assert(strlen(txn_id) < sizeof(txn->txn_id));
- strncpy(txn->txn_id, txn_id, sizeof(txn->txn_id) - 1);
- txn->txn_id[sizeof(txn->txn_id) - 1] = '\0';
+ apr_cpystrn(txn->txn_id, txn_id, sizeof(txn->txn_id));
txn->being_written = FALSE;
/* Link this transaction into the head of the list. We will typically
@@ -6641,14 +6641,12 @@ recover_find_max_ids(svn_fs_t *fs, svn_revnum_t re
if (svn_fs_fs__key_compare(node_id, max_node_id) > 0)
{
assert(strlen(node_id) < MAX_KEY_SIZE);
- strncpy(max_node_id, node_id, MAX_KEY_SIZE - 1);
- max_node_id[MAX_KEY_SIZE - 1] = '\0';
+ apr_cpystrn(max_node_id, node_id, MAX_KEY_SIZE);
}
if (svn_fs_fs__key_compare(copy_id, max_copy_id) > 0)
{
assert(strlen(copy_id) < MAX_KEY_SIZE);
- strncpy(max_copy_id, copy_id, MAX_KEY_SIZE - 1);
- max_copy_id[MAX_KEY_SIZE - 1] = '\0';
+ apr_cpystrn(max_copy_id, copy_id, MAX_KEY_SIZE);
}
if (kind == svn_node_file)
