[GRASS-dev] [GRASS-SVN] r60048 - grass/trunk/vector/v.in.db

martinl · May 2, 2014, 4:59pm

Hi,

2014-05-02 18:53 GMT+02:00 <svn_grass@osgeo.org>:

- Vect_open_new(&Map, outvect->answer, with_z);
+ if (Vect_open_new(&Map, outvect->answer, with_z) == -1)
+ exit(EXIT_FAILURE);
+

please provide more info where it fails. Also note that Vect_open_new
calls G_fatal_error() so the most of modules don't check return code
of this function.

Martin

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

Huidae_Cho · May 2, 2014, 5:15pm

Segmentation fault when you do v.in.db … output=a@other_mapset because Vect_open_new returns -1 with a warning, not a fatal error, “unable to create new … is not the current mapset”. I didn’t check why it’s returning -1 instead of throwing a fatal error in this case. I believe that this warning has to be a fatal error if no modules rely on -1 return.

Huidae

···

On Fri, May 2, 2014 at 12:59 PM, Martin Landa <landa.martin@gmail.com> wrote:

Hi,

2014-05-02 18:53 GMT+02:00 <svn_grass@osgeo.org>:

Vect_open_new(&Map, outvect->answer, with_z);

if (Vect_open_new(&Map, outvect->answer, with_z) == -1)

exit(EXIT_FAILURE);

please provide more info where it fails. Also note that Vect_open_new
calls G_fatal_error() so the most of modules don’t check return code
of this function.

Martin

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

Huidae_Cho · May 2, 2014, 5:18pm

More detail… When this happens, Map is not fully initialized and following Vect_* calls with Map can fail unexpectedly, which caused a segmentation fault in this case.

···

On Fri, May 2, 2014 at 1:15 PM, Huidae Cho <grass4u@gmail.com> wrote:

Segmentation fault when you do v.in.db … output=a@other_mapset because Vect_open_new returns -1 with a warning, not a fatal error, “unable to create new … is not the current mapset”. I didn’t check why it’s returning -1 instead of throwing a fatal error in this case. I believe that this warning has to be a fatal error if no modules rely on -1 return.

Huidae

On Fri, May 2, 2014 at 12:59 PM, Martin Landa <landa.martin@gmail.com> wrote:

Hi,

2014-05-02 18:53 GMT+02:00 <svn_grass@osgeo.org>:

Vect_open_new(&Map, outvect->answer, with_z);

if (Vect_open_new(&Map, outvect->answer, with_z) == -1)

exit(EXIT_FAILURE);

please provide more info where it fails. Also note that Vect_open_new
calls G_fatal_error() so the most of modules don’t check return code
of this function.

Martin

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

Huidae_Cho · May 2, 2014, 9:19pm

OK, I did a quick search and there are 104 calls to Vect_open_new. 63 calls don’t check its return value and 41 calls check. 27 of the 41 that do the check do some cleaning work before finally throwing a fatal error. Most of the cleaning work is Vect_close(already open vectors), which I don’t think is really necessary before throwing a fatal error. v.convert fcloses Digin and v.surf.rst deletes left-over files. Even Vect_close returns 1. Based on these observations, I think Vect_open_new needs to return what it’s returning now on failure and the 63 calls that don’t check its return value have to be fixed to avoid a potential segmentation fault. I’ll fix them tonight if I didn’t overlook something.

53 files that don’t check the return value:

./display/d.extract/main.c
./lib/sites/sites.c
./raster/r.carve/vect.c
./raster/r.contour/main.c
./raster/r.random/random.c
./raster/r.stream.extract/close.c
./raster/r.stream.order/stream_vector.c
./raster/r.stream.segment/stream_vector.c

