On Tue, Mar 31, 2015 at 8:38 AM, Djalal Harouni <tix...@opendz.org> wrote: > On Mon, Mar 30, 2015 at 07:32:35PM -0700, Shawn Landden wrote: >> On Mon, Mar 30, 2015 at 5:04 PM, Djalal Harouni <tix...@opendz.org> wrote: >> > On Fri, Mar 27, 2015 at 09:51:26AM -0700, Shawn Landden wrote: >> >> On Fri, Mar 27, 2015 at 8:16 AM, Tom Gundersen <t...@jklm.no> wrote: >> > [...] >> >> >> * Current expression may modify/interact with a global state which may >> >> >> cause a fatal error, and if the caller wants to know if that failed, >> >> >> then abort(), your patch just introduced a regression, without the >> >> >> explicit abort(), all callers now have to call abort(). >> >> > >> >> > Yeah, this is the point I think. I still think the patch makes sense >> >> > though, it really don't see why there should be a distinction between >> >> > assert_se() and assert() when it comes to logging and aborting. >> >> > >> >> > If assert_se() fails it indicates we may have messed up the global >> >> > state, and if assert() fails it indicates that the global state may be >> >> > messed up (by someone else). Either way the global state is >> >> > potentially messed up and not logging/aborting seems as (un)safe in >> >> > both situations. >> >> > >> >> > Another way of seeing it, intuitively I don't see why we should >> >> > distinguish between >> >> > >> >> > assert_se(foo() >= 0); >> >> > >> >> > and >> >> > >> >> > r = foo(); >> >> > assert(r >= 0); >> >> Yes. The use case is that embedded people don't want all the strings >> >> that the logging adds to their binaries when these are ASSERTS, they >> >> are only expected to catch programming errors. It is not to make it >> >> faster, I think that is negligible. >> > Hmm embedded cases are real, I had to deal with some in the past. But >> > not sure here since I didn't see any numbers before/after stripping, but >> > perhaps you can start by updating the callers and their semantics if you >> > think that the code there is robust and it's worth it... ? >> How about we leave the abort() and just drop the logging with NDEBUG set? > Not sure if this is the best solution, you will just hide the path of > code where we failed! it will not be easy to debug later. > > As said, the callers except to have a log error and abort, if you remove > one of them you are altering callers, IMO they do not handle errors and > they do not log errors, they rely on assert_se(). As you know in systemd > we do log errors... > >> I think the performance impact is negligible, I just want to drop all >> the strings. >> Perhaps we could replace the logging with backtrace() in this case? > Don't know are you sure that if you add a backrack() routine there it > will be better than the current assert_se(), actually without real > numbers it is hard to say... > > IMO we should not touch this macro, the fix should be in the callers, so > you have to make sure that callers are robust and can handle errors > correctly (log them too perhaps)... > The point is that assert() and assert_se() should only be used for programming errors, this is covered in CODING_STYLE. > > -- > Djalal Harouni > http://opendz.org > _______________________________________________ > systemd-devel mailing list > systemd-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/systemd-devel
-- Shawn Landden _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
