[GRASS-dev] [GRASS-SVN] r60054 - in grass/trunk: display/d.extract display/d.path display/d.vect.chart general/manage/lister lib/sites lib/vector/Vlib ps/ps.map raster/r.carve raster/r.contour raster/r.le/r.le.setup raster/r.random raster/r.stream.ext

GRASS GIS GRASS GIS developer
1

Hi,

2014-05-03 15:49 GMT+02:00 <svn_grass@osgeo.org>:

Author: hcho
Date: 2014-05-03 06:49:33 -0700 (Sat, 03 May 2014)
New Revision: 60054

I would say that such invasive changes should be discussed in advance.
Now the vector library goes in different direction that raster library
which promote fatal error even if raster map not found so the raster
modules don't check return code of Rast_open_old() at all. Even this
function has still integer return type. I am not fun of calling
G_fatal_error() on such places. But we need to keep some consistency
and to define rules when call G_fatal_error() and when G_warning() +
error return code is preferable (and then to update alll modules to
check the return code).

Martin

--
Martin Landa * http://geo.fsv.cvut.cz/gwiki/Landa

2

Martin,

I agree that it’s not fun to call fatal error whenever opening something, so I thought about adding fatal error in Vect routines. But some modules actually check its return value and do cleaning before exit. Another option would be to have Vect*2 routines that call fatal error and let the developer decide which version to use. Some low level routines mix -1 return and fatal error on failure, which I think should be fixed.

Simply calling fatal error and terminating the process may not be a good idea in some case where many leftover files need to be deleted. I believe at some point we need to standardize error handling.

I found an interesting discussion at http://lists.osgeo.org/pipermail/grass-dev/2013-April/062938.html and think you had similar concerns as mine here. I’m with Markus M that the warnings in Vect_open routines somehow deserve a fatal error, but I believe that you changed fatal errors to warnings to handle exception cases, which is exactly what I did. Those modules had not been updated to reflect your changes. But still I agree that we need a better way to handle opening errors.

Regards,
Huidae

On May 4, 2014 3:58 AM, “Martin Landa” <landa.martin@gmail.com> wrote:

Hi,

2014-05-03 15:49 GMT+02:00 <svn_grass@osgeo.org>:

Author: hcho
Date: 2014-05-03 06:49:33 -0700 (Sat, 03 May 2014)
New Revision: 60054

I would say that such invasive changes should be discussed in advance.
Now the vector library goes in different direction that raster library
which promote fatal error even if raster map not found so the raster
modules don’t check return code of Rast_open_old() at all. Even this
function has still integer return type. I am not fun of calling
G_fatal_error() on such places. But we need to keep some consistency
and to define rules when call G_fatal_error() and when G_warning() +
error return code is preferable (and then to update alll modules to
check the return code).

Martin

–
Martin Landa * http://geo.fsv.cvut.cz/gwiki/Landa

3

Hi,

2014-05-04 16:16 GMT+02:00 Huidae Cho <grass4u@gmail.com>:

I agree that it's not fun to call fatal error whenever opening something, so
I thought about adding fatal error in Vect routines. But some modules
actually check its return value and do cleaning before exit. Another option

please note that cleaning should be implemented as error handlers [1],
vector [2] and db [3] libs have defined standardized (often used)
error handlers. Martin

[1] http://grass.osgeo.org/programming7/gis_2handler_8c.html#aa01633c78dd9561c740e4a89e33ad80c
[2] http://grass.osgeo.org/programming7/vector_2Vlib_2handler_8c.html#aa692177b1d99fdcb7c982b8a64889263
[3] http://grass.osgeo.org/programming7/db_2dbmi__client_2handler_8c.html#a67d9435f4187f1119e1a29b60efdb71b

4

Martin,

Even after defining error handlers, we still need to call G_fatal_error to trigger them. The Vect_set_error_handler_io manual recommends to use this routine after Vect_open_old* because it needs the point to In and Out, but if Vect_open_old* fails and doesn’t call G_fatal_error, at what point, can we call fatal error without checking their return value?

G_add_error_handler is a good way to do a cleanup, but, IMHO, it doesn’t eliminate the need for checking Vect_open_* return value completely because of the above reason as long as they don’t call G_fatal_error internally. But again, when they call fatal error internally, they don’t have pointers to maps. It would be great if we could keep track of opened raster/vector maps and properly close existing maps and delete unfinished new maps inside G_fatal_error. And use G_add_error_handler to do a non-map related cleanup.

Huidae

···

