[OSM-dev] Reimplementation of the GPX importer

[Daniel Silverstone]

On Mon, 2008-09-22 at 23:46 +0100, Jon Burgess wrote:
> > I'd really appreciate any comments anyone might have
> Hope you don't mind lots of comments :-)

The more the better.

> main.c:
> If the GPX_SLEEP_TIME environment variable is not set then getenv()
> returns NULL and causes a segv in atoi().

Well caught, 

> If you have an error talking to the db (i.e. job->error is set on return
> from db_find_work()) then cstart will be uninitialised leading to bogus
> timing information being printed.

Moved initialisation of cstart to top of main loop.

> db.c:
> #define FN(V)
> - Should not be required, free(NULL) is fine.

My brain was clearly frazzled when I wrote that code. Cleaned up.

> mysql.c:
> - May be useful to print mysql_error(handle) when mysql_real_connect()
> fails.

Mmm, would be handy, done so.

> filename.c:
> - Would be good to validate getenv(base) != NULL
> or report error.

Good idea -- WARN and default to '.' if can't find it.

> gpx.c:
> - The old parser used to accept gz, bzip2, tar, zip etc, is this still
> supported?

I couldn't find this in gpx.rb -- is it some magical behaviour of the
ruby libxml binding?!

> In gpx_handle_end_element()
> - It looks like you've made ctx->got_ele mandatory. I'm not sure the old
> code enforced this.

I *thought* it was mandatory, but a reread of the ruby leaves me
confused. I shall make it optional.

> - Looks like you are missing a check of lat/long against +-90/180 limits

D'oh, fixed.

> In gpx_parse_coord()
> - It would be worth checking to see if anyone has uploaded any files
> which use ',' as the decimal separator as is used in some locales. I've
> no idea if this is strictly a legal in a GPX file.

My understanding of the XML schema is that it's not valid, but who knows
for sure. Is there an easy way to grep this? I have no access to the OSM
server filesystems.

> image.c:
> gpx_parse_coord()
> - May want to emit an error if fopen(outfilename, "wb") fails

Nod, done.

> mercator.c:
> mercator_sheet_y()
> - Bad things happen if you try to project 90 degrees of latitude into
> mercator. Clamping the input range to +-85.0511 degrees would fit the
> limits of the normal slippy map. 

Okay, clamps engaged :-)

> interpolate.c:
> interpolate()
> - missing fclose(inputfile) ?

Argh, I am so angry I missed that, Thank you.

That review was excellent and caught many good points.

If someone in the know can clarify how .gz .bz2 etc were all handled
before, then I'll work out what I can do to support it.

> Overall it looks great and a huge performance win over the old code.
> Sorry I forgot to say that earlier, I was getting carried away
> reviewing the code.

As I said, the review was excellent and raised many good points. I am
just embarrassed that I missed so much.

> One last bug:

> interpolate.c: In function ‘interpolate’:
> interpolate.c:101: warning: format ‘%s’ expects type ‘char *’, but argument 3 has type ‘struct FILE *’
> ERROR("Unable to open input file %s (errno=%s)", inputfile, strerror(errno));
> ==> should be inputpath not inputfile.

Fixed.

> I got this warning after updating log.h with:
> extern void _gpxlog(const char *level, const char *fmt, ...)
>         __attribute__ ((format (printf, 2, 3)));

Added to log.h, thanks for that.

Attached is r18 of the codebase. I hope I've handled your points well
enough.

Let's see if we can get the answers to the above, and then move on to
some real-world testing. If I can get hold of a tarball of a bunch of
the GPXes without having to script up something to stress the webserver,
that'd be ace.

D.

-- 
Daniel Silverstone                         http://www.digital-scurf.org/
PGP mail accepted and encouraged.            Key Id: 2BC8 4016 2068 7895

-------------- next part --------------
A non-text attachment was scrubbed...
Name: gpx-import-r18.tar.gz
Type: application/x-compressed-tar
Size: 17418 bytes
Desc: not available
URL: <http://lists.openstreetmap.org/pipermail/dev/attachments/20080923/e9858e8f/attachment.bin>