./raster/r.stream.snap/points_io.c
./raster/r.to.vect/main.c
./raster/r.volume/main.c
./raster/simwe/simlib/output.c
./vector/v.buffer/main.c
./vector/v.build.polylines/main.c
./vector/v.build/main.c
./vector/v.clean/main.c
./vector/v.distance/main.c
./vector/v.drape/main.c
./vector/v.edit/main.c
./vector/v.external/main.c
./vector/v.extract/main.c
./vector/v.extrude/main.c
./vector/v.in.ascii/main.c
./vector/v.in.dwg/main.c
./vector/v.in.lidar/main.c
./vector/v.in.ogr/main.c
./vector/v.in.region/main.c
./vector/v.in.sites/main.c
./vector/v.kernel/main.c
./vector/v.lrs/v.lrs.create/main.c
./vector/v.lrs/v.lrs.label/main.c
./vector/v.lrs/v.lrs.segment/main.c
./vector/v.net.alloc/main.c
./vector/v.net.iso/main.c
./vector/v.net.salesman/main.c
./vector/v.net.steiner/main.c
./vector/v.overlay/main.c
./vector/v.parallel/main.c
./vector/v.patch/main.c
./vector/v.perturb/main.c
./vector/v.proj/main.c
./vector/v.qcount/main.c
./vector/v.reclass/main.c
./vector/v.rectify/main.c
./vector/v.sample/main.c
./vector/v.segment/main.c
./vector/v.select/main.c
./vector/v.split/main.c
./vector/v.surf.rst/main.c
./vector/v.to.points/main.c
./vector/v.transform/main.c
./vector/v.vol.rst/main.c
./vector/v.voronoi/main.c

···

On Fri, May 2, 2014 at 1:18 PM, Huidae Cho <grass4u@gmail.com> wrote:

More detail… When this happens, Map is not fully initialized and following Vect_* calls with Map can fail unexpectedly, which caused a segmentation fault in this case.

On Fri, May 2, 2014 at 1:15 PM, Huidae Cho <grass4u@gmail.com> wrote:

Segmentation fault when you do v.in.db … output=a@other_mapset because Vect_open_new returns -1 with a warning, not a fatal error, “unable to create new … is not the current mapset”. I didn’t check why it’s returning -1 instead of throwing a fatal error in this case. I believe that this warning has to be a fatal error if no modules rely on -1 return.

Huidae

On Fri, May 2, 2014 at 12:59 PM, Martin Landa <landa.martin@gmail.com> wrote:

Hi,

2014-05-02 18:53 GMT+02:00 <svn_grass@osgeo.org>:

Vect_open_new(&Map, outvect->answer, with_z);

if (Vect_open_new(&Map, outvect->answer, with_z) == -1)

exit(EXIT_FAILURE);

please provide more info where it fails. Also note that Vect_open_new
calls G_fatal_error() so the most of modules don’t check return code
of this function.

Martin

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

Glynn_Clements1 · May 4, 2014, 7:23am

Huidae Cho wrote:

OK, I did a quick search and there are 104 calls to Vect_open_new. 63 calls
don't check its return value and 41 calls check.

This is the main reason why I replaced status returns with fatal
errors in much of lib/raster and lib/gis (r40209, r40217, r40701).

I propose that lib/vector takes the same approach (I didn't touch
lib/vector at the time because I'm not that familiar with it, and was
unsure as to its stability).

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

Huidae_Cho · May 4, 2014, 2:37pm

Glynn,

I agree with you that Vect_open needs to call fatal error just like Rast_open because there would be nothing to do except cleaning garbage when opening fails. Some modules actually delete temp files before quitting and http://lists.osgeo.org/pipermail/grass-dev/2013-April/062938.html this discussion may be just for that, I think.

BTW, I don’t see Rast_open_old returning a negative value on failure as documented in the developer’s manual. If this is clearly documented and fatal error is also mentioned, it would save unnecessary conditional checks against Rast_open_old.

Just my 2 cents.
Huidae

On May 4, 2014 3:23 AM, “Glynn Clements” <glynn@gclements.plus.com> wrote:

Huidae Cho wrote:

OK, I did a quick search and there are 104 calls to Vect_open_new. 63 calls
don’t check its return value and 41 calls check.

This is the main reason why I replaced status returns with fatal
errors in much of lib/raster and lib/gis (r40209, r40217, r40701).

I propose that lib/vector takes the same approach (I didn’t touch
lib/vector at the time because I’m not that familiar with it, and was
unsure as to its stability).

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

Glynn_Clements1 · May 5, 2014, 12:44pm

Huidae Cho wrote:

BTW, I don't see Rast_open_old returning a negative value on failure as
documented in the developer's manual. If this is clearly documented and
fatal error is also mentioned, it would save unnecessary conditional checks
against Rast_open_old.

The 6.x versions (G_open_cell_old() etc) are documented as returning
-1 on failure (at least in the Doxygen comments, which should end up
in the manual).

The 7.x versions (Rast_open_old() etc) raise fatal errors, so any
checks for a negative return value would be redundant. Such checks
should have been removed by r40215, but I can't rule out having missed
some.

In cases where the return value was purely a status indication,
eliminating the status return resulted in the return type changing
from int to void. Anything checking the (now-void) return value would
result in a compilation error, so I doubt that I missed any.

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

