Hi, > In the discussion around the error reporting of dlopen() errors when > loading APR database drivers, I have understood that
> * currently, only the fact that a driver failed to load can be > reported back to the caller > * the caller gets APR_EDSOOPEN when dynamic loading failed, but no clue > about the name of the file which caused the error. > At the moment, APU_DBD_DRIVER_FMT describes the format that is used > internally in apr_dbd.c to build the name of the shared object, from > the value of the APU_DSO_LIBDIR macro (which is apu-private from > apr-util/include/private/apu_config.h) and the name of the driver, > by using: > #define APU_DBD_DRIVER_FMT APU_DSO_LIBDIR "/apr_dbd_%s.so" > ... > apr_snprintf(path, sizeof path, APU_DBD_DRIVER_FMT, name); > 1) > This is going to break badly if APR is installed into a directory > that is called, say, > /my/na%sty/apr-lib/ > because then the two %s are both interpreted as string expansion > format chars, and apr will probably dump core. > Suggestion: > use > #define APU_DBD_DRIVER_FMT "%s/apr_dbd_%s.so" > and > apr_snprintf(path, sizeof path, APU_DBD_DRIVER_FMT, APU_DSO_LIBDIR, > name); > this should protect against directories with '%' characters. > 2) the APU_DBD_DRIVER_FMT should IMO not be public at all, but > should be even more local (local to apr_dbd.c) -- its usage semantics > are highly specific to apr_dbd.c > 3) For finding out the name that caused dynamic loading to fail, the > interface of the function doing the loading --apr_dbd_get_driver()-- > should either be extended to allow for returning the absolute path > of the failing shared lib, > or a new function should be added that returns the name of the > shared object in question, when given the same driver name as > apr_dbd_get_driver(), something like > /* Get the absolute path of the driver's shared object */ > apr_status_t apr_dbd_get_driver_dso_path(apr_pool_t *pool, > const char *name, > char **returned_name); > that fails with APR_ENOTIMPL if !defined(APU_DSO_BUILD) > What do you think? I find this is a good approach if we really need APU_DSO_BUILD. When I introduced the APU_DBD_DRIVER_FMT I thought that I could make things simpler; but from your previous and recent comments I see now that there is not such a simple solution possible; also I'm asking me why we need to provide a hardcoded APU_DSO_LIBDIR at all? Another solution which I would find easiest would be if we would just configure the full driver path/name in mod_dbd instead of using a driver alias only; this would stop the ping-pong we would have to do else: - send the driver alias to apr_dbd - apr_dbd creates a not at runtime changeable full path from that and tries to load the driver - on load failure we would have to ask apr_dbd to give us the full driver path back in order to tell the user where we expected the driver if we would configure the full driver path/name in mod_dbd the user would all the time know (since he configured self) where APU expects the driver + mod_dbd could provide a proper error message on load failure; we would then load the DBD driver just as we do with the other (httpd) DSO modules = specify a full or relative path (relative to server root). This way both APU_DBD_DRIVER_FMT and APU_DSO_LIBDIR would become obsolete (unless APU_DSO_LIBDIR is also used for other loads which I'm not aware). I beleive that would provide most flexibility. If there are no more comments or advises I will later this evening revert my changes and remove APU_DBD_DRIVER_FMT, and restore the way it was before where apr_dbd.c has its ifdefs + change the apr_snprinf() for Unix so as Martin suggested. thanks, Guenter.