On Sun, May 4, 2014 at 2:52 PM, Martin Landa <landa.martin@gmail.com> wrote:

Hi,

2014-05-04 16:16 GMT+02:00 Huidae Cho <grass4u@gmail.com>:

I agree that it’s not fun to call fatal error whenever opening something, so
I thought about adding fatal error in Vect routines. But some modules
actually check its return value and do cleaning before exit. Another option

please note that cleaning should be implemented as error handlers [1],
vector [2] and db [3] libs have defined standardized (often used)
error handlers. Martin

[1] http://grass.osgeo.org/programming7/gis_2handler_8c.html#aa01633c78dd9561c740e4a89e33ad80c
[2] http://grass.osgeo.org/programming7/vector_2Vlib_2handler_8c.html#aa692177b1d99fdcb7c982b8a64889263
[3] http://grass.osgeo.org/programming7/db_2dbmi__client_2handler_8c.html#a67d9435f4187f1119e1a29b60efdb71b

5

Martin Landa wrote:

2014-05-03 15:49 GMT+02:00 <svn_grass@osgeo.org>:
> Author: hcho
> Date: 2014-05-03 06:49:33 -0700 (Sat, 03 May 2014)
> New Revision: 60054

I would say that such invasive changes should be discussed in advance.
Now the vector library goes in different direction that raster library

The vector library has always used status returns; it's just that
almost nothing bothered to actually check them.

This patch just fixes many instances of a specific bug in one go.

Personally, I would have fixed it by eliminating status returns;
*that* would have qualified as an "invasive" change.

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

6

Huidae Cho wrote:

Simply calling fatal error and terminating the process may not be a good
idea in some case where many leftover files need to be deleted. I believe
at some point we need to standardize error handling.

I feel that exit handlers (or similar) are the way to go here. We
might want our own mechanism rather than using atexit(), so that it
works even when longjmp()ing out of a fatal error handler.

Apart from the global error handler (G_set_error_routine), we probably
want to support multiple, non-exclusive handlers.

[We may also want to support the ability to push/pop the global error
handler so that it's possible to perform the equivalent of catching an
exception then re-throwing it. But this needs feedback from anyone who
happens to be using it (its only within GRASS is by 3 curses-based
imagery modules, all of which are disabled, to restore the terminal).]

For temporary files, I suggest adding a "mode" argument to the
creation function to indicate whether the file should be:

* deleted automatically upon fatal error
* deleted automatically upon exit, successful or not
* deleted (unlink()ed) immediately (on Unix) or on exit (Windows).
* retained until explicitly deleted.

Building this behaviour into the libraries rather than having the
caller do it provides consistency. It also allows e.g. having an
environment variable to suppress deletion of temporary files for
debugging purposes.

But still I agree that we need a better way to handle opening
errors.

I don't feel that it's worth attempting to recover from such errors,
in the sense of continuing with normal execution. Clean up, then
terminate.

Users shouldn't specify non-existent or otherwise invalid maps as
arguments, and shouldn't expect anything beyond an error message if
they do.

If there are specific cases where a map's existence shouldn't be taken
for granted (e.g. the name was obtained from metadata which might be
stale or otherwise inapplicable), we should use G_find_* before
attempting to open it.

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

7

Huidae Cho wrote:

G_add_error_handler is a good way to do a cleanup,

I had actually forgetten about the existence of this.

But again, when they call fatal error internally, they don't have pointers
to maps. It would be great if we could keep track of opened raster/vector
maps and properly close existing maps and delete unfinished new maps inside
G_fatal_error. And use G_add_error_handler to do a non-map related cleanup.

Right.

Anything related to raster maps is available through the R__ structure
in lib/raster (R.h, init.c). R__.fileinfo is an array of "fileinfo"
structures, with R__.fileinfo_count elements. The open_mode field of
each structure will be -1 for unused slots, or one of the OPEN_*
constants (1, 2, or 3) for an open map.

We might want an OPEN_INCOMPLETE value for maps which are in the
process of being opened; this would only be relevant for handling
clean-up for fatal errors from within Rast_open_*.

I'm not that familiar with the vector library, but it looks like
everything relating to a map is stored in a Map_info structure which
is passed in by the caller. There's no global table of vector maps,
right?

If so, I suggest changing that, however invasive it may be. I'd
certainly suggest holding off on any 7.0 release until that's
resolved.

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

8

Glynn Clements wrote:

Huidae Cho wrote:

