Sender: rich AT phekda DOT freeserve DOT co DOT uk Message-ID: <3EA5CD9C.741DEA9D@phekda.freeserve.co.uk> Date: Wed, 23 Apr 2003 00:17:48 +0100 From: Richard Dawe X-Mailer: Mozilla 4.77 [en] (X11; U; Linux 2.2.23 i586) X-Accept-Language: de,fr MIME-Version: 1.0 To: djgpp-workers AT delorie DOT com CC: dborca AT yahoo DOT com Subject: Re: dxe review References: <10304150357 DOT AA21198 AT clio DOT rice DOT edu> Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Reply-To: djgpp-workers AT delorie DOT com Hello. Charles Sandmann wrote: > I've put the modules for the updated dxe3 code in: > > ftp://clio.rice.edu/djgpp/dxe3.zip > http://clio.rice.edu/djgpp/dxe3.zip [snip] I've just taken a look at the code. I looked at this particular version of the ZIP: iolanthe:~/src/djgpp-dxe3/dxe3-20030421 =] ls -l /zips/dxe3-20030421.zip -rw-rw-r-- 1 rich rich 61522 Apr 20 22:54 /zips/dxe3-20030421.zip I have some comments on specific files: src/dxe/dxe3.txt: * This mentions dxe3.h at one point. The only file I can see in the archive is include/sys/dxe.h. Should it refer to that instead? If this file is historic, i.e. just a copy of the one from the DXE3, then ignore this comment. If it's supposed to be up-to-date regarding DJGPP CVS, then I think it should be updated. * Later: Ah, I see, dxegen.txi is now the definitive documentation. Maybe a note should be added at the top of dxe3.txt, to say that. src/dxe/dxegen.txi (some of these comments probably apply to dxe3.txt too): * -o is listed twice in the syntax for dxe3gen: explicitly and as an option. * "(where MODNAME stands for the name of your dynamic module)" - I think MODNAME should be in @code{}. * "you CANNOT have exported data inside the library" - CANNOT should be in @emph{} or @strong{} - probably @emph. * It's better to use @dots{} rather than '...'. * "Unix apps" -> "Unix applications" * "For plain C you will have to use the GNU C extension (which is also supported on all platforms where GNU C is available, but you are restricted with the compiler):" - "the GNU C extension" -> "GNU C extensions"; I don't think the text in brackets is needed - we've already said it's a GNU C-specific extension. * Function names in @code{} don't need brackets - we're not calling the functions. E.g.: @code{strlen()} -> @code{strlen}. * There are still some "-*-" list markers in the text. Should these be removed, now you format the list using @itemize? * "The better way is to leave the symbols unresolved" - either it's "a better way" or it's "the best way". * In the example that does "dlopen ("moduleA.dxe", RTLD_GLOBAL);", the example should be split in two, so the text in the middle is formatted normally. * "@strong{Import libraries}" - shouldn't this be an @section? Similarly for other @strong{} usages that are headings. * "(see http://crystal.sourceforge.net)" - use @uref{}, so that the URL appears as a link, when the texinfo is converted to HTML format using makeinfo. Similarly for the other web addresses. * Do the authors really want their e-mail addresses in the texinfo documentation? src/libc/dxe/dlregsym.c: * If the realloc fails, it loses the pointer to the original symbol table _dl_symtabs. realloc does not touch the original memory, if it cannot allocate a new block of the appropriate size. So the original is still valid. Perhaps it should leave the table as it is and just fail, when it cannot reallocate the memory. E.g.: _old_dl_nmaxsymtab = _dl_nmaxsymtab; _old_dl_symtabs = _dl_symtabs; _dl_nmaxsymtab += 8; _dl_symtabs = realloc(...); if (_dl_symtabs == NULL) { _dl_nmaxsymtab = _old_dl_nmaxsymtab; _dl_symtabs = _old_dl_symtabs; return 1; } src/libc/dxe/dlopen.c: * DJGPP has a realpath function now. REALPATH should be defined to realpath. Perhaps this could be done conditionally on __DJGPP__ & __DJGPP_MINOR__, to allow the same code to be used in the DXE3 project. * Why does the code use _chmod in the ACCESS macro? DJGPP's access is symlink-aware now, so perhaps that should be used in the ACCESS macro instead. * FILENAME_MAX includes space for a terminating null. So fname (and some others) doesn't need to be FILENAME_MAX + 1 big. * The code doesn't cope with malloc failing. src/libc/dxe/dlerror.c: * _dl_unresolved_symbol is defined as 128 bytes in size in dlopen.c. The buffer in dlerror is not big enough to hold a symbol name of that size. Perhaps there should be a maximum symbol name length defined somewhere that both files could use? src/libc/dxe/dlopen.txh: * It would be nice to have @ref{} or @xref{} or @pxref{} the functions listed in @code{}, e.g.: dlclose. * Please add a @vindex entry for LD_LIBRARY_PATH. * How about a @ref{} or @xref{} or etc. to dxe3gen? * NULL should be in @code{}, I think. * "void *handle" isn't very texinfo. How about just "handle" instead? * Again, @code{somefunction} doesn't need brackets after somefunction, when you are just naming the function. * "For DJGPP/COFF prepend and underscore in front.": "prepend and" -> "prepend an". * FWIW dlopen and dlclose are in the new POSIX standard. src/libc/dxe/dlerror.txh: * NULL should be in @code{}, I think. * FWIW dlerror is in the new POSIX standard. src/libc/dxe/dlregsym.txh: * Again, @code{somefunction} doesn't need brackets after somefunction, when you are just naming the function. * "If no one is found, the last-resort handler is called." - no one -> none. * Some periods (full-stops) don't have two spaces after them. * "This is a *pointer* to a function" - pointer -> @emph{pointer} or perhaps just remove the asterisks. * "this allows to load modules which you don't know in advance which unresolved symbols it contains" - "modules which you" -> "modules where you". * "that is being called when" -> "that is called when". There is one case where it says "when during" - I think the when needs to go just before "the dynamic loader" in that case. I'm sorry, but I didn't really get a chance to review the code of dxe3gen.c or dxe3res.c or the tests. I also did not check what the DXE loading/unloading/etc. code actually does on a functional level. Some other comments: Why not call the import libraries libsomething.ia instead of libsomething_i.a? .ia works for long & short filenames. It sounds like we should make a DXE version of libc as soon as possible, to allow people to use libc functions in their DXEs without all the horrible problems. I wonder how hard it would be to come up with some support for DXEs in GNU libtool... Anyway, I don't think any of the above comments should stop the code going in DJGPP 2.04 alpha 1. It would be nice if things were fixed after the alpha is released, though... Finally: Thanks! Bye, Rich =] -- Richard Dawe [ http://www.phekda.freeserve.co.uk/richdawe/ ]