Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#6729 closed defect (wontfix)

Can ogr_geocoding.cpp avoid calling OGRRegisterAll()?

Reported by: Kurt Schwehr Owned by: Kurt Schwehr
Priority: low Milestone:
Component: default Version: unspecified
Severity: normal Keywords:
Cc:

Description (last modified by Kurt Schwehr)

If there is any way that these sneaky register calls could be removed it would be good for C++ testing and situations where security is important. aka someone (aka me) wants to only link in no or a limited subset of drivers.

I'm trying to work on a my autotest2 C++ setup and having the registers in the unusual files means that all my tests that do not require any drivers must still link in all the drivers and their libs. My hope is to keep the C++ tests as light and fast as possible.

I am trying to create a gdal base bazel (aka not make or cmake) target that only includes the minimum of targets. Then I will have a separate target that brings in the rest of the drivers and their support libraries.

https://trac.osgeo.org/gdal/browser/trunk/gdal/ogr/ogr_geocoding.cpp?annotate=blame#L424

Added in r25373

static OGRLayer* OGRGeocodeGetCacheLayer( OGRGeocodingSessionH hSession,
                                          bool bCreateIfNecessary,
                                          int* pnIdxBlob )
{
    OGRDataSource* poDS = hSession->poDS;
    CPLString osExt = CPLGetExtension(hSession->pszCacheFilename);

    if( poDS == NULL )
    {
        if( OGRGetDriverCount() == 0 )
            OGRRegisterAll();                             //  <<<--- Why?
find . -name \*.cpp | xargs grep OGRRegisterAll | egrep -v 'apps|swig'
./frmts/gdalallregister.cpp:    OGRRegisterAllInternal();
./frmts/mbtiles/mbtilesdataset.cpp:        OGRRegisterAll();
./frmts/pdf/pdfcreatecopy.cpp:        OGRRegisterAll();
./frmts/rasterlite/rasterlitecreatecopy.cpp:        OGRRegisterAll();
./frmts/rasterlite/rasterlitedataset.cpp:        OGRRegisterAll();
./ogr/ogr_geocoding.cpp:            OGRRegisterAll();
./ogr/ogrsf_frmts/generic/ogrregisterall.cpp:/*                           OGRRegisterAll()                           */
./ogr/ogrsf_frmts/generic/ogrregisterall.cpp:void OGRRegisterAll()
./ogr/ogrsf_frmts/generic/ogrregisterall.cpp:void OGRRegisterAllInternal()
./ogr/ogrsf_frmts/generic/ogrregisterall.cpp:} /* OGRRegisterAll */
./ogr/ogrsf_frmts/sqlite/ogrsqlitevirtualogr.cpp:    OGRRegisterAll();
./ogr/ogrutils.cpp: *    OGRRegisterAll();

And it looks like there is one hiding in pdfcreatecopy.cpp.

find . -name \*.cpp | xargs grep GDALAllRegister | egrep -v 'swig|apps|gdalallregister'
./frmts/pdf/pdfcreatecopy.cpp:            GDALAllRegister();
./gcore/gdal_misc.cpp: *    GDALAllRegister();
./gcore/gdaldrivermanager.cpp: * the user a way of unloading undesired drivers.  The GDALAllRegister()
./ogr/ogrsf_frmts/generic/ogrregisterall.cpp:    GDALAllRegister();

Change History (5)

comment:1 Changed 3 years ago by Kurt Schwehr

Status: newassigned
Summary: Can ogr_geocoding.cpp avoid calling?Can ogr_geocoding.cpp avoid calling OGRRegisterAll()?

comment:2 Changed 3 years ago by Kurt Schwehr

Description: modified (diff)

comment:3 Changed 3 years ago by Even Rouault

The

if( OGRGetDriverCount() == 0 )
            OGRRegisterAll();        

looks safe to me. It will only register drivers if there are none. The geocoder needs CSV or SQLite backends to cache geocoded result.

comment:4 Changed 3 years ago by Kurt Schwehr

Resolution: wontfix
Status: assignedclosed

The call is certainly safe. I'm having trouble more with testing w.r.t. isolation and keeping the amount of code linked for a particular test to a minimum. e.g. if I'm writing a test in C++ against the OGR core functions and don't need any drivers, avoiding dragging in OGRRegisterAll() and all the drivers is huge. I do that for all my port/* tests and it greatly reduced the time to do the whole compile, link, run, check results game. When tests get many times an hour (24x7x365), it really starts to add up.

Perhaps something like this would be okay?

#ifdef CPL_NO_REGISTER_IN_LIB
       if( OGRGetDriverCount() == 0 )
       {
           CPLError(CE_Failure, CPLE_AppDefined,
                    "Must register CSV or SQLite before calling OGRGeocodeGetCacheLayer");
           return NULL;
       }
#else
       if( OGRGetDriverCount() == 0 )
            OGRRegisterAll();
#endif  // CPL_NO_REGISTER_IN_LIB

But really, I just removed ogr_geocoding.cpp from my build. Therefor, I will just close this issue. If anyone needs this without the OGRRegisterAll, the idea is here.

comment:5 Changed 3 years ago by Even Rouault

Your above change looks OK to me

Note: See TracTickets for help on using tickets.