[00:12:23] --- jaltman has become available [00:13:31] what is the reason that the OpenAFS assert() implementation is called assert() instead of afs_assert()? [00:14:46] The assert() macro definition on Windows is a no-op in non-debug builds. As a result it is not safe to directly use assert() as a wrapper around a function call. [00:17:31] is it safe to change all references of #include to #include ? [00:24:14] I guess it isn't safe to do so in rx since that code is supposed to be afs independent [02:08:38] --- andersk has left [04:25:00] A no op, as in #define assert(x) or as in #define assert ? [07:03:02] #define assert(x) [07:04:41] well that's dumb We should see how many places we're using assert() with a parameter that actually has side effects. Maybe there aren't many? [07:12:42] --- Brandon_CMU has left [07:12:48] --- Brandon_CMU has become available [07:15:46] all of the lock acquisition and condition wait code in budb, comerr, des, libacl, lwp, rx, rxkad, ubik, util, venus, viced, vol, volser, WINNT/pthread/test. there are also seek operations wrapped with assert() in various locations. [07:20:34] --- deason has become available [07:22:38] I thought this was brought up before... and I thought the easiest way brought up was to just change the lock/cv stuff to use another macro that just did the eval for non-debug [07:33:02] in my opinion openafs should not be replacing system header files and macros with its own. While I personally believe that assertion statements should never be coded to have side effects, that is how the source tree was created. I would prefer that we as a starting point rename include/afs/assert.h to include/afs/afsassert.h and change the assert() macro within to afs_assert(). That will be the smallest change we can make for all directories other than rx and rxkad which I do not believe should have another afs dependency added. For those directories I propose that either a local rx_assert() macro be defined in rx.h or remove the use of assert() wrapping the lock operation. [07:33:25] I am sure this was brought up before but apparently nothing was done about it [07:36:04] Sure, we "shouldn't replace system header files", but of course the meaning of "system" is somewhat subjective (is des.h a system header?), and even for a conservative definition, newer OS's have system headers that were certainly not "system" when AFS was written. [07:37:23] That said, having a private header with the same name as a system header is perfectly valid and something the language was designed to deal with; that's why there's a difference between "" and <>. [07:37:24] assert.h is quite standard these days. [07:37:45] > perfectly valid ... as long as we don't publish it, of course [07:38:40] the problem we have is that assert.h is published to include/afs but we can't actually build the tree without include/afs being in the include path since none of the references to #include include the afs prefix [07:39:17] assert.h is not the only header file which is accessed in that manner. [07:39:57] The use of "" versus <> fails for openafs because we frequently build source files from current directories other than where the source file resides. [07:40:04] but the assert abuse that deason pointed out, isnt that he real issue? [07:40:10] Yeah; for starters, we really should use for files in include/afs instead of putting that on the path. That would solve a lot of prblems. [07:41:18] how unhappy will you be if the openafs rx, rxkad, rxgk, ... directories contain sources that include headers? [07:44:17] I think it would be nice for rx and related things to be a package separable from OpenAFS, but that's not currently true (you need OpenAFS's build system to compile it) and I see no reason to bend over backwards about it. [07:44:37] OTOH, you could just put assert.h in and have everything include it from there. [07:46:26] we build the rx sources in multiple directories. So unless we would to publish include/rx/assert.h and reference it as #include that will not work. Does that work for you? [07:47:34] That's exactly what I was suggesting doing. If something's going to be common, better to have it in than in [07:48:21] But also, we don't have to _install_ a file for it to appear in ${TOP_INCDIR}/rx [07:48:56] for the unix build system that is true; it is not true for windows [08:38:38] --- reuteras has left [08:53:44] I think I would be most comfortable with a separate rx/assert.h and afs/assert.h. the afs assert() macro references the afs/util/assert.c AssertionFailed() function and I'm not sure I want to move that into rx [08:55:13] What would you have the rx macro do? [08:55:15] also, remind me why we can't just use the OS's assert macro everywhere, other than the side-effect problem? [09:01:08] --- Jeffrey Altman has become available [09:07:43] --- Jeffrey Altman has left [09:18:40] The openafs macro calls AssertionFailed() with __FILE__ and __LINE__ and on some systems can log the failure. [09:19:31] What I attempted to do was use the OS assert.h on Windows since the Windows version buys us nothing. It calls DebugBreak() which is worse than simply letting the assertion be handled [09:22:09] What I would have the rxi_assert() macro do is call the dpf() macro to execute the operation and if the condition fails call the dpf() macro if it is defined and finally call the crt assert() [09:23:53] For non-rx I would have the afs_assert() macro behave as the current code works. It would be a rename of afs/assert.h to afs/afs_assert.h and replacement of all assert() references with afs_assert(). the Windows implementation of afs_assert() would perform the operation and then call the crt assert() [10:13:20] --- andersk has become available [10:21:42] --- steven.jenkins has left [10:22:04] --- steven.jenkins has become available [10:59:47] --- phalenor has left [11:09:48] --- phalenor has become available [11:34:39] --- Russ has become available [11:42:27] --- shadow@gmail.com/owlB905A33D has left [12:18:52] --- shadow@gmail.com/owlAB650B6D has become available [12:35:56] looking closely at rx, we shouldn't be using assert() there at all. Instead we have the osi_Assert and osi_Panic macros which are already defined. [12:49:04] Oh, that solves the problem neatly. [12:57:45] I will push that and a rename of afs/assert.h to afs/afs_assert.h later tonight. [13:06:14] if you didn't already write it, jeff, i can [14:21:47] I already wrote the rx and rxkad portion. I haven't done the afs/assert.h to afs/afs_assert.h portion [14:29:29] http://gerrit.openafs.org/2987 [16:10:37] --- deason has left [18:32:38] --- deason has become available [21:28:34] --- jaltman has left: Disconnected [21:34:21] --- jaltman has become available [21:56:18] --- deason has left [22:15:24] --- reuteras has become available [23:05:55] --- Russ has left: Disconnected