"Bert Huijben" <b...@qqmail.nl> writes:
>> -----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:
>> > - 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,
>> NULL,
>> > + _("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
18217977168218728251394687124089371267338971528174
76066745969754933395997209053270030282678007662838
67331479599455916367452421574456059646801054954062
15017704234999886990788594743994796171248406730973
80736524850563115569208508785942830080999927310762
50733948404739350551934565743979678824151197232629
947748581376,
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.
--
Philip