But again, when they call fatal error internally, they don't have pointers
to maps. It would be great if we could keep track of opened raster/vector
maps and properly close existing maps and delete unfinished new maps inside
G_fatal_error. And use G_add_error_handler to do a non-map related cleanup.

Right.

Anything related to raster maps is available through the R__ structure
in lib/raster (R.h, init.c). R__.fileinfo is an array of "fileinfo"
structures, with R__.fileinfo_count elements. The open_mode field of
each structure will be -1 for unused slots, or one of the OPEN_*
constants (1, 2, or 3) for an open map.

I'm not that familiar with the vector library, but it looks like
everything relating to a map is stored in a Map_info structure which
is passed in by the caller. There's no global table of vector maps,
right?

No. But such a list of opened maps sounds like a good idea.

If so, I suggest changing that, however invasive it may be.

Very invasive.

I'd
certainly suggest holding off on any 7.0 release until that's
resolved.

Hmm. This sounds like synchronizing some basic concepts of the raster
and vector libraries, which in turn sounds like something for GRASS 8.
GRASS 7 is IMHO in pretty good shape right now, lots of new features,
faster, global LFS, etc. And GRASS 7 is not new, it is actually many
years old. Therefore I would rather like to see get 7 released soon
and synchronize raster and vector libs in GRASS 8. For example,
something I am really missing in GRASS raster data is the history of
GRASS vector data. Also some more raster metadata like number of
non-null cells, mean and stddev (see GDAL) would be really helpful.
Not to mention the organization of raster file storage: have one
raster folder like for vector.

Markus M

9

On Wed, May 7, 2014 at 9:21 PM, Markus Metz
<markus.metz.giswork@gmail.com> wrote:

Glynn Clements wrote:

...

If so, I suggest changing that, however invasive it may be.

Very invasive.

I'd
certainly suggest holding off on any 7.0 release until that's
resolved.

Hmm. This sounds like synchronizing some basic concepts of the raster
and vector libraries, which in turn sounds like something for GRASS 8.

IMHO yes.

GRASS 7 is IMHO in pretty good shape right now, lots of new features,
faster, global LFS, etc. And GRASS 7 is not new, it is actually many
years old. Therefore I would rather like to see get 7 released soon
and synchronize raster and vector libs in GRASS 8.

+1

For example,
something I am really missing in GRASS raster data is the history of
GRASS vector data. Also some more raster metadata like number of
non-null cells, mean and stddev (see GDAL) would be really helpful.
Not to mention the organization of raster file storage: have one
raster folder like for vector.

Just for fun: ideas may be collected in this new trac page:

http://trac.osgeo.org/grass/wiki/Grass8Planning
:slight_smile:

markusN

10

Markus Metz wrote:

> If so, I suggest changing that, however invasive it may be.

Very invasive.

Probably not. Admittedly, it would affect a lot of code, but only in
fairly trivial ways. Specifically, all of the "Map" variables would
become pointers rather than structures, calls to vector functions will
have "&Map" changed to "Map", and the open functions will return a
pointer rather than having one passed in.

Although, we could probably get away with just storing a table of
pointers. In theory, callers might move/copy the Map_info structures;
in practice, they don't.

> I'd
> certainly suggest holding off on any 7.0 release until that's
> resolved.

Hmm. This sounds like synchronizing some basic concepts of the raster
and vector libraries,

Not really; it's just fixing a design flaw in the 6.x vector API.

Apart from the issue of the vector library not having a list of open
maps, another issue is that the size (and occasionally[1] the layout)
of the Map_info structure gets baked into the modules, so anything
which changes the layout of the structure constitutes a change to the
ABI.

[1] e.g. v.info references a couple of fields directly, rather than
using the appropriate accessor functions.

Ideally, the size and layout of the Map_info structure should only be
known to the vector libraries. The public headers should declare
Map_info as an abstract structure ("incomplete type" in C parlance).

which in turn sounds like something for GRASS 8.

FWIW, the first 6.0 beta was tagged on 2005-01-12, i.e. over 9 years
ago. Unless there's a reason to believe that 7.0->8.0 will happen
somewhat quicker than 6.0->7.0, 8.0 is too long to wait.

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

11

Hmm. This sounds like synchronizing some basic concepts of the raster
and vector libraries, which in turn sounds like something for GRASS 8.
GRASS 7 is IMHO in pretty good shape right now, lots of new features,
faster, global LFS, etc. And GRASS 7 is not new, it is actually many
years old. Therefore I would rather like to see get 7 released soon
and synchronize raster and vector libs in GRASS 8. For example,
something I am really missing in GRASS raster data is the history of
GRASS vector data. Also some more raster metadata like number of
non-null cells, mean and stddev (see GDAL) would be really helpful.
Not to mention the organization of raster file storage: have one
raster folder like for vector.

