Opened 14 years ago

Closed 14 years ago

#3407 closed enhancement (fixed)

Allow DSN template for pgeo to be set as a config option

Reported by: gaige Owned by: gaige
Priority: normal Milestone: 1.7.2
Component: OGR_SF Version: svn-trunk
Severity: normal Keywords: PGEO
Cc: warmerdam

Description

Because there are times when you need to override this and would rather not do at compile time, I propose the following:

The PGEO_DRIVER_TEMPLATE configuration option provides a way to replace the existing default string with a new one at run time or from any calling system that needs to change the default driver. This provides for multiple levels of changes to this setting. In my case, I'm checking for multiple drivers and using the one that is most suitable.

-Gaige

==== /ogr/ogrsf_frmts/pgeo/ogrpgeodatasource.cpp
87,89c87,92
<         pszDSN = (char *) CPLMalloc(strlen(pszNewName)+50);
<         sprintf( pszDSN, "DRIVER=Microsoft Access Driver (*.mdb);DBQ=%s", 
<                  pszNewName );
---
> 		const char *pszDSNStringTemplate = NULL;
> 		pszDSNStringTemplate = CPLGetConfigOption( "PGEO_DRIVER_TEMPLATE", "DRIVER=Microsoft Access Driver (*.mdb);DBQ=%s");
> 		if (pszDSNStringTemplate) {
> 			pszDSN = (char *) CPLMalloc(strlen(pszNewName)+strlen(pszDSNStringTemplate)+100);
> 			sprintf( pszDSN, pszDSNStringTemplate,  pszNewName );
> 		}

Change History (11)

comment:1 by warmerdam, 14 years ago

Component: defaultOGR_SF
Keywords: PGEO added

I'm not sure I grasp the need for this. Are there other appropriate odbc drivers for .mdb files on windows?

Note that this change ought to be documented in the PGEO docs.

I have no objection with implementing it.

comment:2 by warmerdam, 14 years ago

Cc: warmerdam added
Owner: changed from warmerdam to gaige

comment:3 by gaige, 14 years ago

On windows, it works fine. On the Macintosh, we don't have built-in support for .mdb files and there is inconsistency in the naming of the drivers due to the existence of multiple drivers that perform the same function.

Originally, I had just used a private version of the file (kept in sync by my source control system) which would assign it to some other driver by default. Unfortunately, during our testing, we found that driver to be buggy and couldn't use it for accessing pgeo databases reliably, so we switched to using a different default.

Clearly, we could just pass a different DSN, but as one of the solutions, we include a copy of an alternate mdb file ODBC driver in our code and wanted to directly access it without requiring it to be installed in the system, which required the use of the direct DRIVER= keyword instead of using the DSN. But, this introduced a different problem, because the user on the Mac is allowed to put an Application anywhere they like on the disk, so the path to the executable (and all of its resources) cannot be determined prior to runtime.

Since the construction of the file path is done inside of the PGeo driver, the only options are to modify how the PGEO driver builds the path or to use a completely different file access method above GDAL for PGeo files, I wanted to do something that solved the runtime determination of the DSN.

In this case, I took a page from the runtime configuration of GDAL_DATA, which allows runtime configuration of the location of various data files, which has worked beautifully for my needs in the past.

This seemed like a good solution, with some precedent in the code elsewhere, for the need to configure at runtime.

Does that make sense? Thanks, -Gaige

comment:4 by warmerdam, 14 years ago

Sounds good to me, go ahead.

comment:5 by gaige, 14 years ago

Resolution: fixed
Status: newclosed

r20429 /trunk/gdal/ogr/ogrsf_frmts/pgeo/ogrpgeodatasource.cpp Allow DSN template for pgeo to be set as a config option (#3407)

comment:6 by Even Rouault, 14 years ago

r20440 /trunk/gdal/ogr/ogrsf_frmts/pgeo/ogrpgeodatasource.cpp: PGeo: add validation of PGEO_DRIVER_TEMPLATE content to avoid crashes

in reply to:  6 comment:7 by gaige, 14 years ago

Replying to rouault:

r20440 /trunk/gdal/ogr/ogrsf_frmts/pgeo/ogrpgeodatasource.cpp: PGeo: add validation of PGEO_DRIVER_TEMPLATE content to avoid crashes

Was this prospective, or did you experience a crash?

Thanks, -Gaige

comment:8 by Even Rouault, 14 years ago

Did you try setting PGEO_DRIVER_TEMPLATE="foo%s%s%s%s" ;-) ?

Yes, this was just a potential vulnerability (but one can argue it is not obvious for the user that the value of this option must be a valid printf formatting string..).

By the way, might be worth documenting the option in drv_pgeo.html unless you want to keep it private for you...

comment:9 by gaige, 14 years ago

Resolution: fixed
Status: closedreopened

Sorry, I didn't mean to come across as defensive, I just wanted to make sure that I didn't miss something when I ran the test suite. Yes, I do think it's a good addition and yes, I will document it in the docs, since the whole purpose of putting it into trunk was to make the system more accessible to folks who don't have Windows systems.

-Gaige

comment:10 by Even Rouault, 14 years ago

Be reassured. I did understand the intent of your question well. I might sound ironic at times. Not always obvious to tune your way of expressing things adequately when you write in a foreign language...

comment:11 by gaige, 14 years ago

Resolution: fixed
Status: reopenedclosed

r20441 /trunk/gdal/ogr/ogrsf_frmts/pgeo/drv_pgeo.html : Add documentation for PGEO_DRIVER_TEMPLATE (#3407)

Note: See TracTickets for help on using tickets.