Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#5169 closed enhancement (fixed)

OGR Walk Driver

Reported by: ermite Owned by: warmerdam
Priority: normal Milestone: 1.11.0
Component: OGR_SF Version: svn-trunk
Severity: normal Keywords: Walk
Cc:

Description

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.

Attachments (7)

ogr-walk-driver20130725.patch (96.4 KB) - added by ermite 8 years ago.
Patch for adding OGR Walk Driver
world.zip (813.6 KB) - added by ermite 8 years ago.
Walk Sample Data
ogr-walk-driver20130726.patch (97.0 KB) - added by ermite 8 years ago.
Patch for adding OGR Walk Driver (Fixed)
ogr-walk-driver20130729.patch (3.4 KB) - added by ermite 8 years ago.
Adding driver documentation plus some minor changes
ogr-walk-driver20130806.patch (6.4 KB) - added by ermite 8 years ago.
Adding mdb tools detection under Linux
ogrodbc20130807.patch (1.0 KB) - added by ermite 8 years ago.
Checking if it is Walk MDB in ODBC driver
ogr-walk-driver20130903.patch (30.9 KB) - added by ermite 8 years ago.
Adding arc/circle translation and 3D data support

Download all attachments as: .zip

Change History (22)

Changed 8 years ago by ermite

Patch for adding OGR Walk Driver

Changed 8 years ago by ermite

Attachment: world.zip added

Walk Sample Data

comment:1 Changed 8 years ago by 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.

comment:2 Changed 8 years ago by 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.

comment:3 Changed 8 years ago by Even Rouault

Milestone: 1.10.12.0

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.

Changed 8 years ago by ermite

Patch for adding OGR Walk Driver (Fixed)

comment:4 Changed 8 years ago by 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;

comment:5 Changed 8 years ago by 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.

Changed 8 years ago by ermite

Adding driver documentation plus some minor changes

comment:6 Changed 8 years ago by Even Rouault

ogr-walk-driver20130729.patch commited

comment:7 Changed 8 years ago by ermite

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

comment:8 Changed 8 years ago by Even Rouault

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

Changed 8 years ago by ermite

Adding mdb tools detection under Linux

comment:9 Changed 8 years ago by ermite

Keywords: Walk added
Version: unspecifiedsvn-trunk

comment:10 Changed 8 years ago by Even Rouault

Resolution: fixed
Status: newclosed

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

Changed 8 years ago by ermite

Attachment: ogrodbc20130807.patch added

Checking if it is Walk MDB in ODBC driver

comment:11 Changed 8 years ago by Even Rouault

ogrodbc20130807.patch patch applied in r26271

Changed 8 years ago by ermite

Adding arc/circle translation and 3D data support

comment:12 Changed 8 years ago by 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.

comment:13 Changed 8 years ago by ermite

Resolution: fixed
Status: closedreopened

comment:14 Changed 8 years ago by Even Rouault

Resolution: fixed
Status: reopenedclosed

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

comment:15 Changed 7 years ago by Even Rouault

Milestone: 2.01.11.0
Note: See TracTickets for help on using tickets.