completely agreed. Martin

--
Martin Landa * http://geo.fsv.cvut.cz/gwiki/Landa

12

2014-05-07 23:48 GMT+02:00 Glynn Clements <glynn@gclements.plus.com>:

which in turn sounds like something for GRASS 8.

FWIW, the first 6.0 beta was tagged on 2005-01-12, i.e. over 9 years
ago. Unless there's a reason to believe that 7.0->8.0 will happen
somewhat quicker than 6.0->7.0, 8.0 is too long to wait.

this could be related to our release policy which I hope that will
change with GRASS 7. In other words trunk can become GRASS 8 sooner
compared to 6->7. We don't need to wait.

Martin

--
Martin Landa * http://geo.fsv.cvut.cz/gwiki/Landa

13

On Wed, May 7, 2014 at 5:48 PM, Glynn Clements <glynn@gclements.plus.com>wrote:

Markus Metz wrote:

> > If so, I suggest changing that, however invasive it may be.
>
> Very invasive.

Probably not. Admittedly, it would affect a lot of code, but only in
fairly trivial ways. Specifically, all of the "Map" variables would
become pointers rather than structures, calls to vector functions will
have "&Map" changed to "Map", and the open functions will return a
pointer rather than having one passed in.

Although, we could probably get away with just storing a table of
pointers. In theory, callers might move/copy the Map_info structures;
in practice, they don't.

> > I'd
> > certainly suggest holding off on any 7.0 release until that's
> > resolved.
>
> Hmm. This sounds like synchronizing some basic concepts of the raster
> and vector libraries,

Not really; it's just fixing a design flaw in the 6.x vector API.

Apart from the issue of the vector library not having a list of open
maps, another issue is that the size (and occasionally[1] the layout)
of the Map_info structure gets baked into the modules, so anything
which changes the layout of the structure constitutes a change to the
ABI.

[1] e.g. v.info references a couple of fields directly, rather than
using the appropriate accessor functions.

Ideally, the size and layout of the Map_info structure should only be
known to the vector libraries. The public headers should declare
Map_info as an abstract structure ("incomplete type" in C parlance).

Why don't we keep an array of Map_info only internally (private to
libvect), just like R__.fileinfo, and use an integer map descriptor at high
level? Then we can implement accessor functions, only through which high
level routines can read/write map properties.

> which in turn sounds like something for GRASS 8.

FWIW, the first 6.0 beta was tagged on 2005-01-12, i.e. over 9 years
ago. Unless there's a reason to believe that 7.0->8.0 will happen
somewhat quicker than 6.0->7.0, 8.0 is too long to wait.

I think it all depends on how we want to change the vector lib. IMHO, only
adding an array of open Map_infos would not be very invasive, but
synchronizing with librast (e.g., using a map descriptor and hiding
Map_info) would take a lot more work and need to wait until 8.0.

Huidae

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

14

On Thu, May 8, 2014 at 6:20 AM, Martin Landa <landa.martin@gmail.com> wrote:

2014-05-07 23:48 GMT+02:00 Glynn Clements <glynn@gclements.plus.com>:
>> which in turn sounds like something for GRASS 8.
>
> FWIW, the first 6.0 beta was tagged on 2005-01-12, i.e. over 9 years
> ago. Unless there's a reason to believe that 7.0->8.0 will happen
> somewhat quicker than 6.0->7.0, 8.0 is too long to wait.

this could be related to our release policy which I hope that will
change with GRASS 7. In other words trunk can become GRASS 8 sooner
compared to 6->7. We don't need to wait.

This is something I would prefer to do in a separate branch. Doing this in
trunk (trunk as G8) would lead to divergence between branches and similar
(similarly bad IMO) situation as we have/had with 6.4 (release), 6.5 (devel
for release) and trunk (the real devel).

15

Huidae Cho wrote:

Why don't we keep an array of Map_info only internally (private to
libvect), just like R__.fileinfo, and use an integer map descriptor at high
level? Then we can implement accessor functions, only through which high
level routines can read/write map properties.

It doesn't necessarily have to be an integer (although that would
avoid the issue of code bypassing the accessor functions and
dereferencing the pointer locally).

