On Tue, Mar 31, 2015 at 2:40 PM, Djalal Harouni <tix...@opendz.org> wrote: > On Tue, Mar 31, 2015 at 11:10:34AM -0700, Shawn Landden wrote: >> 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. > No, I don't see any mention of assert_se(). > > > With this you are breaking the logic that commes with the following > commits: > 0c0cdb06c139b52ff1 > 787784c4c1b24a132 > e80afdb3e4a123 > 288c0991d5a0b240 > b680a194bf9ed4c6 > bdf7026e9557349c > 8d95631ea6c039a60b > > And the list is long... Please, check the code and the commit logs... > > You are also hiding the error logs. A simple "grep -r "assert_se" src/" > will show you perhaps the ones that can be improved without affecting > the semantics of other callers. > > You do realize that some of these are tests to make sure that systemd > works fine ? please read the commit logs, you will clearly see that the > authors do *not* want assert_se() to be optimized, and some authors will > even use assert_se() to handle errors in the core code. > > IOW this patch will *weaken* all the test infrastructure and perhaps > other areas. > > How tests are supposed to log errors after this ? > > >> I submitted a new version of this patch, with numbers (10kB smaller) >> to the mailing list. > Hmm thanks for the patch, this seems a valid number, but as I have said > I still think this is not the best approach. > > You are affecting all the callers, if you want to do this then update > the parts that really need to be updated, the real callers, IMHO before > you update a function, first check callers. > > So here if you think that the caller worth it and it is robust, then yeh > remove the assert_se() from there and make it handle the error correctly, > hmm but perhaps others may think differently... > > I will let others comment. Well the patch I last submitted still calls abort() (did you see that patch?). but yeah I don't have strong opinions about this.... > > Thanks! > > -- > Djalal Harouni > _______________________________________________ > 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
