[GRASS-dev] About the vector changes

Glynn_Clements1 · August 6, 2009, 3:31pm

Vector maps no longer work:

$ v.info fields
ERROR: Spatial index was written with LFS but this GRASS version does not
support LFS. Try to rebuild topology or upgrade GRASS.

Is this intentional? If so, can existing maps be converted, or do I
need to download new versions?

BTW, spidx_port.off_t_size is 42, so the issue is that the code
expects the new format, not that sizeof(off_t) is wrong.

Also, lib/vector/diglib doesn't actually enable LFS; the Makefile
needs:

ifneq ($(USE_LARGEFILES),)
EXTRA_CFLAGS = -D_FILE_OFFSET_BITS=64
endif

--
Glynn Clements <glynn@gclements.plus.com>

Markus_GRASS · August 6, 2009, 4:47pm

Glynn Clements wrote:

Vector maps no longer work:

$ v.info fields
ERROR: Spatial index was written with LFS but this GRASS version does not
support LFS. Try to rebuild topology or upgrade GRASS.

Can't reproduce. This was fields from spearfish I assume. Without building topo I get

ERROR: Unable to open vector map <fields@PERMANENT> on level 2. Try to rebuild vector topology by v.build.

So I do
v.build map=fields
v.info fields now produces standard output

Is this intentional? If so, can existing maps be converted, or do I
need to download new versions?

This should only happen for very large vectors where off_t of size 8 is needed to read/write the sidx file which is obviously not the case for fields. Ideally this error would go away if you rebuild topo.

BTW, spidx_port.off_t_size is 42, so the issue is that the code
expects the new format, not that sizeof(off_t) is wrong.

Weird, spidx_port.off_t_size should be 4 or 8, and 8 is only used when necessary, same like for reading/writing topo. I haven't included some debug info on spidx_port.off_t_size, so I guess you have added that yourself? Maybe a good idea, it's already there in dig_Rd_Plus_head() for reading topo. Can you also report sizeof(off_t)? That could help me tracking down the bug.

Also, lib/vector/diglib doesn't actually enable LFS; the Makefile
needs:

ifneq ($(USE_LARGEFILES),)
EXTRA_CFLAGS = -D_FILE_OFFSET_BITS=64
endif

No, because this is set system-wide in Grass.make
https://trac.osgeo.org/grass/browser/grass/trunk/include/Make/Grass.make#L82
because not only the library but also all modules including vector.h must have 64bit enabled. Instead of adding the appropriate lines to each single Makefile I decided to set it system-wide, that also avoids problems with addon modules. It worked so far on Linux 32bit and Linux 64bit. What platform are you using? I hesitate to ask, but have you made funny changes to your local copy?

A bit confused,

Markus M

Glynn_Clements1 · August 6, 2009, 7:48pm

Markus Metz wrote:

> Vector maps no longer work:
>
> $ v.info fields
> ERROR: Spatial index was written with LFS but this GRASS version does not
> support LFS. Try to rebuild topology or upgrade GRASS.
>
Can't reproduce. This was fields from spearfish I assume.

Yes.

Without building topo I get

ERROR: Unable to open vector map <fields@PERMANENT> on level 2. Try to
rebuild vector topology by v.build.

So I do
v.build map=fields
v.info fields now produces standard output

Running v.build requires that you own the mapset which contains it.

g.copy produces the same error, but it does appear to copy the map.
Running v.build on the copy results in a working map.

> BTW, spidx_port.off_t_size is 42, so the issue is that the code
> expects the new format, not that sizeof(off_t) is wrong.

Weird, spidx_port.off_t_size should be 4 or 8, and 8 is only used when
necessary, same like for reading/writing topo. I haven't included some
debug info on spidx_port.off_t_size, so I guess you have added that
yourself? Maybe a good idea, it's already there in dig_Rd_Plus_head()
for reading topo. Can you also report sizeof(off_t)? That could help me
tracking down the bug.

sizeof(off_t) is 8.

FWIW, the complete structure is:

Breakpoint 1, G_fatal_error (
msg=0xb7f38128 "Spatial index was written with LFS but this GRASS version does not support LFS. Try to rebuild topology or upgrade GRASS.")
at error.c:151
151 va_start(ap, msg);

where

#0 G_fatal_error (
    msg=0xb7f38128 "Spatial index was written with LFS but this GRASS version does not support LFS. Try to rebuild topology or upgrade GRASS.")
    at error.c:151
#1 0xb7f332d6 in dig_Rd_spidx_head (fp=0xbfdb6920, ptr=0xbfdb672c)
    at spindex_rw.c:280
#2 0xb7fd93e2 in Vect_open_sidx (Map=0xbfdb6720, mode=0) at open.c:854
#3 0xb7fd7f99 in Vect__open_old (Map=0xbfdb6720, name=0x8e827a0 "fields",
    mapset=0x804af50 "", update=0, head_only=1) at open.c:262
#4 0xb7fd87a8 in Vect_open_old_head (Map=0xbfdb6720,
    name=0x8e827a0 "fields", mapset=0x804af50 "") at open.c:501
#5 0x08049860 in main (argc=3, argv=0xbfdb6cf4) at main.c:104

frame 1

