Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#1223 closed defect (fixed)

OGRSFDriver::CreateDataSource creates data source with NULL driver

Reported by: amagnum2@… Owned by: Mateusz Łoskot
Priority: normal Milestone: 1.4.2
Component: OGR_SRS Version: unspecified
Severity: normal Keywords:
Cc: warmerdam

Description (last modified by Mateusz Łoskot)

I use CreateDataSource? to create several shape files layers in a folder, or to create tables in PGSQL.

I get an instance of the SHAPE driver, and then I specify in the name paramater the relative path of the folder, then I issue CreateLayer? to create the shape files.

  • First part

When I use OGRSFDriver::CreateDataSource? the driver (fetched with GetDriver?()) is NULL.

  • Second part

When I use OGRSFDriverRegistrar::Open on the same directory with shape files the driver pointer is attached.

What's more interesting is that it still crates the layers properly even though the driver is NULL :)

Ariel

Attachments (4)

open.cpp (576 bytes) - added by Mateusz Łoskot 12 years ago.
Test presenting the second part of explanation in the ticket report
create.cpp (961 bytes) - added by Mateusz Łoskot 12 years ago.
Test presenting the first part of explanation in the ticket report
Makefile (444 bytes) - added by Mateusz Łoskot 12 years ago.
Makefile used to build all tests
create_c.cpp (1.2 KB) - added by Mateusz Łoskot 12 years ago.
This program presents the idea behing the half-fix of this issue.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 12 years ago by warmerdam

Cc: warmerdam added
Description: modified (diff)
Milestone: 1.4.2
Owner: changed from warmerdam to Mateusz Łoskot
Priority: highnormal
Severity: majornormal

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.

comment:2 Changed 12 years ago by Mateusz Łoskot

Description: modified (diff)
Status: newassigned

comment:3 Changed 12 years ago by Mateusz Łoskot

Description: modified (diff)

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!

comment:4 Changed 12 years ago by Mateusz Łoskot

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?

comment:5 Changed 12 years ago by warmerdam

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.

comment:6 Changed 12 years ago by Mateusz Łoskot

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.

comment:7 in reply to:  6 Changed 12 years ago by Mateusz Łoskot

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 ) )

comment:8 Changed 12 years ago by Mateusz Łoskot

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?

comment:9 Changed 12 years ago by Mateusz Łoskot

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

comment:10 Changed 12 years ago by Mateusz Łoskot

Resolution: fixed
Status: assignedclosed

The discussed fix was applied in r11378.

Changed 12 years ago by Mateusz Łoskot

Attachment: open.cpp added

Test presenting the second part of explanation in the ticket report

Changed 12 years ago by Mateusz Łoskot

Attachment: create.cpp added

Test presenting the first part of explanation in the ticket report

Changed 12 years ago by Mateusz Łoskot

Attachment: Makefile added

Makefile used to build all tests

Changed 12 years ago by Mateusz Łoskot

Attachment: create_c.cpp added

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

comment:11 Changed 12 years ago by Mateusz Łoskot

comment:12 Changed 12 years ago by Mateusz Łoskot

Note: See TracTickets for help on using tickets.