Re: [GRASS-dev] [GRASS-SVN] r60048 - grass/trunk/vector/v.in.db
On Fri, May 2, 2014 at 11:19 PM, Huidae Cho gras...@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 gras...@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 gras...@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.mar...@gmail.com wrote: Hi, 2014-05-02 18:53 GMT+02:00 svn_gr...@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 ___ grass-dev mailing list grass-dev@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/grass-dev
Re: [GRASS-dev] [GRASS-SVN] r60048 - grass/trunk/vector/v.in.db
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 gl...@gclements.plus.com ___ grass-dev mailing list grass-dev@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/grass-dev
Re: [GRASS-dev] [GRASS-SVN] r60048 - grass/trunk/vector/v.in.db
Hi, 2014-05-02 18:53 GMT+02:00 svn_gr...@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
Re: [GRASS-dev] [GRASS-SVN] r60048 - grass/trunk/vector/v.in.db
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.mar...@gmail.comwrote: Hi, 2014-05-02 18:53 GMT+02:00 svn_gr...@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
Re: [GRASS-dev] [GRASS-SVN] r60048 - grass/trunk/vector/v.in.db
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 gras...@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.mar...@gmail.comwrote: Hi, 2014-05-02 18:53 GMT+02:00 svn_gr...@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
Re: [GRASS-dev] [GRASS-SVN] r60048 - grass/trunk/vector/v.in.db
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 gras...@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 gras...@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.mar...@gmail.comwrote: Hi, 2014-05-02 18:53 GMT+02:00 svn_gr...@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
