[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: Always use SVN_ERR_ASSERT [was: svn commit: r1329234 - in /subversion/trunk: ./ subversion/libsvn_delta/compat.c]

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Mon, 23 Apr 2012 21:11:30 +0100 (BST)

----- Original Message -----
> From: Daniel Shahaf <danielsh_at_elego.de>
> To: Julian Foad <julianfoad_at_btopenworld.com>
> Cc: Bert Huijben <bert_at_qqmail.nl>; "dev_at_subversion.apache.org" <dev_at_subversion.apache.org>
> Sent: Monday, 23 April 2012, 20:59
> Subject: Re: Always use SVN_ERR_ASSERT [was: svn commit: r1329234 - in /subversion/trunk: ./ subversion/libsvn_delta/compat.c]
>
> Julian Foad wrote on Mon, Apr 23, 2012 at 20:40:59 +0100:
>> Daniel Shahaf wrote:
>>
>> > Julian Foad wrote:
>> >>  I (Julian Foad) wrote:
>> >>  > There isn't currently an easy build switch (such as
> NDEBUG) to disable
>> >>  > SVN_ERR_ASSERT completely at compile time.  That's just
> a side issue.  If
>> >>  > you want such a switch, just ask; we can easily create one. 
> Or if you think we
>> >>  > need two levels of assertions -- one for quick tests and
> another for slow tests
>> >>  > -- and want to be able to compile-out the slow ones
> independently of the quick
>> >>  > ones, just ask.  But implying we should use 'assert'
> for slow tests and
>> >>  > 'SVN_ERR_ASSERT' for quick tests is the Wrong Way.
>> >>
>> >>  We can also introduce run-time control of whether the conditions
> are
>> >> evaluated: test a global 'assertions enabled?' variable or
> function
>> >> before evaluating the condition.  For example:
>> [...]
>> > That doesn't sound right.  Surely we don't want to allow
> disabling _all_
>> > uses of SVN_ERR_ASSERT() this way?  (Remember that some of them
>> > translate to segfaults (possibly corruptions?) if the condition
> doesn't
>> > hold)
>>
>> Hi Daniel.
>>
>> In places where there will be a seg-fault if the condition is false,
>> the assertion statement doesn't prevent abnormal program termination,
>> it only makes it easier to see what went wrong.
>>
>
> SVN_ERR_ASSERT() prevents an abnormal termination when a non-default
> malfunction handler has been installed.

Potentially, but the app can't tell how badly the library data has gone wrong, so may want to terminate the program anyway after informing the user.  (Quoting an email (from Steve King?) within the past year or two.)

>> In places where the processing will continue with wrong data or wrong
>> behaviour if the condition is false, the assertion statement doesn't
>> prevent the program from going wrong, it just changes the failure mode
>> to a more obvious one.
>>
>> People who don't care about the failure mode in such cases may wish to
>> turn off the checks.
>
> Depends, I think.  If libsvn_ra asserts that a log message is in UTF-8,
> it is reasonable to want to disable that since the only harm resulting
> is an svn_error_create() on the server.

How do you know it won't crash?

>  But if libsvn_fs asserts the
> sanity of a revision before committing it, I think most people will
> prefer to have the commit attempt fail.
>
> (The latter example is not realistic since, at least in FSFS, we
> generally return an svn_error_t rather than an assertion when a sanity
> check fails.)

- Julian
Received on 2012-04-23 22:12:04 CEST

This is an archived mail posted to the Subversion Dev mailing list.