#1 0xb7f332d6 in dig_Rd_spidx_head (fp=0xbfdb6920, ptr=0xbfdb672c)
at spindex_rw.c:280
280 G_fatal_error("Spatial index was written with LFS but this "

print *ptr

$1 = {Version_Major = 5, Version_Minor = 0, Back_Major = 5, Back_Minor = 0,
  spidx_Version_Major = 5, spidx_Version_Minor = 0, spidx_Back_Major = 5,
  spidx_Back_Minor = 0, cidx_Version_Major = 0, cidx_Version_Minor = 0,
  cidx_Back_Major = 0, cidx_Back_Minor = 0, with_z = 0, spidx_with_z = 0,
  off_t_size = 4, head_size = 142, spidx_head_size = 0, cidx_head_size = 0,
  release_support = 0, port = {byte_order = 0, off_t_size = 0,
    dbl_cnvrt = "\000\001\002\003\004\005\006\a",
    flt_cnvrt = "\000\001\002\003", lng_cnvrt = "\000\001\002\003",
    int_cnvrt = "\000\001\002\003", shrt_cnvrt = "\000\001",
    off_t_cnvrt = "\000\001\002\003\004\005\006\a", dbl_quick = 1,
    flt_quick = 1, lng_quick = 1, int_quick = 1, shrt_quick = 1,
    off_t_quick = 1}, spidx_port = {byte_order = 0, off_t_size = 42,
    dbl_cnvrt = "\000\000\000\000\000\000\000", flt_cnvrt = "\000\000\000",
    lng_cnvrt = "\000\000\000", int_cnvrt = "\000\000\000",
    shrt_cnvrt = "\000", off_t_cnvrt = "\000\000\000\000\000\000\000",
    dbl_quick = 0, flt_quick = 0, lng_quick = 0, int_quick = 0,
    shrt_quick = 0, off_t_quick = 0}, cidx_port = {byte_order = 0,
    off_t_size = 0, dbl_cnvrt = "\000\000\000\000\000\000\000",
    flt_cnvrt = "\000\000\000", lng_cnvrt = "\000\000\000",
    int_cnvrt = "\000\000\000", shrt_cnvrt = "\000",
    off_t_cnvrt = "\000\000\000\000\000\000\000", dbl_quick = 0,
    flt_quick = 0, lng_quick = 0, int_quick = 0, shrt_quick = 0,
    off_t_quick = 0}, mode = 0, built = 0, box = {N = 4928110, S = 4913925,
    E = 609544.37, W = 589438.87, T = 0, B = 0}, Node = 0x0, Line = 0x0,
  Area = 0x0, Isle = 0x0, n_nodes = 217, n_edges = 0, n_lines = 271,
  n_areas = 65, n_isles = 11, n_faces = 0, n_volumes = 0, n_holes = 0,
  n_plines = 0, n_llines = 0, n_blines = 208, n_clines = 63, n_flines = 0,
  n_klines = 0, n_vfaces = 0, n_hfaces = 0, alloc_nodes = 0, alloc_edges = 0,
  alloc_lines = 0, alloc_areas = 0, alloc_isles = 0, alloc_faces = 0,
  alloc_volumes = 0, alloc_holes = 0, Node_offset = 142, Edge_offset = 0,
  Line_offset = 8314, Area_offset = 20157, Isle_offset = 24417,
  Volume_offset = 0, Hole_offset = 0, Spidx_built = 0, Spidx_new = 0,
  spidx_fp = {file = 0x8e830f0, start = 0x0, current = 0x0, end = 0x0,
    size = 0, alloc = 0, loaded = 0}, spidx_node_fname = 0x0,
  Node_spidx_offset = 0, Line_spidx_offset = 0, Area_spidx_offset = 0,
  Isle_spidx_offset = 0, Face_spidx_offset = 0, Volume_spidx_offset = 0,
  Hole_spidx_offset = 0, Node_spidx = 0x8e834e8, Line_spidx = 0x8e83740,
  Area_spidx = 0x8e83998, Isle_spidx = 0x8e83bf0, Face_spidx = 0x0,
  Volume_spidx = 0x0, Hole_spidx = 0x0, update_cidx = 0, n_cidx = 0,
  a_cidx = 5, cidx = 0x8e83328, cidx_up_to_date = 0, coor_size = 27905,
  coor_mtime = 0, do_uplist = 0, uplines = 0x0, alloc_uplines = 0,
  n_uplines = 0, upnodes = 0x0, alloc_upnodes = 0, n_upnodes = 0}

Other than off_t_size = 42, ptr->spidx_port is all zeros.

> Also, lib/vector/diglib doesn't actually enable LFS; the Makefile
> needs:
>
> ifneq ($(USE_LARGEFILES),)
> EXTRA_CFLAGS = -D_FILE_OFFSET_BITS=64
> endif
>
No, because this is set system-wide in Grass.make
https://trac.osgeo.org/grass/browser/grass/trunk/include/Make/Grass.make#L82

Okay; I missed that part.

That's likely to cause problems for anything which uses $(VECT_CFLAGS)
but which isn't itself 64-bit aware (i.e. uses fseek/ftell, calculates
offsets as int/long).

because not only the library but also all modules including vector.h
must have 64bit enabled.

That means all such modules must be made 64-bit safe. Without
_FILE_OFFSET_BITS=64, open(), fopen() etc will simply refuse to open
files larger than 2GiB. If you set _FILE_OFFSET_BITS=64 in code which
wasn't designed for it, ftell() will fail once you get past the 2GiB
mark, storing the result of lseek() in a long will silently truncate
it, etc.

This change has the potential to corrupt files, so it needs to be
completed ASAP.

Instead of adding the appropriate lines to each
single Makefile I decided to set it system-wide, that also avoids
problems with addon modules. It worked so far on Linux 32bit and Linux
64bit. What platform are you using?

Linux/x86 (32-bit).

I hesitate to ask, but have you made funny changes to your local
copy?

No.

--
Glynn Clements <glynn@gclements.plus.com>

Hamish · August 7, 2009, 3:41am

Glynn wrote:

Vector maps no longer work:

$ v.info fields
ERROR: Spatial index was written with LFS but this GRASS version does not
support LFS. Try to rebuild topology or upgrade GRASS.

Hi,

I've got nothing to add about that; just while on the subject: it would
be nice if v.info could work (even partially) for maps without topology
built. e.g. massive LIDAR datasets.

thanks,
Hamish

Markus_GRASS · August 7, 2009, 6:34am

Glynn Clements wrote:

Markus Metz wrote:

Without building topo I get

ERROR: Unable to open vector map <fields@PERMANENT> on level 2. Try to rebuild vector topology by v.build.

So I do
v.build map=fields
v.info fields now produces standard output

Running v.build requires that you own the mapset which contains it.

Yes, if you want to work with vectors not in the current mapset using grass7, you have to rebuild topology for all vectors in the corresponding mapset first.

g.copy produces the same error, but it does appear to copy the map.
Running v.build on the copy results in a working map.

Not here, g.copy also copies vector maps where one or more support files (topo, sidx, cidx) is missing. This is correct because g.copy should also copy level 1 vectors that don't have these support files.

BTW, spidx_port.off_t_size is 42, so the issue is that the code
expects the new format, not that sizeof(off_t) is wrong.


Weird, spidx_port.off_t_size should be 4 or 8, and 8 is only used when necessary, same like for reading/writing topo. I haven't included some debug info on spidx_port.off_t_size, so I guess you have added that yourself? Maybe a good idea, it's already there in dig_Rd_Plus_head() for reading topo. Can you also report sizeof(off_t)? That could help me tracking down the bug.

sizeof(off_t) is 8.

FWIW, the complete structure is:

Breakpoint 1, G_fatal_error (
    msg=0xb7f38128 "Spatial index was written with LFS but this GRASS version does not support LFS. Try to rebuild topology or upgrade GRASS.")
    at error.c:151
151 va_start(ap, msg);


where


#0 G_fatal_error (
    msg=0xb7f38128 "Spatial index was written with LFS but this GRASS version does not support LFS. Try to rebuild topology or upgrade GRASS.")
    at error.c:151
#1 0xb7f332d6 in dig_Rd_spidx_head (fp=0xbfdb6920, ptr=0xbfdb672c)
    at spindex_rw.c:280
#2 0xb7fd93e2 in Vect_open_sidx (Map=0xbfdb6720, mode=0) at open.c:854
#3 0xb7fd7f99 in Vect__open_old (Map=0xbfdb6720, name=0x8e827a0 "fields", mapset=0x804af50 "", update=0, head_only=1) at open.c:262
#4 0xb7fd87a8 in Vect_open_old_head (Map=0xbfdb6720, name=0x8e827a0 "fields", mapset=0x804af50 "") at open.c:501
#5 0x08049860 in main (argc=3, argv=0xbfdb6cf4) at main.c:104


frame 1


#1 0xb7f332d6 in dig_Rd_spidx_head (fp=0xbfdb6920, ptr=0xbfdb672c)
    at spindex_rw.c:280
