Re: svn commit: r884250 - /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c

"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 -
>> /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