OGRSFDriver::CreateDataSource creates data source with NULL driver

Mateusz,

Could you look into this? This may be a general problem with many drivers with regard to setting the driver information when doing a CreateDataSource?(). Depending on the complexity of changes we may put into 1.4 or just in trunk.

Ariel,

I attached two small tests based on your explanation of the problem.

I think I understand the problem correctly, but please could you verify my understanding and tests I prepared?

Here is my run of these tests:

mloskot:~/dev/gdal/bugs/1223$ ./create 
Test: create
NOT OK! Driver is NULL!

mloskot:~/dev/gdal/bugs/1223$ ./open 
Test: open
OK! Driver attached!

Frank,

I found where is the problem.

If a datasource is opened using OGRSFDriverRegistrar::Open() then this function assigns pointer to activated driver to the returned datastore. This works well.

When a datastore is created using OGRSFDriver::CreateDataStore?() interface, what results in call fowarded to <specialized driver>::CreateDataStore?() (ie. OGRShapeDriver::Open() function), then the driver is not assigned to datasource object. This is the problem.

Potentially, the fix is trivial: assign pointer to used driver inside CreateDataStore? call, but this is not possible with current interface of OGRDataSource.

The problem here is that the OGRDataSource::m_poDriver is protected and there is no way to access it inside function like OGRShapeDriver::Open().

In the OGRSFDriverRegistrar::Open() function the situation is different, because the OGRSFDriverRegistrar is a friend of OGRDataSource, so it's possible to access the m_poDriver member directly.

In order to solve this issue, we need to find a way how to set new value to OGRDataSource::m_poDriver from subclasses of OGRSFDriver.

I see following possibilities:

• make the OGRSFDriver a friend of OGRDataSource (simpler, but less elegant)
• add new public function OGRDataSource::SetDriver?(OGRSFDriver* poDriver) what will change the interface (harder, but better designed)

What do you think?

Mateusz,

I could live with the second solution, but the problem with it (in my mind) is that it means that ever driver has to be updated to set this. The nice thing about centralizing the driver setting in OGROpen() is that the drivers don't even have to be aware about it.

Garr.

This is one of the things I hate about the approach of directly overriding methods - it makes it very hard to provide generic level checking and actions before the format specific method gets called.

I'm not sure that I want to have to touch all the drivers. Let me think on this for a bit, unless you have a solution to my dilemma.

Frank,

There is also a modification of 2nd solution possible

The idea is to add new constructor to OGRDataSource class:

OGRDataSource::OGRDataSource(OGRSFDriver* poDriver);

and next it used as follows:

OGRDataSource *OGRShapeDriver::Open( const char * pszFilename,
                                     int bUpdate )

{
    OGRShapeDataSource  *poDS;

    poDS = new OGRShapeDataSource( this ); // HERE

• This version makes the contract stronger that a DS can not be instantiated without assigned driver.

• Also, explicit assignment of driver in OGRSFDriverRegistrar::Open can be removed, because this responsibility could move to the specialized data source class, similarly to the code above:

OGRDataSource *OGRShapeDriver::Open( const char * pszFilename,
                                     int bUpdate )

{
    OGRShapeDataSource  *poDS;

    poDS = new OGRShapeDataSource( this );

I understand this solution may not be as less intrusive as you like, but let's have it discussed and archived.

Replying to mloskot:

The second code snipped is incorrect, certainly.

Here I wanted to update OGRShapeDriver::CreateDataSource?, as follows:

OGRDataSource *OGRShapeDriver::CreateDataSource( const char * pszName,
                                                 char **papszOptions )

{

// ...

    OGRShapeDataSource  *poDS = NULL;

    poDS = new OGRShapeDataSource( this ); // HERE IS THE FIX
    
    if( !poDS->Open( pszName, TRUE, FALSE, bSingleNewFile ) )

Frank,

There is yet another solution, a dirty hack but I think it's as less intrusive for public API as possible. Consider following change in a specialized class of OGRDataSource:

OGRShapeDataSource::OGRShapeDataSource()

{
    pszName = NULL;
    papoLayers = NULL;
    nLayers = 0;
    bSingleNewFile = FALSE;

    m_poDriver = OGRSFDriverRegistrar::GetRegistrar()->GetDriverByName("ESRI Shapefile");

    CPLAssert( NULL != m_poDriver );
}

What do you think?

Here I paste discussion Frank and I had on the IRC:

<FrankW> We could modify the OGR_Dr_CreateDataSource() C API function to
set the driver, and just document the C method as not setting the driver,
so people should be wary.
<FrankW> ...just document the *C++* method...
<mloskot> We don not have access to OGRDataSource::m_poDriver from
public space
<FrankW> We can make OGR_Dr_CreateDataSource() a friend of the class for
this purpose. 
<FrankW> Of, if you prefer add a setter method. 
mloskot> both are possible, though I'd prefer the latter ;-)
<FrankW> ok, fine with me

The discussed fix was applied in r11378.

Test presenting the second part of explanation in the ticket report

Test presenting the first part of explanation in the ticket report

Makefile used to build all tests

This program presents the idea behing the half-fix of this issue.

The OGR C/C++ API manual has been also updated:

• void OGRDataSource::SetDriver(OGRSFDriver *poDriver)
• OGR_Dr_CreateDataSource

and here:

• OGRSFDriver::CreateDataSource()