Hi Shawn, On Tue, Mar 31, 2015 at 04:59:29PM -0700, Shawn Landden wrote: > 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: [...] > >> 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.... Yes I saw the patch and the numbers, thank you!
I just didn't comment to let the thread clean, and see what others may think. Thanks! -- Djalal Harouni http://opendz.org _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
