OGR Walk Driver

[ermite]

Attached please review the implementation of OGR Walk driver. The driver is used for accessing Walk files, a geospatial data format developed by Walkinfo Tech. mainly for land surveying, evaluation, planning, checking and data analysis in China.

The details of Walkinfo Tech. can be found at: ​http://www.walkinfo.com.cn/en.html

I also attached a sample Walk data for testing(Note that ODBC support is required on Linux). The driver's documentation will follow up soon.

[ermite]

Patch for adding OGR Walk Driver

[ermite]

Walk Sample Data

[Even Rouault]

Hi,

I've identified several issues :

• Conditional compilation is likely not properly done in :
  • ogr/ogrsf_frmts/generic/GNUmakefile (-DWALK_ENABLED should be conditionalized by the availability of ODBC, see how it is done in other ODBC dependant drivers)
  • ogr/ogrsf_frmts/generic/makefile.vc : same remark
  • ogr/ogrsf_frmts/makefile.vc : the 'walk' subdir should be conditionalized, as well as walk\*.obj
• The ByteOrder() inline function is broken. It will always return wkbNDR, even on big endian hosts ( *(&some_var) == some_var on all architectures ). You should rather use #ifdef CPL_LSB to test if the platform is little-endian or not.
• The use of the "unsigned long" type to deserialize the Walk geometry blobs is suspicious. sizeof(unsigned long) varies according to the platforms. It is 4 byte on Linux 32bit, Windows 32bit and Windows 64bit, but it is 8 byte on Linuxs 64bit. I'd rather use GUInt32 (CPL typedef), which is always 4 bytes on all the above platforms. You mentionned that the driver was tested on Linux. I suppose this was Linux 32 bit ? Note: in my experience, the OpenSource MDB ODBC driver isn't ready for 64 bit. So this remark is mainly a provision for the future.
• From a security point of view Binary2WkbMGeom() will do out-of-buffer reads if the geometry blob is not conformant to the Walk specification. Ideally, Binary2WkbMGeom() should have an argument with the buffer size, which should be validated against when unreferencing the p pointer
• Binary2WkbGeom() (and related functions) will not work on big-endian hosts. All places like geom->wkbType = *(unsigned long *)p or memcpy(&geom->point, p, sizeof(WKBPoint)) where you unreference the p pointer would need some byte-swapping for big-endian hosts. There are CPL_LSBPTRxxx() macros in cpl_port.h to make the byte-swapping if necessary. Furthermore stuff like *(unsigned long *)p might cause sigbus on exotic platforms that have strong alignmenet requirements (SPARC processors come to mind). Doing memcpy(&var, p, sizeof(var)) is a safer alternative. For this remark and the previous one, OGRCreateFromShapeBin() in ogr/ogrpgeogeometry.cpp can be a source of inspiration.

[ermite]

I've attached a corrected patch according to the comments. Point 1, 2, 3 and 5 should have been solved. I also added an arument nBytes in Binary2WkbMGeom() and Binery2WkbGeom(). Since the functions are only called in OGRWalkLayer, the assumption is that the data conform our specification. Therefore, only a size checking is done at the beginning of functions. Might have a stricter validation in future version.

[Even Rouault]

I've commited ogr-walk-driver20130726.patch in r26213 with the following changes :

• compilation fix : OGRGeometry* poGeom declaration was missing. Please at least try compiling your contribution for a next time.
• In OGRWalkDriver::Open(), I've uncommented the if( !EQUAL(CPLGetExtension(pszFilename), "MDB") ) test. Otherwise the driver would have tried to open any file with ODBC, which is not appropriate since it would trigger a CPL error. If other file extensions are valid, you should add another kind of test, like detecting in the first bytes of the file if it is a walk file, or something like that

It would be good to have a drv_walk.html documentation page now.

[ermite]

Patch for adding OGR Walk Driver (Fixed)

[ermite]

Thanks Even, the compilation error is due to my misoperation on the patch. I uploaded a fixed-up patch. I did something more with your second change: by default, we assume that a datasource prefixed with a "WALK:" or with .mdb extention will be open. So the test currently used (in OGRWalkDriver.cpp) is as follows:

if( !EQUALN(pszFilename,"WALK:",5)

&& !EQUAL(CPLGetExtension(pszFilename), "MDB") ) return NULL;

[Even Rouault]

Now that I've commited the drivers, please generate your future patches as diff against the subversion HEAD. Your proposed change is equivalent to what I commited yesterday.

[ermite]

Adding driver documentation plus some minor changes

[Even Rouault]

ogr-walk-driver20130729.patch commited

[ermite]

Thanks. Yet I don't find drv_walk.html in the GDAL trunk

[Even Rouault]

Done now. I forgot to 'svn add' the new file.

[ermite]

Adding mdb tools detection under Linux

[Even Rouault]

I've added the same functionnality in r26268 as in ogr-walk-driver20130806.patch but by factoring code instead of copy&pasting it.

[ermite]

Checking if it is Walk MDB in ODBC driver

[Even Rouault]

ogrodbc20130807.patch patch applied in r26271

[ermite]

Adding arc/circle translation and 3D data support

[ermite]

OGR Walk Driver updates on 3 September 2013:

1. Adding arc(3-point) and circle(5-point) translation in OGRWalkTool.cpp;
2. Rewriting the tranlation codes in OGRWalkTool.cpp. The Walk geometries are directly translated into OGR geometries (ByteOrder() function is no longer used), 3D data are partly supported;
3. Some minor changes;
4. Updating the driver documentations.

[Even Rouault]

r26421 "Walk: apply ogr-walk-driver20130903.patch (#5169)"