Julian Foad wrote on Mon, Apr 23, 2012 at 21:11:30 +0100:
>
>
>
>
> ----- 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.)
>
Yes. We don't distinguish various kinds of asserts, so the app needs to
assume the worst. But it's a cleaner termination than SIGABRT.
> >> 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?
>
The requirement for UTF-8 is at the repos layer, so at least ra_local
and ra_svn should cope just fine with arbitrary binary data in there.
I don't recall whether that's true for ra_dav too.
But I think that whether or not libsvn_ra* segfault on non-UTF-8 log
message wasn't your point.
> > 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-24 14:39:38 CEST