Markus_Metz · May 7, 2014, 6:31pm

On Fri, May 2, 2014 at 11:19 PM, Huidae Cho <grass4u@gmail.com> wrote:

OK, I did a quick search and there are 104 calls to Vect_open_new. 63 calls don't check its return value and 41 calls check. 27 of the 41 that do the check do some cleaning work before finally throwing a fatal error. Most of the cleaning work is Vect_close(already open vectors), which I don't think is really necessary before throwing a fatal error.

Yes, it is sometimes necessary to close database drivers that have
already been successfully opened, if only because of these annoying db
driver zombies staying behind on MS Windows, which might in turn be a
problem of how G_spawn_ex() is called, or in G_spawn_ex() itself. Also
GDAL and OGR like to have their opened databases properly closed,
otherwise confusing segmentation faults can occur.

Markus M

v.convert fcloses Digin and v.surf.rst deletes left-over files. Even Vect_close returns 1. Based on these observations, I think Vect_open_new needs to return what it's returning now on failure and the 63 calls that don't check its return value have to be fixed to avoid a potential segmentation fault. I'll fix them tonight if I didn't overlook something.

53 files that don't check the return value:

./display/d.extract/main.c
./lib/sites/sites.c
./raster/r.carve/vect.c
./raster/r.contour/main.c
./raster/r.random/random.c
./raster/r.stream.extract/close.c
./raster/r.stream.order/stream_vector.c
./raster/r.stream.segment/stream_vector.c
./raster/r.stream.snap/points_io.c
./raster/r.to.vect/main.c
./raster/r.volume/main.c
./raster/simwe/simlib/output.c
./vector/v.buffer/main.c
./vector/v.build.polylines/main.c
./vector/v.build/main.c
./vector/v.clean/main.c
./vector/v.distance/main.c
./vector/v.drape/main.c
./vector/v.edit/main.c
./vector/v.external/main.c
./vector/v.extract/main.c
./vector/v.extrude/main.c
./vector/v.in.ascii/main.c
./vector/v.in.dwg/main.c
./vector/v.in.lidar/main.c
./vector/v.in.ogr/main.c
./vector/v.in.region/main.c
./vector/v.in.sites/main.c
./vector/v.kernel/main.c
./vector/v.lrs/v.lrs.create/main.c
./vector/v.lrs/v.lrs.label/main.c
./vector/v.lrs/v.lrs.segment/main.c
./vector/v.net.alloc/main.c
./vector/v.net.iso/main.c
./vector/v.net.salesman/main.c
./vector/v.net.steiner/main.c
./vector/v.overlay/main.c
./vector/v.parallel/main.c
./vector/v.patch/main.c
./vector/v.perturb/main.c
./vector/v.proj/main.c
./vector/v.qcount/main.c
./vector/v.reclass/main.c
./vector/v.rectify/main.c
./vector/v.sample/main.c
./vector/v.segment/main.c
./vector/v.select/main.c
./vector/v.split/main.c
./vector/v.surf.rst/main.c
./vector/v.to.points/main.c
./vector/v.transform/main.c
./vector/v.vol.rst/main.c
./vector/v.voronoi/main.c

On Fri, May 2, 2014 at 1:18 PM, Huidae Cho <grass4u@gmail.com> wrote:

More detail.. When this happens, Map is not fully initialized and following Vect_* calls with Map can fail unexpectedly, which caused a segmentation fault in this case.

On Fri, May 2, 2014 at 1:15 PM, Huidae Cho <grass4u@gmail.com> wrote:

Segmentation fault when you do v.in.db ... output=a@other_mapset because Vect_open_new returns -1 with a warning, not a fatal error, "unable to create new ... is not the current mapset". I didn't check why it's returning -1 instead of throwing a fatal error in this case. I believe that this warning has to be a fatal error if no modules rely on -1 return.

Huidae

On Fri, May 2, 2014 at 12:59 PM, Martin Landa <landa.martin@gmail.com> wrote:

Hi,

2014-05-02 18:53 GMT+02:00 <svn_grass@osgeo.org>:

> - Vect_open_new(&Map, outvect->answer, with_z);
> + if (Vect_open_new(&Map, outvect->answer, with_z) == -1)
> + exit(EXIT_FAILURE);
> +

please provide more info where it fails. Also note that Vect_open_new
calls G_fatal_error() so the most of modules don't check return code
of this function.

Martin

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

_______________________________________________
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev