"Bert Huijben" <bert_at_qqmail.nl> writes:
>> -----Original Message----- From: Julian Foad
>> [mailto:julian.foad_at_wandisco.com] Sent: maandag 21 december 2009
>> 16:05 To: Stefan Sperling Cc: Philip Martin; Mark Phippard;
>> dev_at_subversion.apache.org Subject: Re: svn commit: r884250 -
>> Stefan Sperling wrote:
>> > - 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';
>> > + end = apr_cpystrn(txn->txn_id, txn_id, sizeof(txn->txn_id)); if
>> > + (end < txn->txn_id + strlen(txn_id)) return
>> > + svn_error_createf(SVN_ERR_FS_TXN_NAME_TOO_LONG,
>> > + _("Transaction name '%s' was
>> > + truncated"), txn_id);
>> The "was truncated" error message is not helpful. (That sounds like
>> the server is saying, "I have renamed one of your transactions".)
>> Any reason not to use SVN_ERR_ASSERT?
>> The logic looks correct.
> Assertions are not nice to library users. (Not everybody uses 'svn' as
> his/her subversion client). An assertion/abort in release code will
> most likely make my users loose at least some of their work.
> I don't know if this case is user and or network triggerable, but
> littering our code with assertions makes our code slow and easier to
> crash, while the error return method was designed to be more graceful.
> I guess this code is server side. For this case errors are transferred
> to the client, while a server assertion will only be visible as
> connection closed unexpectedly' or something similar.
According to key-gen.h the whole change is more-or-less pointless.
/* The alphanumeric keys passed in and out of svn_fs_fs__next_key
are guaranteed never to be longer than this many bytes,
*including* the trailing null byte. It is therefore safe
to declare a key as "char key[MAX_KEY_SIZE]".
Note that this limit will be a problem if the number of
keys in a table ever exceeds
but that's a risk we'll live with for now. */
#define MAX_KEY_SIZE 200
Are we going to exceed the limit in the forseable future? If not then
all this checking is superflous. The original strlen looked fine to me.
Received on 2009-12-21 16:42:37 CET