The fact that an integer is used for raster maps is largely a
historical accident. Prior to 7.0, the "descriptor" was the actual
file descriptor of the cell/fcell file (hence the common use of the
name "fd" for the variable holding the descriptor). As file
descriptors are small, non-negative integers, this could be (and was)
used as the index into a table which held other information about the
map.

In 7.0, the "descriptor" is only the index into the table
(R__.fileinfo[fd]). The actual file descriptor for the cell/fcell file
is stored in R__.fileinfo[fd].data_fd. This avoids wasting a slot for
each open file which isn't a raster map's cell/fcell file.

> > which in turn sounds like something for GRASS 8.
>
> FWIW, the first 6.0 beta was tagged on 2005-01-12, i.e. over 9 years
> ago. Unless there's a reason to believe that 7.0->8.0 will happen
> somewhat quicker than 6.0->7.0, 8.0 is too long to wait.

I think it all depends on how we want to change the vector lib. IMHO, only
adding an array of open Map_infos would not be very invasive, but
synchronizing with librast (e.g., using a map descriptor and hiding
Map_info) would take a lot more work and need to wait until 8.0.

Provided that modules (or higher-level libraries) don't try to do
anything unexpected with the Map_info structures, we could just keep a
table of pointers.

Problems only arise if code does something like passing in a pointer
to one structure when opening a map, copying it, then passing in a
pointer to the copy when closing it. With the existing API, this would
probably work.

I find it hard to envisage code actually doing that, but it's not
something which could easily be checked statically (i.e. by analysing
the code). We'd have to implement the internal table-of-pointers, add
code to check that any Map_info pointers passed to other functions
were already in the table, then run tests (or rely upon the users
reporting any warnings).

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

16

I think that it’s better to hide implementation details such as the Map_info structure, then we don’t have to worry about user code making unexpected changes in the Map_info structures. As long as we keep track of open maps, close/delete them when needed and implement the accessor functions, the user doesn’t need to access Map_info, I think. Are there any good reasons why we have to expose the Map_info structure to module developers?

Huidae

···

On Thu, May 8, 2014 at 12:01 PM, Glynn Clements <glynn@gclements.plus.com> wrote:

Huidae Cho wrote:

Why don’t we keep an array of Map_info only internally (private to
libvect), just like R__.fileinfo, and use an integer map descriptor at high
level? Then we can implement accessor functions, only through which high
level routines can read/write map properties.

It doesn’t necessarily have to be an integer (although that would
avoid the issue of code bypassing the accessor functions and
dereferencing the pointer locally).

The fact that an integer is used for raster maps is largely a
historical accident. Prior to 7.0, the “descriptor” was the actual
file descriptor of the cell/fcell file (hence the common use of the
name “fd” for the variable holding the descriptor). As file
descriptors are small, non-negative integers, this could be (and was)
used as the index into a table which held other information about the
map.

In 7.0, the “descriptor” is only the index into the table
(R__.fileinfo[fd]). The actual file descriptor for the cell/fcell file
is stored in R__.fileinfo[fd].data_fd. This avoids wasting a slot for
each open file which isn’t a raster map’s cell/fcell file.

which in turn sounds like something for GRASS 8.

FWIW, the first 6.0 beta was tagged on 2005-01-12, i.e. over 9 years
ago. Unless there’s a reason to believe that 7.0->8.0 will happen
somewhat quicker than 6.0->7.0, 8.0 is too long to wait.

I think it all depends on how we want to change the vector lib. IMHO, only
adding an array of open Map_infos would not be very invasive, but
synchronizing with librast (e.g., using a map descriptor and hiding
Map_info) would take a lot more work and need to wait until 8.0.

Provided that modules (or higher-level libraries) don’t try to do
anything unexpected with the Map_info structures, we could just keep a
table of pointers.

Problems only arise if code does something like passing in a pointer
to one structure when opening a map, copying it, then passing in a
pointer to the copy when closing it. With the existing API, this would
probably work.

I find it hard to envisage code actually doing that, but it’s not
something which could easily be checked statically (i.e. by analysing
the code). We’d have to implement the internal table-of-pointers, add
code to check that any Map_info pointers passed to other functions
were already in the table, then run tests (or rely upon the users
reporting any warnings).

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

17

Huidae Cho wrote:

Are there any good reasons why we have to expose the Map_info
structure to module developers?

No.

It's only that way because changing it would be a fair amount of
work[1], and after ~9 years of 7.0 being considered something for the
distant future, suddenly it needs to be ready for pre-release Real Soon.

[1] None of it should be particularly difficult, but it's not quite
simple enough to do with a sed script.

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