Re: Report proper errors from the loadlibrary loader.
Hi Ralf, Den 2010-01-20 22:15 skrev Ralf Wildenhues: * Peter Rosin wrote on Sun, Jan 17, 2010 at 09:19:27PM CET: The \r\n at the end of the messages should probably be removed. Sounds like a good idea to me. I have since added that to the loadlibraryerror function. And I did manage to write a test that exposes this new functionallity - as you requested - by unloading all modules except the loadlibrary loader (and the preopen loader). The test is attached. Good. Thanks. I can't find obvious issues with the test, and it should skip on all unixy systems (due to loadlibrary loader not being present). I have checked that it indeed skips on linux due to no loadlibrary loader. But, I have not succeeded in generating any other errors than the above two. I have tried to remove execute bits on the plugin and a dependent library, and I have tried removing a dependent library. But real problems usually manifests themselves in weird ways and the correct error message may be crucial in some unforseen case. Is it worth it? We can cross that bridge when we get to it, i.e., when we have a problem report with a setup that provoke such an error. The test case is good because it ensures several code paths are covered in the source. If it works on MiNGW (I gather from above that you've already tested Cygwin and Wine) then feel free to commit both patch and test. I did some minor tweaks to the test after going through the following systems, listing the test result after the tweaks: MSYS/MinGWok Wine/MinGWok Wine/MinGW --disable-static skip (empty $dlname in the .la file) Cygwin/gccok linux/gcc skip (no loadlibrary loader) So, I'm pushing as attached. Thanks for the review! Cheers, Peter 2010-01-20 Peter Rosin Report proper errors from the loadlibrary loader. * libltdl/loaders/loadlibrary.c (loadlibraryerror): New helper function that returns the latest Windows error as a string, or the provided default string on failure to do so. (LOADLIB_SETERROR): New macro that wraps previous to make it easy to use. (vm_open, vm_close, vm_sym): Make use of previous. (LOCALFREE): New macro to help free the Windows error string. (vl_exit): Make use of previous. * tests/loadlibarry.at: New file, new test that makes sure the loadlibrary loader reports non-standard error messages. * Makefile.am (TESTSUITE_AT): Add above test. Signed-off-by: Peter Rosin -- They are in the crowd with the answer before the question. > Why do you dislike Jeopardy? diff --git a/Makefile.am b/Makefile.am index 8d6682b..40a0138 100644 --- a/Makefile.am +++ b/Makefile.am @@ -485,6 +485,7 @@ TESTSUITE_AT= tests/testsuite.at \ tests/lt_dlopen_a.at \ tests/lt_dlopenext.at \ tests/ltdl-api.at \ + tests/loadlibrary.at \ tests/lalib-syntax.at \ tests/resident.at \ tests/slist.at \ diff --git a/libltdl/loaders/loadlibrary.c b/libltdl/loaders/loadlibrary.c index 40435fe..139851a 100644 --- a/libltdl/loaders/loadlibrary.c +++ b/libltdl/loaders/loadlibrary.c @@ -98,12 +98,19 @@ get_vtable (lt_user_data loader_data) #include +#define LOCALFREE(mem) LT_STMT_START { \ + if (mem) { LocalFree ((void *)mem); mem = NULL; }} LT_STMT_END +#define LOADLIB__SETERROR(errmsg) LT__SETERRORSTR (loadlibraryerror (errmsg)) +#define LOADLIB_SETERROR(errcode) LOADLIB__SETERROR (LT__STRERROR (errcode)) + +static const char *loadlibraryerror (const char *default_errmsg); static UINT WINAPI wrap_geterrormode (void); static UINT WINAPI fallback_geterrormode (void); typedef UINT (WINAPI geterrormode_type) (void); static geterrormode_type *geterrormode = wrap_geterrormode; +static char *error_message = 0; /* A function called through the vtable when this loader is no @@ -112,6 +119,7 @@ static int vl_exit (lt_user_data LT__UNUSED loader_data) { vtable = NULL; + LOCALFREE (error_message); return 0; } @@ -213,7 +221,9 @@ vm_open (lt_user_data LT__UNUSED loader_data, const char *filename, } } -if (cur || !module) +if (!module) + LOADLIB_SETERROR (CANNOT_OPEN); +else if (cur) { LT__SETERROR (CANNOT_OPEN); module = 0; @@ -231,9 +241,9 @@ vm_close (lt_user_data LT__UNUSED loader_data, lt_module module) { int errors = 0; - if (FreeLibrary((HMODULE) module) == 0) + if (FreeLibrary ((HMODULE) module) == 0) { - LT__SETERROR (CANNOT_CLOSE); + LOADLIB_SETERROR (CANNOT_CLOSE); ++errors; } @@ -250,7 +260,7 @@ vm_sym (lt_user_data LT__UNUSED loader_data, lt_module module, const char *name) if (!address) { - LT__SETERROR (SYMBOL_NOT_FOUND); + LOADLIB_SETERROR
Re: Report proper errors from the loadlibrary loader.
Hi Peter, * Peter Rosin wrote on Sun, Jan 17, 2010 at 09:19:27PM CET: > Den 2010-01-17 17:45 skrev Bob Friesenhahn: > >On Sun, 17 Jan 2010, Ralf Wildenhues wrote: > >> > >>I'm not too fond of adding new static storage and manipulation (yet > >>another reason lt_dlopen may not be called concurrently from different > >>threads), or improved functionality without testsuite exposure that we > >>really improved. Further, the documentation I found about GetLastError > >>states W2K as minimum version, so I hope that you checked that this > >>works with older Windows as well. Otherwise, I am fine with the patch. > > > >It seems that GraphicsMagick has been using GetLastError() for a > >very long time. Certainly at least since Y2K. There is no > >special provision in this part of the code for anything older than > >Windows XP and Windows 2000. Then again, practically no one cares > >about Windows versions older than this. > > I think what has happened is that MS has abandoned WinNT and older and > are thus ignoring them in MSDN. GetLastError certainly predates Win2k > and I think it was in Win32 from from very beginning. OK, that settles that then. * Peter Rosin wrote on Mon, Jan 18, 2010 at 03:24:27PM CET: > Going back to what this actually brings us... > > Previously we had reported errors of "can't open the module" and "symbol not > found". With the patch that turns into "The specified module could not be > found.\r\n" and "The specified procedure could not be found." (WinXP) or > "Module not found\r\n" and "Procedure not found\r\n" (Wine). > > The \r\n at the end of the messages should probably be removed. Sounds like a good idea to me. > And I did > manage to write a test that exposes this new functionallity - as you > requested - by unloading all modules except the loadlibrary loader (and > the preopen loader). The test is attached. Good. Thanks. I can't find obvious issues with the test, and it should skip on all unixy systems (due to loadlibrary loader not being present). > But, I have not succeeded in generating any other errors than the above > two. I have tried to remove execute bits on the plugin and a dependent > library, and I have tried removing a dependent library. But real problems > usually manifests themselves in weird ways and the correct error message > may be crucial in some unforseen case. Is it worth it? We can cross that bridge when we get to it, i.e., when we have a problem report with a setup that provoke such an error. The test case is good because it ensures several code paths are covered in the source. If it works on MiNGW (I gather from above that you've already tested Cygwin and Wine) then feel free to commit both patch and test. Thanks! Ralf
Re: Report proper errors from the loadlibrary loader.
Den 2010-01-17 10:31 skrev Ralf Wildenhues: What does this patch and its followup fix get us? What behavior changed in relation to previous code, and if this is fixing a bug, is there need and chance to test for it? Previously the reported error was a plain "can't open the module" or "symbol not found", even though the system might have reported why it could not open the library or not find the symbol. I would say it's about the same as using dlerror when present in the dlopen loader. Ah, ok. Thanks. I'm not too fond of adding new static storage and manipulation (yet another reason lt_dlopen may not be called concurrently from different threads), or improved functionality without testsuite exposure that we really improved. Further, the documentation I found about GetLastError states W2K as minimum version, so I hope that you checked that this works with older Windows as well. Otherwise, I am fine with the patch. Going back to what this actually brings us... Previously we had reported errors of "can't open the module" and "symbol not found". With the patch that turns into "The specified module could not be found.\r\n" and "The specified procedure could not be found." (WinXP) or "Module not found\r\n" and "Procedure not found\r\n" (Wine). The \r\n at the end of the messages should probably be removed. And I did manage to write a test that exposes this new functionallity - as you requested - by unloading all modules except the loadlibrary loader (and the preopen loader). The test is attached. But, I have not succeeded in generating any other errors than the above two. I have tried to remove execute bits on the plugin and a dependent library, and I have tried removing a dependent library. But real problems usually manifests themselves in weird ways and the correct error message may be crucial in some unforseen case. Is it worth it? Cheers, Peter -- They are in the crowd with the answer before the question. > Why do you dislike Jeopardy? # loadlibrary.at -- test loadlibrary functionality -*- Autotest -*- # # Copyright (C) 2010 Free Software Foundation, Inc. # This file is part of GNU Libtool. # # GNU Libtool is free software; you can redistribute it and/or # modify it under the terms of the GNU General Public License as # published by the Free Software Foundation; either version 2 of # the License, or (at your option) any later version. # # GNU Libtool is distributed in the hope that it will be useful, # but WITHOUT ANY WARRANTY; without even the implied warranty of # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the # GNU General Public License for more details. # # You should have received a copy of the GNU General Public License # along with GNU Libtool; see the file COPYING. If not, a copy # can be downloaded from http://www.gnu.org/licenses/gpl.html, # or obtained by writing to the Free Software Foundation, Inc., # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. AT_SETUP([loadlibrary error messages]) AT_KEYWORDS([libltdl]) AT_DATA([main.c], [[#include #include static int standard_error_message(const char *error) { int error_number; for (error_number = 0; error_number < LT_ERROR_MAX; ++error_number) { lt_dlseterror (error_number); if (error == lt_dlerror ()) { return 1; } } lt_dlseterror (LT_ERROR_UNKNOWN); return 0; } int main (int argc, char* argv[]) { int err = 0; lt_dlhandle module = NULL; const lt_dlvtable *loadlibrary; const lt_dlvtable *preopen; lt_dlloader *loader = NULL; lt_dlloader *next; const lt_dlvtable *vtable; void *symbol; const char *error; if (argc < 2) { fprintf (stderr, "usage: %s plugin [symbol]\n", argv[0]); return 1; } lt_dlinit (); loadlibrary = lt_dlloader_find ("lt_loadlibrary"); if (!loadlibrary) { /* Skip if the loadlibrary loader isn't supported */ printf ("loadlibrary loader not found\n"); err = 77; goto cleanup; } preopen = lt_dlloader_find ("lt_preopen"); if (!loadlibrary) { printf ("preopen loader not found\n"); err = 2; goto cleanup; } /* Remove all loaders except the preopen and loadlibrary loaders. */ while (next = lt_dlloader_next (loader)) { if (lt_dlloader_get (next) == loadlibrary) { loader = next; continue; } if (lt_dlloader_get (next) == preopen) { loader = next; continue; } lt_dlloader_remove (lt_dlloader_get (next)->name); } module = lt_dlopen (argv[1]); error = lt_dlerror (); if (module) { printf ("lt_dlopen: success!\n"); if (argc == 2) { /* But failure was the desired result... */ err = 2; goto cleanup; } } else if (argc > 2) { /* Didn't expect failure... */ printf ("lt_dlopen: failure: %s\n", error); err = 2; goto cleanup;
Re: Report proper errors from the loadlibrary loader.
Den 2010-01-17 17:45 skrev Bob Friesenhahn: On Sun, 17 Jan 2010, Ralf Wildenhues wrote: I'm not too fond of adding new static storage and manipulation (yet another reason lt_dlopen may not be called concurrently from different threads), or improved functionality without testsuite exposure that we really improved. Further, the documentation I found about GetLastError states W2K as minimum version, so I hope that you checked that this works with older Windows as well. Otherwise, I am fine with the patch. It seems that GraphicsMagick has been using GetLastError() for a very long time. Certainly at least since Y2K. There is no special provision in this part of the code for anything older than Windows XP and Windows 2000. Then again, practically no one cares about Windows versions older than this. I think what has happened is that MS has abandoned WinNT and older and are thus ignoring them in MSDN. GetLastError certainly predates Win2k and I think it was in Win32 from from very beginning. Cheers, Peter -- They are in the crowd with the answer before the question. > Why do you dislike Jeopardy?
Re: Report proper errors from the loadlibrary loader.
On Sun, 17 Jan 2010, Ralf Wildenhues wrote: I'm not too fond of adding new static storage and manipulation (yet another reason lt_dlopen may not be called concurrently from different threads), or improved functionality without testsuite exposure that we really improved. Further, the documentation I found about GetLastError states W2K as minimum version, so I hope that you checked that this works with older Windows as well. Otherwise, I am fine with the patch. It seems that GraphicsMagick has been using GetLastError() for a very long time. Certainly at least since Y2K. There is no special provision in this part of the code for anything older than Windows XP and Windows 2000. Then again, practically no one cares about Windows versions older than this. It is necessary to collect and store the error as soon as possible since any additional activity might invoke a new system call, which results in an unrelated error being reported by GetLastError(). Bob -- Bob Friesenhahn bfrie...@simple.dallas.tx.us, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer,http://www.GraphicsMagick.org/
Re: Report proper errors from the loadlibrary loader.
Hi Peter, * Peter Rosin wrote on Mon, Jan 11, 2010 at 09:16:37PM CET: > Den 2010-01-04 21:48 skrev Ralf Wildenhues: > >* Peter Rosin wrote on Sat, Jan 02, 2010 at 04:02:57AM CET: > >>Please consider the attached patch. > >> > >>I'm just about to go skiing for a week or so, so I'll push when I get > >>back if this patch is blessed (knock wood) after I leave... > > > >Well happy new year and hope you had a fun time in the snow! > > That I did, and so did the kids, the weather was superb, a bit on the > cold side though (-20 to -30 degrees C or so). But sunny (the sky was > clear), no wind and very dry so therefore not unbearable... Sounds nice! > >What does this patch and its followup fix get us? What behavior changed > >in relation to previous code, and if this is fixing a bug, is there need > >and chance to test for it? > > Previously the reported error was a plain "can't open the module" or > "symbol not found", even though the system might have reported why it > could not open the library or not find the symbol. I would say it's > about the same as using dlerror when present in the dlopen loader. Ah, ok. Thanks. I'm not too fond of adding new static storage and manipulation (yet another reason lt_dlopen may not be called concurrently from different threads), or improved functionality without testsuite exposure that we really improved. Further, the documentation I found about GetLastError states W2K as minimum version, so I hope that you checked that this works with older Windows as well. Otherwise, I am fine with the patch. Thanks, Ralf
Re: Report proper errors from the loadlibrary loader.
Den 2010-01-04 21:48 skrev Ralf Wildenhues: Hi Peter, * Peter Rosin wrote on Sat, Jan 02, 2010 at 04:02:57AM CET: Please consider the attached patch. I'm just about to go skiing for a week or so, so I'll push when I get back if this patch is blessed (knock wood) after I leave... Well happy new year and hope you had a fun time in the snow! That I did, and so did the kids, the weather was superb, a bit on the cold side though (-20 to -30 degrees C or so). But sunny (the sky was clear), no wind and very dry so therefore not unbearable... What does this patch and its followup fix get us? What behavior changed in relation to previous code, and if this is fixing a bug, is there need and chance to test for it? Previously the reported error was a plain "can't open the module" or "symbol not found", even though the system might have reported why it could not open the library or not find the symbol. I would say it's about the same as using dlerror when present in the dlopen loader. It would be possible to test if the reported error is not "can't open the module" / "symbol not found", under the assumption that it is un likely for Microsoft/Wine to have used those exact words and case, but that would require some way to check if the used loader was in fact the loadlibrary loader and skip if not. I don't know how to do the last part (Cygwin uses .dll for its dlopen loader, so that check is a no-no). (honest questions, not trying to criticise the patch in any way) No problem, and cheers, Peter -- They are in the crowd with the answer before the question. > Why do you dislike Jeopardy?
Re: Report proper errors from the loadlibrary loader.
Hi Peter, * Peter Rosin wrote on Sat, Jan 02, 2010 at 04:02:57AM CET: > Please consider the attached patch. > > I'm just about to go skiing for a week or so, so I'll push when I get > back if this patch is blessed (knock wood) after I leave... Well happy new year and hope you had a fun time in the snow! What does this patch and its followup fix get us? What behavior changed in relation to previous code, and if this is fixing a bug, is there need and chance to test for it? (honest questions, not trying to criticise the patch in any way) Thanks, Ralf > ChangeLog: > > 2010-01-02 Peter Rosin > > Report proper errors from the loadlibrary loader. > * libltdl/loaders/loadlibrary.c: Update copyright years. > (loadlibraryerror): New helper function that returns the > latest Windows error as a string, or the provided default > string on failure to do so. > (LOADLIB_SETERROR): New macro that wraps previous to make it > easy to use. > (vm_open, vm_close, vm_sym): Make use of previous. > (LOCALFREE): New macro to help free the Windows error string. > (vl_exit): Make use of previous.
Re: Report proper errors from the loadlibrary loader.
Den 2010-01-02 04:02 skrev Peter Rosin: Hi! Please consider the attached patch. I'm just about to go skiing for a week or so, so I'll push when I get back if this patch is blessed (knock wood) after I leave... Here a followup patch that takes care of a problem with the error reporting patch in the previous message. Cheers, Peter 2010-01-02 Peter Rosin Handle simulated errors correctly. * libltdl/loaders/loadlibrary.c (vm_open): Don't go fishing for a Windows error message when the error is simulated. diff --git a/libltdl/loaders/loadlibrary.c b/libltdl/loaders/loadlibrary.c index 3c08f2e..3e65212 100644 --- a/libltdl/loaders/loadlibrary.c +++ b/libltdl/loaders/loadlibrary.c @@ -218,9 +218,11 @@ vm_open (lt_user_data LT__UNUSED loader_data, const char *filename, } } -if (cur || !module) +if (!module) + LOADLIB_SETERROR (CANNOT_OPEN); +else if (cur) { -LOADLIB_SETERROR (CANNOT_OPEN); +LT__SETERROR (CANNOT_OPEN); module = 0; } }
Report proper errors from the loadlibrary loader.
Hi! Please consider the attached patch. I'm just about to go skiing for a week or so, so I'll push when I get back if this patch is blessed (knock wood) after I leave... Cheers, Peter ChangeLog: 2010-01-02 Peter Rosin Report proper errors from the loadlibrary loader. * libltdl/loaders/loadlibrary.c: Update copyright years. (loadlibraryerror): New helper function that returns the latest Windows error as a string, or the provided default string on failure to do so. (LOADLIB_SETERROR): New macro that wraps previous to make it easy to use. (vm_open, vm_close, vm_sym): Make use of previous. (LOCALFREE): New macro to help free the Windows error string. (vl_exit): Make use of previous. diff --git a/libltdl/loaders/loadlibrary.c b/libltdl/loaders/loadlibrary.c index 97fddf4..3c08f2e 100644 --- a/libltdl/loaders/loadlibrary.c +++ b/libltdl/loaders/loadlibrary.c @@ -1,7 +1,7 @@ /* loader-loadlibrary.c -- dynamic linking for Win32 Copyright (C) 1998, 1999, 2000, 2004, 2005, 2006, - 2007, 2008 Free Software Foundation, Inc. + 2007, 2008, 2010 Free Software Foundation, Inc. Written by Thomas Tanner, 1998 NOTE: The canonical source of this file is maintained with the @@ -98,12 +98,23 @@ get_vtable (lt_user_data loader_data) #include +#define LOCALFREE(mem) LT_STMT_START { \ + if (mem) { LocalFree ((void *)mem); mem = NULL; }} LT_STMT_END +#define LOADLIB__SETERROR(errmsg) LT__SETERRORSTR (loadlibraryerror (errmsg)) +#define LOADLIB_SETERROR(errcode) LOADLIB__SETERROR (LT__STRERROR (errcode)) + +static const char *loadlibraryerror (const char *default_errmsg); + +static char *error_message = 0; + + /* A function called through the vtable when this loader is no longer needed by the application. */ static int vl_exit (lt_user_data LT__UNUSED loader_data) { vtable = NULL; + LOCALFREE (error_message); return 0; } @@ -209,7 +220,7 @@ vm_open (lt_user_data LT__UNUSED loader_data, const char *filename, if (cur || !module) { -LT__SETERROR (CANNOT_OPEN); +LOADLIB_SETERROR (CANNOT_OPEN); module = 0; } } @@ -225,9 +236,9 @@ vm_close (lt_user_data LT__UNUSED loader_data, lt_module module) { int errors = 0; - if (FreeLibrary((HMODULE) module) == 0) + if (FreeLibrary ((HMODULE) module) == 0) { - LT__SETERROR (CANNOT_CLOSE); + LOADLIB_SETERROR (CANNOT_CLOSE); ++errors; } @@ -244,8 +255,32 @@ vm_sym (lt_user_data LT__UNUSED loader_data, lt_module module, const char *name) if (!address) { - LT__SETERROR (SYMBOL_NOT_FOUND); + LOADLIB_SETERROR (SYMBOL_NOT_FOUND); } return address; } + + + +/* --- HELPER FUNCTIONS --- */ + + +/* Return the windows error message, or the passed in error message on + failure. */ +static const char * +loadlibraryerror (const char *default_errmsg) +{ + LOCALFREE (error_message); + + FormatMessageA (FORMAT_MESSAGE_ALLOCATE_BUFFER | + FORMAT_MESSAGE_FROM_SYSTEM | + FORMAT_MESSAGE_IGNORE_INSERTS, + NULL, + GetLastError (), + 0, + (char *) &error_message, + 0, NULL); + + return error_message ? error_message : default_errmsg; +}