280 G_fatal_error("Spatial index was written with LFS but this "


print *ptr


$1 = {Version_Major = 5, Version_Minor = 0, Back_Major = 5, Back_Minor = 0, spidx_Version_Major = 5, spidx_Version_Minor = 0, spidx_Back_Major = 5, spidx_Back_Minor = 0, cidx_Version_Major = 0, cidx_Version_Minor = 0, cidx_Back_Major = 0, cidx_Back_Minor = 0, with_z = 0, spidx_with_z = 0, off_t_size = 4, head_size = 142, spidx_head_size = 0, cidx_head_size = 0, release_support = 0, port = {byte_order = 0, off_t_size = 0, dbl_cnvrt = "\000\001\002\003\004\005\006\a", flt_cnvrt = "\000\001\002\003", lng_cnvrt = "\000\001\002\003", int_cnvrt = "\000\001\002\003", shrt_cnvrt = "\000\001", off_t_cnvrt = "\000\001\002\003\004\005\006\a", dbl_quick = 1, flt_quick = 1, lng_quick = 1, int_quick = 1, shrt_quick = 1, off_t_quick = 1}, spidx_port = {byte_order = 0, off_t_size = 42, dbl_cnvrt = "\000\000\000\000\000\000\000", flt_cnvrt = "\000\000\000", lng_cnvrt = "\000\000\000", int_cnvrt = "\000\000\000", shrt_cnvrt = "\000", off_t_cnvrt = "\000\000\000\000\000\000\000", dbl_quick = 0, flt_quick = 0, lng_quick = 0, int_quick = 0, shrt_quick = 0, off_t_quick = 0}, cidx_port = {byte_order = 0, off_t_size = 0, dbl_cnvrt = "\000\000\000\000\000\000\000", flt_cnvrt = "\000\000\000", lng_cnvrt = "\000\000\000", int_cnvrt = "\000\000\000", shrt_cnvrt = "\000", off_t_cnvrt = "\000\000\000\000\000\000\000", dbl_quick = 0, flt_quick = 0, lng_quick = 0, int_quick = 0, shrt_quick = 0, off_t_quick = 0}, mode = 0, built = 0, box = {N = 4928110, S = 4913925, E = 609544.37, W = 589438.87, T = 0, B = 0}, Node = 0x0, Line = 0x0, Area = 0x0, Isle = 0x0, n_nodes = 217, n_edges = 0, n_lines = 271, n_areas = 65, n_isles = 11, n_faces = 0, n_volumes = 0, n_holes = 0, n_plines = 0, n_llines = 0, n_blines = 208, n_clines = 63, n_flines = 0, n_klines = 0, n_vfaces = 0, n_hfaces = 0, alloc_nodes = 0, alloc_edges = 0, alloc_lines = 0, alloc_areas = 0, alloc_isles = 0, alloc_faces = 0, alloc_volumes = 0, alloc_holes = 0, Node_offset = 142, Edge_offset = 0, Line_offset = 8314, Area_offset = 20157, Isle_offset = 24417, Volume_offset = 0, Hole_offset = 0, Spidx_built = 0, Spidx_new = 0, spidx_fp = {file = 0x8e830f0, start = 0x0, current = 0x0, end = 0x0, size = 0, alloc = 0, loaded = 0}, spidx_node_fname = 0x0, Node_spidx_offset = 0, Line_spidx_offset = 0, Area_spidx_offset = 0, Isle_spidx_offset = 0, Face_spidx_offset = 0, Volume_spidx_offset = 0, Hole_spidx_offset = 0, Node_spidx = 0x8e834e8, Line_spidx = 0x8e83740, Area_spidx = 0x8e83998, Isle_spidx = 0x8e83bf0, Face_spidx = 0x0, Volume_spidx = 0x0, Hole_spidx = 0x0, update_cidx = 0, n_cidx = 0, a_cidx = 5, cidx = 0x8e83328, cidx_up_to_date = 0, coor_size = 27905, coor_mtime = 0, do_uplist = 0, uplines = 0x0, alloc_uplines = 0, n_uplines = 0, upnodes = 0x0, alloc_upnodes = 0, n_upnodes = 0}

Other than off_t_size = 42, ptr->spidx_port is all zeros.

Another problem is ptr->port.off_t_size = 0, it should be 4, as ptr->sidx_port.off_t_size.
topo was read successfully, reading sidx failed, cidx was not yet read. The libraries should not even attempt to read sidx if that file doesn't exist. The same test is done for all vector files. I don't understand why that happens, I get the correct error messages.

No, because this is set system-wide in Grass.make
https://trac.osgeo.org/grass/browser/grass/trunk/include/Make/Grass.make#L82

That's likely to cause problems for anything which uses $(VECT_CFLAGS)
but which isn't itself 64-bit aware (i.e. uses fseek/ftell, calculates
offsets as int/long).

because not only the library but also all modules including vector.h must have 64bit enabled.

That means all such modules must be made 64-bit safe. Without
_FILE_OFFSET_BITS=64, open(), fopen() etc will simply refuse to open
files larger than 2GiB. If you set _FILE_OFFSET_BITS=64 in code which
wasn't designed for it, ftell() will fail once you get past the 2GiB
mark, storing the result of lseek() in a long will silently truncate
it, etc.

This change has the potential to corrupt files, so it needs to be
completed ASAP.

OK. So I have to check and correct all occurrences of ftell and fseek in all vector modules and make sure the result of lseek is stored in off_t not long, and that offset passed to lseek is of type off_t not long. What else needs to be done?

open() and fopen() should behave the same like in the vector libs because of VECT_CFLAGS

Instead of adding the appropriate lines to each single Makefile I decided to set it system-wide, that also avoids problems with addon modules. It worked so far on Linux 32bit and Linux 64bit. What platform are you using?

Linux/x86 (32-bit).

With or without LFS?

I'm going to test (again) on Linux 32bit with and without LFS, it will take some time (a day or two).

Markus M

Markus_GRASS · August 7, 2009, 7:04am

Hamish wrote:

Glynn wrote:


Vector maps no longer work:

$ v.info fields
ERROR: Spatial index was written with LFS but this GRASS version does not
       support LFS. Try to rebuild topology or upgrade GRASS.

Hi,

I've got nothing to add about that; just while on the subject: it would
be nice if v.info could work (even partially) for maps without topology
built. e.g. massive LIDAR datasets.

It could be useful if more vector modules could support vectors without topology, most importantly all v.surf.* modules. Not a new idea, was mentioned earlier.

Markus M

Hamish · August 7, 2009, 7:44am

Hamish:

> it would be nice if v.info could work (even partially) for maps
without topology built. e.g. massive LIDAR datasets.

MMetz:

It could be useful if more vector modules could support vectors without
topology, most importantly all v.surf.* modules. Not a new idea, was
mentioned earlier.

AFAIK, they already do.

Hamish

__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com

Markus_GRASS · August 7, 2009, 8:41am

Hamish:

Hamish:


it would be nice if v.info could work (even partially) for maps


without topology built. e.g. massive LIDAR datasets.

MMetz:

It could be useful if more vector modules could support vectors without topology, most importantly all v.surf.* modules. Not a new idea, was mentioned earlier.

AFAIK, they already do.

Oops, right.

Should v.info calculate bounding box and number of features for level 1? This can take some time because the whole coor file must be read, but I would find it useful to know the extends and number of points (, lines, boundaries, centroids) even for level 1. That helps setting the right region for the v.surf.* modules.

Markus

neteler · August 7, 2009, 9:01am

On Fri, Aug 7, 2009 at 10:41 AM, Markus Metz
<markus.metz.giswork@googlemail.com> wrote:
...

