On Thu, 28 Feb 2008, SF Markus Elfring wrote: > > If Szaka would not have given error handling very high priority > > ntfs-3g would not work as well as it does. > > I guess that there are a few places left for fine-tuning in the software > infrastructure.
There are a few places, and Szaka already gave you a suggesiton on how to improve logging for example. I reviewed your patch and did not find a single change which I would take on board because (at least among free software authors, there is a tradition to demand that code has to be right to be merged). Saka gave you directions to improve it, and I would not take it either as it is because it clutters the code with statements which should be put into a logging library and not into the main code stream. The string changes appear also bad to me because they may not even be an improvement overall and can lead to bugs itself because these changes may change the value of sizeof() on these strings. For further info, please read this thread from the Linux Kernel ML: http://lkml.org/lkml/2005/6/21/16 Also, to make reviewing patches easyer, they should be splitted up in one patch per addressed issue, e.g. fixes for different warning types should not be mixed into one patch if it extends over several pages. Besides I want to say a word on the functions for which adding return code checks is sensible: I do not agree on adding extra code to the program binary to check which exists the program on a condition like close() of a directory like /proc/filesystems returning an error code, because even if the kernel would return an error from this system call at some point, it would be a negible issue which would not even need close examination as the function in question is only executed once and it does not matter much if that filedescriptor stays open or not because e.g. the close() was interrupted by a signal. Of course one could write a handler for close which handles the return values of close(), but in places like these, the program may just repeat the close if -EINTR was returned on close and otherwise just issue a warning and continue. But that should be turned into an optional sanity check which only enabled when the program is compiled with --enable-debug to not hurt the binary code size without proper justification. > > Pointing at possible areas of possible improvements like the handling > > of this return value fchdir is easy (gcc does it automatically, so you > > do not even have to have human intelligence for it) but finding the > > right way to do handle it what separates your patch for it from a > > useful contribution, IMHO. > > Some efforts are usual to achieve consensus on complete error and exception > handling. Handling that fchdir is not high on the my personal list of priorities and for the other parts I gave now my reasons also why I would not apply your patch. > > I would not bet that Linux kernel will not switch to C++ anytime soon > > and I would like to avoid any relious discussion like using C++ > > to use it's exception support. > > Would the reuse of the software "http://cexcept.sourceforge.net/"; help? No: Please consider that all the software which ntfs-3g depends on needs to be available for all platforms where ntfs-3g runs and I would also require that it is actively maintained. The software to which you refer to seems to be abadoned since 2000 and I am not aware of any project which uses it. I find it much more useful to use the facilities provided by C already to handle exceptions. The Linux Kernel for example makes extensive use of the goto statement as an inexpensive jump to the error recovery section of the function. More important than working on requriting your patch to call a new, to-be-written error logging library, I see it much more important to work for example on the permission handling branch to improve it's usability and ensure that the return values of all ntfs-3g-internal functions which perform important roles like index lookup are properly handled. This can be done by estabilishing the use of __attribute__ ((__warn_unused_result__)) (a GCC extension which can be used when gcc is used for compilation) to generate warnings if the return values of critical functions are not checked. I'll submit a patch (for demonsration only) in this regard in a few moments. Bernhard -- So "used" cases that used "unused" could break, though older compilers in essence used "unused" to mean both "used" and "unused". Since "unused" becomes useless for using in "used" cases, we now must be sure to use "used" when that's the use that's useful. -- Roland McGrath :-) - a funny quote about __attribute__((unused)) Source: http://www.ussg.iu.edu/hypermail/linux/kernel/0410.1/2000.html ------------------------------------------------------------------------- This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/ _______________________________________________ ntfs-3g-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/ntfs-3g-devel
