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 p...@lysator.liu.se
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 p...@lysator.liu.se
--
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 windows.h
+#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
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 ltdl.h
#include stdio.h
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;
}
else if
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.
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.
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 p...@lysator.liu.se 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 p...@lysator.liu.se
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;
}
}