Should v.info calculate bounding box and number of features for level 1?
This can take some time because the whole coor file must be read, but I
would find it useful to know the extends and number of points (, lines,
boundaries, centroids) even for level 1. That helps setting the right region
for the v.surf.* modules.

While I also think that this would be important info to obtain, another
option could be to modify v.univar to also work with x, y [,z]. Then the
user wouldn't be surprised that it takes time... (v.info is moreover just
reading a map header in "real" time).

Markus

Glynn_Clements1 · August 7, 2009, 7:31pm

Markus Metz wrote:

Yes, if you want to work with vectors not in the current mapset using
grass7, you have to rebuild topology for all vectors in the
corresponding mapset first.

So it's intentional that GRASS7 is no longer compatible with previous
vector maps?

That isn't necessarily a problem, but I must have missed the
announcement. But the error message needs to be clear; and accurate:
the problem isn't a lack of LFS support, and the map *wan't* created
with the LFS enabled (the map hasn't changed in 5 years).

> FWIW, the complete structure is:

[snip]

> Other than off_t_size = 42, ptr->spidx_port is all zeros.

Another problem is ptr->port.off_t_size = 0, it should be 4, as
ptr->sidx_port.off_t_size.
topo was read successfully, reading sidx failed, cidx was not yet read.
The libraries should not even attempt to read sidx if that file doesn't
exist. The same test is done for all vector files. I don't understand
why that happens, I get the correct error messages.

PERMANENT/vector/fields contains:

  -rw-r--r-- 1 root root 4117 May 25 2004 cidx
  -rw-r--r-- 1 root root 27905 May 25 2004 coor
  -rw-r--r-- 1 root root 55 May 25 2004 dbln
  -rw-r--r-- 1 root root 278 May 25 2004 head
  -rw-r--r-- 1 root root 186 May 25 2004 hist
  -rw-r--r-- 1 root root 24538 May 25 2004 sidx
  -rw-r--r-- 1 root root 25161 May 25 2004 topo

MD5 hashes:

  6d564fa2eec15fc5080694d74e106f26 cidx
  a09c6ee5263e9404c3890ad2f3d75aed coor
  f47b9255570ced6368981740ffc9ebb2 dbln
  7ed61317b7d9a658acb5bd305c9fbb7c head
  0679efebdb1616566d03243eb5b8c867 hist
  35c963fd5ececd8364d1c018353442c0 sidx
  4e5b18920309348c8df961f869d01793 topo

The files are unmodified from:

-rw-r--r-- 1 glynn root 19965571 Oct 16 2004 spearfish_grass57data.tar.gz

c7a356fcc9ab649f73973315cba205cb spearfish_grass57data.tar.gz

The 5.7 Spearfish maps worked until quite recently.

>> No, because this is set system-wide in Grass.make
>> https://trac.osgeo.org/grass/browser/grass/trunk/include/Make/Grass.make#L82
>
> That's likely to cause problems for anything which uses $(VECT_CFLAGS)
> but which isn't itself 64-bit aware (i.e. uses fseek/ftell, calculates
> offsets as int/long).
>
>> because not only the library but also all modules including vector.h
>> must have 64bit enabled.
>
> That means all such modules must be made 64-bit safe. Without
> _FILE_OFFSET_BITS=64, open(), fopen() etc will simply refuse to open
> files larger than 2GiB. If you set _FILE_OFFSET_BITS=64 in code which
> wasn't designed for it, ftell() will fail once you get past the 2GiB
> mark, storing the result of lseek() in a long will silently truncate
> it, etc.
>
> This change has the potential to corrupt files, so it needs to be
> completed ASAP.

OK. So I have to check and correct all occurrences of ftell and fseek in
all vector modules and make sure the result of lseek is stored in off_t
not long, and that offset passed to lseek is of type off_t not long.
What else needs to be done?

Offsets need to be calculated as off_t, not int or long. Casting them
when lseek/G_fseek are called is too late. IOW:

int row, cols, cols, bytes_per_cell;
off_t offset = row * (col * rows + col) * bytes_per_cell;

needs to be e.g.:

off_t offset = ((off_t) row * cols + col) * bytes_per_cell;

so that off_t is used throughout the calculation.

Also, this needs to be checked for anything which uses VECT_CFLAGS,
which isn't limited to v.* modules (a few g.* and r.* modules use
vector functions).

>> Instead of adding the appropriate lines to each
>> single Makefile I decided to set it system-wide, that also avoids
>> problems with addon modules. It worked so far on Linux 32bit and Linux
>> 64bit. What platform are you using?
>>
>
> Linux/x86 (32-bit).
>
With or without LFS?

With.

--
Glynn Clements <glynn@gclements.plus.com>

Markus_GRASS · August 8, 2009, 10:50am

Glynn Clements wrote:

Markus Metz wrote:

Yes, if you want to work with vectors not in the current mapset using grass7, you have to rebuild topology for all vectors in the corresponding mapset first.

So it's intentional that GRASS7 is no longer compatible with previous
vector maps?

Yes, for two reasons. GRASS7 expects now a sidx file while GRASS6 doesn't, and the last GRASS version writing a sidx file (I think GRASS57) wrote it differently to the new format. I have now changed the format numbers and set forward/backward compatibility info. GRASS6 can open GRASS7 vectors, but GRASS7 can't open GRASS6 or GRASS5 vectors, rebuilding topology is required. IOW, I broke backwards compatibility which is against GRASS development policy I guess. My argument is that enabling backwards compatibility would be about the same effort like rebuilding topology. I could write a module that converts any existing sidx file to the new format or builds the spatial index from topo if no sidx file is found, then writes a new sidx file in the new format. That would avoid rebuilding the core topology and category index.

The main change and justification is that GRASS7 can search the spatial index in file without loading the spatial index to memory. Only the header is loaded. The last GRASS version that wrote a sidx file could not search in file, the nodes were loaded from file and the spatial index was recreated in memory first. The sidx file was missing the location of children for each node of the RTree, that was not fixable with the previous method because it was depth-first pre-order traversal. I use now depth-first post-order traversal and thus know the location of all children once I write out a node. This is needed to do search and query in file. All functions for reading and writing the sidx file had to be rewritten from scratch. Further on, the previous RTree implementation was based on a testing version written by the original author Tony Guttman and was missing some essentials that belong to a standard data structure (ideas for modification taken from dglib and libavl). The new R*Tree used in GRASS7 has a new interface and new algorithms. All changes affect only the vector libs, the vector API for writing modules remained unchanged. I guess I should have mentioned that earlier.

That isn't necessarily a problem, but I must have missed the
announcement.

Was hidden here:
http://lists.osgeo.org/pipermail/grass-dev/2009-July/045098.html

