Bert Huijben wrote:
>
> > -----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.
That's exactly what SVN_ERR_ASSERT was invented for, and the reason it
is catchable in client code and its action is redefinable. Please see
its definition and past discussions.
> 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.
It's not "littering", it's responsible, neat, flexible design. Using
loads of custom error message for internal malfunctions that the user
would have no ability to recover from is "littering".
> I guess this code is server side.
Yes.
> For this case errors are transferred to the client, while a server assertion will only be visible as 'connection closed unexpectedly' or something similar.
Depends on how it's handled at the caller. We should make the server do
something more friendly than just abort. Not by replacing all low-level
assertions with long-winded error returns, but by catching the assertion
errors at a higher level and handling a whole group of them in one
place.
> 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.
- Julian
Received on 2009-12-21 16:51:42 CET