But the error message needs to be clear; and accurate:
the problem isn't a lack of LFS support, and the map *wan't* created
with the LFS enabled (the map hasn't changed in 5 years).

Yes, it has changed, at least the spearfish dataset for GRASS6. Files of vector fields were last modified on April 11 2005 and Jan 17 2006.

-rw-r--r-- 1 mmetz users 4117 2006-01-17 17:50 cidx
-rw-r--r-- 1 mmetz users 27905 2005-04-11 14:26 coor
-rw-r--r-- 1 mmetz users 55 2005-04-11 14:26 dbln
-rw-r--r-- 1 mmetz users 278 2005-04-11 14:26 head
-rw-r--r-- 1 mmetz users 191 2005-04-11 14:26 hist
-rw-r--r-- 1 mmetz users 25161 2006-01-17 17:50 topo

Downloaded today 2009-08-08.

FWIW, the complete structure is:

[snip]

Other than off_t_size = 42, ptr->spidx_port is all zeros.


Another problem is ptr->port.off_t_size = 0, it should be 4, as ptr->sidx_port.off_t_size.
topo was read successfully, reading sidx failed, cidx was not yet read. The libraries should not even attempt to read sidx if that file doesn't exist. The same test is done for all vector files. I don't understand why that happens, I get the correct error messages.

PERMANENT/vector/fields contains:

  -rw-r--r-- 1 root root 4117 May 25 2004 cidx
  -rw-r--r-- 1 root root 27905 May 25 2004 coor
  -rw-r--r-- 1 root root 55 May 25 2004 dbln
  -rw-r--r-- 1 root root 278 May 25 2004 head
  -rw-r--r-- 1 root root 186 May 25 2004 hist
  -rw-r--r-- 1 root root 24538 May 25 2004 sidx

There is no sidx file for vectors in
http://grass.osgeo.org/sampledata/spearfish_grass60data-0.3.tar.gz

  -rw-r--r-- 1 root root 25161 May 25 2004 topo

The files are unmodified from:

  -rw-r--r-- 1 glynn root 19965571 Oct 16 2004 spearfish_grass57data.tar.gz

  c7a356fcc9ab649f73973315cba205cb spearfish_grass57data.tar.gz

Ah, ok, you use the grass57 dataset. I have now changed the version number and compatibility info, you should now get reasonable warnings/errors with the spearfish57 dataset.

Offsets need to be calculated as off_t, not int or long. Casting them
when lseek/G_fseek are called is too late. IOW:

  int row, cols, cols, bytes_per_cell;
  off_t offset = row * (col * rows + col) * bytes_per_cell;

needs to be e.g.:

  off_t offset = ((off_t) row * cols + col) * bytes_per_cell;

so that off_t is used throughout the calculation.

OK.

Also, this needs to be checked for anything which uses VECT_CFLAGS,
which isn't limited to v.* modules (a few g.* and r.* modules use
vector functions).

I fixed r.drain, v.in.dxf, v.vol.rst. AFAICT all other g.*, r.* and v.* modules are ok. Please let me know if I missed something.

I got carried away (replacing all fseek/ftell occurrences I could find with G_fseek/G_ftell, adjusting off_t as you showed above) and also made r3.in|out.v5d LFS-safe, but did not submit. Should I?

Markus M

Markus_GRASS · August 8, 2009, 11:50am

Hamish:

[...] it would
be nice if v.info could work (even partially) for maps without topology
built. e.g. massive LIDAR datasets.

Try trunk r38644, new -l flag for v.info to open vector map on level 1.

Markus M

Glynn_Clements1 · August 8, 2009, 5:03pm

Markus Metz wrote:

>> Yes, if you want to work with vectors not in the current mapset using
>> grass7, you have to rebuild topology for all vectors in the
>> corresponding mapset first.
>>
>
> So it's intentional that GRASS7 is no longer compatible with previous
> vector maps?

Yes, for two reasons. GRASS7 expects now a sidx file while GRASS6
doesn't, and the last GRASS version writing a sidx file (I think
GRASS57) wrote it differently to the new format. I have now changed the
format numbers and set forward/backward compatibility info. GRASS6 can
open GRASS7 vectors, but GRASS7 can't open GRASS6 or GRASS5 vectors,
rebuilding topology is required. IOW, I broke backwards compatibility
which is against GRASS development policy I guess.

Breaking backwards compatibility is allowed in 7.0. We'll probably be
breaking raster compatibility at some point (but in a far more
fundamental way; old rasters won't be recognised at all).

Something this fundamental should have been announced (maybe it was
and I missed it). Also, the error message is misleading.

My argument is that
enabling backwards compatibility would be about the same effort like
rebuilding topology. I could write a module that converts any existing
sidx file to the new format or builds the spatial index from topo if no
sidx file is found, then writes a new sidx file in the new format. That
would avoid rebuilding the core topology and category index.

Maybe the sidx file should have been renamed?

> That isn't necessarily a problem, but I must have missed the
> announcement.
Was hidden here:
http://lists.osgeo.org/pipermail/grass-dev/2009-July/045098.html

Right; I didn't get the significance of that at the time.

> PERMANENT/vector/fields contains:
>
> -rw-r--r-- 1 root root 4117 May 25 2004 cidx
> -rw-r--r-- 1 root root 27905 May 25 2004 coor
> -rw-r--r-- 1 root root 55 May 25 2004 dbln
> -rw-r--r-- 1 root root 278 May 25 2004 head
> -rw-r--r-- 1 root root 186 May 25 2004 hist
> -rw-r--r-- 1 root root 24538 May 25 2004 sidx

There is no sidx file for vectors in
http://grass.osgeo.org/sampledata/spearfish_grass60data-0.3.tar.gz

IOW: sidx was removed in 6.0 (existing sidx files were silently
ignored), then an entirely different sidx file was added recently, and
the code is confused by the existence of a 5.7 sidx file, right?

A different name might have been a better choice. Although the 5.7
datasets are old, they worked until a couple of weeks ago, so people
may still be using them.

Error messages about LFS could cause people to waste effort rebuilding
GRASS to no effect (we've seen something similar with people being
misdirected by the "helpful" information added to the "incompatible
library version" errors).

Ah, ok, you use the grass57 dataset. I have now changed the version
number and compatibility info, you should now get reasonable
warnings/errors with the spearfish57 dataset.

Okay, I'll check it.

> Also, this needs to be checked for anything which uses VECT_CFLAGS,
> which isn't limited to v.* modules (a few g.* and r.* modules use
> vector functions).

I fixed r.drain, v.in.dxf, v.vol.rst. AFAICT all other g.*, r.* and v.*
modules are ok. Please let me know if I missed something.

I don't know which modules need fixing and which don't. I was just
pointing out that e.g. g.{copy,remove,rename,list} use the vector
libraries (but don't use VECT_CFLAGS; is that a bug?), r.to.vect,
d.vect, etc use the libraries and VECT_CFLAGS, so it isn't just v.*
modules which need to be checked. It might just be v.* modules which
need to be fixed; I don't know.

I got carried away (replacing all fseek/ftell occurrences I could find
with G_fseek/G_ftell, adjusting off_t as you showed above) and also made
r3.in|out.v5d LFS-safe, but did not submit. Should I?

Yes; ideally, we should use G_{fseek,ftell} everywhere. Ultimately, it
would be better to enable -D_FILE_OFFSET_BITS=64 globally (to ensure
that there aren't any cases where a FILE* is obtained via fopen64()
then passed to code which can't handle it), which means making
everything LFS-aware.

--
Glynn Clements <glynn@gclements.plus.com>

Glynn_Clements1 · August 8, 2009, 6:14pm

Glynn Clements wrote:

> Ah, ok, you use the grass57 dataset. I have now changed the version
> number and compatibility info, you should now get reasonable
> warnings/errors with the spearfish57 dataset.

Okay, I'll check it.

Nope. As of r38644, the error message is unchanged.

--
Glynn Clements <glynn@gclements.plus.com>

neteler · August 8, 2009, 6:34pm

On Sat, Aug 8, 2009 at 7:03 PM, Glynn Clements<glynn@gclements.plus.com> wrote:

Markus Metz wrote:

>> Yes, if you want to work with vectors not in the current mapset using
>> grass7, you have to rebuild topology for all vectors in the
>> corresponding mapset first.
>>
>
> So it's intentional that GRASS7 is no longer compatible with previous
> vector maps?

Yes, for two reasons. GRASS7 expects now a sidx file while GRASS6
doesn't, and the last GRASS version writing a sidx file (I think
GRASS57) wrote it differently to the new format. I have now changed the
format numbers and set forward/backward compatibility info. GRASS6 can
open GRASS7 vectors, but GRASS7 can't open GRASS6 or GRASS5 vectors,
rebuilding topology is required. IOW, I broke backwards compatibility
which is against GRASS development policy I guess.

Breaking backwards compatibility is allowed in 7.0. We'll probably be
breaking raster compatibility at some point (but in a far more
fundamental way; old rasters won't be recognised at all).

Something this fundamental should have been announced (maybe it was
and I missed it). Also, the error message is misleading.

My argument is that
enabling backwards compatibility would be about the same effort like
rebuilding topology. I could write a module that converts any existing
sidx file to the new format or builds the spatial index from topo if no
sidx file is found, then writes a new sidx file in the new format. That
would avoid rebuilding the core topology and category index.

Maybe the sidx file should have been renamed?

> That isn't necessarily a problem, but I must have missed the
> announcement.
Was hidden here:
http://lists.osgeo.org/pipermail/grass-dev/2009-July/045098.html

Right; I didn't get the significance of that at the time.

> PERMANENT/vector/fields contains:
>
> -rw-r--r-- 1 root root 4117 May 25 2004 cidx
> -rw-r--r-- 1 root root 27905 May 25 2004 coor
> -rw-r--r-- 1 root root 55 May 25 2004 dbln
> -rw-r--r-- 1 root root 278 May 25 2004 head
> -rw-r--r-- 1 root root 186 May 25 2004 hist
> -rw-r--r-- 1 root root 24538 May 25 2004 sidx

There is no sidx file for vectors in
http://grass.osgeo.org/sampledata/spearfish_grass60data-0.3.tar.gz

IOW: sidx was removed in 6.0 (existing sidx files were silently
ignored), then an entirely different sidx file was added recently, and
the code is confused by the existence of a 5.7 sidx file, right?

A different name might have been a better choice. Although the 5.7
datasets are old, they worked until a couple of weeks ago, so people
may still be using them.

Error messages about LFS could cause people to waste effort rebuilding
GRASS to no effect (we've seen something similar with people being
misdirected by the "helpful" information added to the "incompatible
library version" errors).

Ah, ok, you use the grass57 dataset. I have now changed the version
number and compatibility info, you should now get reasonable
warnings/errors with the spearfish57 dataset.

Okay, I'll check it.

> Also, this needs to be checked for anything which uses VECT_CFLAGS,
> which isn't limited to v.* modules (a few g.* and r.* modules use
> vector functions).

I fixed r.drain, v.in.dxf, v.vol.rst. AFAICT all other g.*, r.* and v.*
modules are ok. Please let me know if I missed something.

I don't know which modules need fixing and which don't. I was just
pointing out that e.g. g.{copy,remove,rename,list} use the vector
libraries (but don't use VECT_CFLAGS; is that a bug?), r.to.vect,
d.vect, etc use the libraries and VECT_CFLAGS, so it isn't just v.*
modules which need to be checked. It might just be v.* modules which
need to be fixed; I don't know.

I got carried away (replacing all fseek/ftell occurrences I could find
with G_fseek/G_ftell, adjusting off_t as you showed above) and also made
r3.in|out.v5d LFS-safe, but did not submit. Should I?

Yes; ideally, we should use G_{fseek,ftell} everywhere.

Patch attached.

Note that it now fails to compile here:

make[1]: Entering directory `/home/neteler/grass70/lib/rst/interp_float'
gcc -I/home/neteler/grass70/dist.x86_64-unknown-linux-gnu/include
-I/home/neteler/grass70/dist.x86_64-unknown-linux-gnu/include -g
-Wall -Werror-implicit-function-declaration -fno-common -mtune=nocona
-m64 -minline-all-stringops -fPIC -I/usr/local/include
-D_FILE_OFFSET_BITS=64 -DPACKAGE=\""grasslibs"\"
-I/home/neteler/grass70/dist.x86_64-unknown-linux-gnu/include
-I/home/neteler/grass70/dist.x86_64-unknown-linux-gnu/include -o
OBJ.x86_64-unknown-linux-gnu/output2d.o -c output2d.c
output2d.c: In function ‘IL_output_2d’:
output2d.c:153: error: void value not ignored as it ought to be
output2d.c:174: error: void value not ignored as it ought to be
output2d.c:190: error: void value not ignored as it ought to be
output2d.c:206: error: void value not ignored as it ought to be
output2d.c:222: error: void value not ignored as it ought to be
output2d.c:238: error: void value not ignored as it ought to be
make[1]: *** [OBJ.x86_64-unknown-linux-gnu/output2d.o] Error 1

No clue how what could be wrong in the patch. Same problem in
raster/r.in.xyz

Feel free to recycle the patch.

Markus

(attachments)

G_fseek_ftell.diff.gz (6.1 KB)

Markus_GRASS · August 8, 2009, 7:00pm

Glynn Clements wrote:

Glynn Clements wrote:

Ah, ok, you use the grass57 dataset. I have now changed the version number and compatibility info, you should now get reasonable warnings/errors with the spearfish57 dataset.


Okay, I'll check it.

Nope. As of r38644, the error message is unchanged.

I got the order of the checks wrong. First check must be version check, only after that comes the check for file size. That LFS check is there to check if sizeof(off_t) = 8 is needed to read the sidx file. Fixed in r36845.

Markus M

Helena_Mitasova · August 8, 2009, 7:11pm

I did not have time to respond to this line of discussion as I am trying to get my class material ready,

but lib/rst also needs to be updated with offsets set to off_t and fseek to G_fseek
in interp2d.c and write2d.c - see below. This should be done only in grass7.

I think GRASS64 / 65 rst version should stay as it is - with the recent fixes
it already should support raster output up to around 30,000x30,000 computed from
tens of millions of points on a higher end PC so if anybody wants to do anything bigger
I would suggest to use grass7. Also the recent bug fixes in v.surf.rst done in grass65
need to be ported to grass7 - it is not as straightforward as I thought - there are too many differences
between grass6 and grass7 already (perhaps time to finish GRASS64 and release it -
RC1 has been released in December 2008),

Helena

here is my exchange with Glynn on offset topic :

For 38300x30200 raster when writing to temp file, the program says
Cannot fseek elev offset2=-438164932

due to this in write2d.c:
   if (fseek(params->Tmp_fd_z, (long)offset2, 0) == -1) {
      fprintf(stderr, "Cannot fseek elev offset2=%d\n", (int)offset2);

offset2 is computed in interp2d.c as follows:

offset1 is number of columns
offset = offset1 * (k - 1); is rows offset, k=1,nrows so this can be
38300x30200
offset2 = (offset + ngstc - 1) * sizeof(FCELL) where ngstc is
starting column

for example:

here it does not give error but offset2 is apparently wrong
offset=1077052800, offset1=30200, k=35665
offset=1077052800, offset2=13245556, ngstc=414, fcell=4

here offset2 is negative so it gives an error:
offset=1070318200, offset1=30200, k=35442
offset=1070318200, offset2=-13694496, ngstc=1, fcell=4
Cannot fseek elev offset2=-13694496

I am on 64bit machine compiled with large file support
and I put this into Makefile (according the the advice on the devlist)

ifneq ($(USE_LARGEFILES),)
         EXTRA_CFLAGS = -D_FILE_OFFSET_BITS=64 $(VECT_CFLAGS)
endif

hoping that would take care of it, but apparently that is not enough.
all offsets above are defined as int.

So how to fix this?
Should I just replace offset2 with off_t as suggested in the dev list?
Do I need to do anything with fseek?
or am I completely misunderstanding this issue?

LFS doesn't work with fseek(); but LFS does nothing on a 64-bit system
anyhow.

The main problem is that offset1 and offset2 are "int"s, and they need
to be either "long" or "off_t". Also, when you calculate an offset by
multiplying a row number by the row width, one of the operands must be
cast to "long" (or "off_t") *before* the multiply, e.g.:

off_t offset = (off_t) row * row_size;

Casting the result of a multiplication won't work; the value will
already have been truncated.

For LFS (files >2GiB on 32-bit systems), the same issues apply,
except:

1. offsets need to be "off_t" rather than "long".

2. fseek() and ftell() won't work, as they use "long" rather than
"off_t". Use G_fseek() and G_ftell() instead; these use the fseeko()
and ftello() functions on platforms where they are available.

Helena Mitasova
Associate Professor
Department of Marine, Earth
and Atmospheric Sciences
1125 Jordan Hall, Campus Box 8208
North Carolina State University
Raleigh NC 27695-8208
http://skagit.meas.ncsu.edu/~helena/

On Aug 8, 2009, at 2:34 PM, Markus Neteler wrote:

On Sat, Aug 8, 2009 at 7:03 PM, Glynn Clements<glynn@gclements.plus.com> wrote:

Markus Metz wrote:

Yes, if you want to work with vectors not in the current mapset using
grass7, you have to rebuild topology for all vectors in the
corresponding mapset first.

So it's intentional that GRASS7 is no longer compatible with previous
vector maps?

Yes, for two reasons. GRASS7 expects now a sidx file while GRASS6
doesn't, and the last GRASS version writing a sidx file (I think
GRASS57) wrote it differently to the new format. I have now changed the
format numbers and set forward/backward compatibility info. GRASS6 can
open GRASS7 vectors, but GRASS7 can't open GRASS6 or GRASS5 vectors,
rebuilding topology is required. IOW, I broke backwards compatibility
which is against GRASS development policy I guess.

Breaking backwards compatibility is allowed in 7.0. We'll probably be
breaking raster compatibility at some point (but in a far more
fundamental way; old rasters won't be recognised at all).

Something this fundamental should have been announced (maybe it was
and I missed it). Also, the error message is misleading.

My argument is that
enabling backwards compatibility would be about the same effort like
rebuilding topology. I could write a module that converts any existing
sidx file to the new format or builds the spatial index from topo if no
sidx file is found, then writes a new sidx file in the new format. That
would avoid rebuilding the core topology and category index.

Maybe the sidx file should have been renamed?

That isn't necessarily a problem, but I must have missed the
announcement.

Was hidden here:
http://lists.osgeo.org/pipermail/grass-dev/2009-July/045098.html

Right; I didn't get the significance of that at the time.

PERMANENT/vector/fields contains:

    -rw-r--r-- 1 root root 4117 May 25 2004 cidx
    -rw-r--r-- 1 root root 27905 May 25 2004 coor
    -rw-r--r-- 1 root root 55 May 25 2004 dbln
    -rw-r--r-- 1 root root 278 May 25 2004 head
    -rw-r--r-- 1 root root 186 May 25 2004 hist
    -rw-r--r-- 1 root root 24538 May 25 2004 sidx

There is no sidx file for vectors in
http://grass.osgeo.org/sampledata/spearfish_grass60data-0.3.tar.gz

IOW: sidx was removed in 6.0 (existing sidx files were silently
ignored), then an entirely different sidx file was added recently, and
the code is confused by the existence of a 5.7 sidx file, right?

A different name might have been a better choice. Although the 5.7
datasets are old, they worked until a couple of weeks ago, so people
may still be using them.

Error messages about LFS could cause people to waste effort rebuilding
GRASS to no effect (we've seen something similar with people being
misdirected by the "helpful" information added to the "incompatible
library version" errors).

Ah, ok, you use the grass57 dataset. I have now changed the version
number and compatibility info, you should now get reasonable
warnings/errors with the spearfish57 dataset.

Okay, I'll check it.

Also, this needs to be checked for anything which uses VECT_CFLAGS,
which isn't limited to v.* modules (a few g.* and r.* modules use
vector functions).

I fixed r.drain, v.in.dxf, v.vol.rst. AFAICT all other g.*, r.* and v.*
modules are ok. Please let me know if I missed something.

I don't know which modules need fixing and which don't. I was just
pointing out that e.g. g.{copy,remove,rename,list} use the vector
libraries (but don't use VECT_CFLAGS; is that a bug?), r.to.vect,
d.vect, etc use the libraries and VECT_CFLAGS, so it isn't just v.*
modules which need to be checked. It might just be v.* modules which
need to be fixed; I don't know.

I got carried away (replacing all fseek/ftell occurrences I could find
with G_fseek/G_ftell, adjusting off_t as you showed above) and also made
r3.in|out.v5d LFS-safe, but did not submit. Should I?

Yes; ideally, we should use G_{fseek,ftell} everywhere.

Patch attached.

Note that it now fails to compile here:

make[1]: Entering directory `/home/neteler/grass70/lib/rst/interp_float'
gcc -I/home/neteler/grass70/dist.x86_64-unknown-linux-gnu/include
-I/home/neteler/grass70/dist.x86_64-unknown-linux-gnu/include -g
-Wall -Werror-implicit-function-declaration -fno-common -mtune=nocona
-m64 -minline-all-stringops -fPIC -I/usr/local/include
-D_FILE_OFFSET_BITS=64 -DPACKAGE=\""grasslibs"\"
-I/home/neteler/grass70/dist.x86_64-unknown-linux-gnu/include
-I/home/neteler/grass70/dist.x86_64-unknown-linux-gnu/include -o
OBJ.x86_64-unknown-linux-gnu/output2d.o -c output2d.c
output2d.c: In function ‘IL_output_2d’:
output2d.c:153: error: void value not ignored as it ought to be
output2d.c:174: error: void value not ignored as it ought to be
output2d.c:190: error: void value not ignored as it ought to be
output2d.c:206: error: void value not ignored as it ought to be
output2d.c:222: error: void value not ignored as it ought to be
output2d.c:238: error: void value not ignored as it ought to be
make[1]: *** [OBJ.x86_64-unknown-linux-gnu/output2d.o] Error 1

No clue how what could be wrong in the patch. Same problem in
raster/r.in.xyz

Feel free to recycle the patch.

Markus
<G_fseek_ftell.diff.gz>
_______________________________________________
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev

Markus_GRASS · August 8, 2009, 7:19pm

Glynn:

Breaking backwards compatibility is allowed in 7.0. We'll probably be
breaking raster compatibility at some point (but in a far more
fundamental way; old rasters won't be recognised at all).

Something this fundamental should have been announced (maybe it was
and I missed it). Also, the error message is misleading.

I will announce it more loudly next time
The error message should be fixed, see other post.

Markus:

My argument is that enabling backwards compatibility would be about the same effort like rebuilding topology. I could write a module that converts any existing sidx file to the new format or builds the spatial index from topo if no sidx file is found, then writes a new sidx file in the new format. That would avoid rebuilding the core topology and category index.

Maybe the sidx file should have been renamed?

That would be fool-proof and no problem.

PERMANENT/vector/fields contains:

  -rw-r--r-- 1 root root 4117 May 25 2004 cidx
  -rw-r--r-- 1 root root 27905 May 25 2004 coor
  -rw-r--r-- 1 root root 55 May 25 2004 dbln
  -rw-r--r-- 1 root root 278 May 25 2004 head
  -rw-r--r-- 1 root root 186 May 25 2004 hist
  -rw-r--r-- 1 root root 24538 May 25 2004 sidx


There is no sidx file for vectors in
http://grass.osgeo.org/sampledata/spearfish_grass60data-0.3.tar.gz

IOW: sidx was removed in 6.0 (existing sidx files were silently
ignored), then an entirely different sidx file was added recently, and
the code is confused by the existence of a 5.7 sidx file, right?

Yes. I wasn't aware that 5.7 was writing sidx files. I found the spearfsh57 sample dataset just now, will use it for more testing in the future.

Also, this needs to be checked for anything which uses VECT_CFLAGS,
which isn't limited to v.* modules (a few g.* and r.* modules use
vector functions).


I fixed r.drain, v.in.dxf, v.vol.rst. AFAICT all other g.*, r.* and v.* modules are ok. Please let me know if I missed something.

I don't know which modules need fixing and which don't. I was just
pointing out that e.g. g.{copy,remove,rename,list} use the vector
libraries (but don't use VECT_CFLAGS; is that a bug?)

Yes. Anything that uses vector libraries must use the same LFS-related flags like the vector libs, otherwise there can be segfaults because the size of a number of structures, most importantly struct Map_info, is on 32bit systems dependent on the LFS flags.

, r.to.vect,
d.vect, etc use the libraries and VECT_CFLAGS, so it isn't just v.*
modules which need to be checked. It might just be v.* modules which
need to be fixed; I don't know.

OK. I will check all modules including <grass/vector.h> or any of the headers in include/vect. All vector modules should be fixed by now. Of the raster modules, only r.drain needed fixing, all other were ok. But I will check again.

I got carried away (replacing all fseek/ftell occurrences I could find with G_fseek/G_ftell, adjusting off_t as you showed above) and also made r3.in|out.v5d LFS-safe, but did not submit. Should I?

Yes;

Will do, testing highly welcome!

ideally, we should use G_{fseek,ftell} everywhere. Ultimately, it
would be better to enable -D_FILE_OFFSET_BITS=64 globally (to ensure
that there aren't any cases where a FILE* is obtained via fopen64()
then passed to code which can't handle it), which means making
everything LFS-aware.

That's beyond my scope because aclocal.m4 and configure.in need adjustment, and I won't touch these. I guess you are suitably qualified for the task

Markus M

Glynn_Clements1 · August 8, 2009, 11:24pm

Markus Neteler wrote:

>> I got carried away (replacing all fseek/ftell occurrences I could find
>> with G_fseek/G_ftell, adjusting off_t as you showed above) and also made
>> r3.in|out.v5d LFS-safe, but did not submit. Should I?
>
> Yes; ideally, we should use G_{fseek,ftell} everywhere.

Patch attached.

Note that it now fails to compile here:

make[1]: Entering directory `/home/neteler/grass70/lib/rst/interp_float'
gcc -I/home/neteler/grass70/dist.x86_64-unknown-linux-gnu/include
-I/home/neteler/grass70/dist.x86_64-unknown-linux-gnu/include -g
-Wall -Werror-implicit-function-declaration -fno-common -mtune=nocona
-m64 -minline-all-stringops -fPIC -I/usr/local/include
-D_FILE_OFFSET_BITS=64 -DPACKAGE=\""grasslibs"\"
-I/home/neteler/grass70/dist.x86_64-unknown-linux-gnu/include
-I/home/neteler/grass70/dist.x86_64-unknown-linux-gnu/include -o
OBJ.x86_64-unknown-linux-gnu/output2d.o -c output2d.c
output2d.c: In function �IL_output_2d�:
output2d.c:153: error: void value not ignored as it ought to be

No clue how what could be wrong in the patch. Same problem in
raster/r.in.xyz

fseek() returns a status code, but G_fseek() doesn't (it calls
G_fatal_error() if fseek() returns non-zero).

lib/rst should be changed to ignore the return value and let G_fseek()
signal an error.

r.in.xyz actually needs the return value, as it can handle
non-seekable streams (e.g. pipes).

It might be possible to keep the fseek(), as it only ever passes an
offset of zero (to seek to either the beginning or end of the file);
it depends upon whether fseek(fp,0,SEEK_END) works if the file is

2GiB.

Alternatively, we may need a G__fseek() which returns the status
rather than calling G_fatal_error().

In any case, the ftell() call needs to be changed to G_ftell().

--
Glynn Clements <glynn@gclements.plus.com>

Glynn_Clements1 · August 8, 2009, 11:31pm

Markus Metz wrote:

> d.vect, etc use the libraries and VECT_CFLAGS, so it isn't just v.*
> modules which need to be checked. It might just be v.* modules which
> need to be fixed; I don't know.

OK. I will check all modules including <grass/vector.h> or any of the
headers in include/vect. All vector modules should be fixed by now. Of
the raster modules, only r.drain needed fixing, all other were ok. But I
will check again.

I suggest checking for VECT_CFLAGS in the Makefiles. If that is used
for compilation, <stdio.h> will use the LFS functions (fopen64 etc),
so the code needs to handle files >2GiB regardless of how (or if) it
uses the vector libraries.

> ideally, we should use G_{fseek,ftell} everywhere. Ultimately, it
> would be better to enable -D_FILE_OFFSET_BITS=64 globally (to ensure
> that there aren't any cases where a FILE* is obtained via fopen64()
> then passed to code which can't handle it), which means making
> everything LFS-aware.

That's beyond my scope because aclocal.m4 and configure.in need
adjustment, and I won't touch these. I guess you are suitably qualified
for the task

The build system changes are straigtforward; just set
_FILE_OFFSET_BITS in config.h (this is already done, but it's
conditional upon USE_LARGEFILES, which doesn't get set).

The effort is in ensuring that code doesn't fail in inconvenient ways
(i.e. corrupting data) when dealing with files >2GiB, i.e. converting
everything to use off_t.

--
Glynn Clements <glynn@gclements.plus.com>