[GRASS-dev] lib/vector/Vlib/*.c: Too much uses of `GRASS_VECT_DIRECTORY'

GRASS GRASS developer
1

  May I suggest the change below?

  Basically, this one introduces three tiny new functions to be
  used instead of coding the construction of the vector
  map-related filenames explicitly. Like:

- sprintf (buf, "%s/%s", GRASS_VECT_DIRECTORY, map_info->name);
+ Vect_map_file_name_rel (buf, map_info);

  I wasn't sure where to declare these functions, so I've added
  the prototypes to `include/Vect.h'.

Index: include/Vect.h

--- include/Vect.h (revision 36440)
+++ include/Vect.h (working copy)
@@ -218,6 +218,13 @@
int Vect_rewind(struct Map_info *);
int Vect_close(struct Map_info *);

+/* filenames */
+int Vect_map_file_name_rel (char [GPATH_MAX], const struct Map_info *);
+int Vect_map_element_file_name_rel (char [GPATH_MAX],
+ const struct Map_info *, const char *);
+int Vect_map_element_file_name_abs (char [GPATH_MAX],
+ const struct Map_info *, const char *);
+
/* Read/write lines, nodes, areas */
/* Level 1 and 2 */
int Vect_read_next_line(struct Map_info *, struct line_pnts *,
Index: lib/vector/Vlib/file_name.c

--- lib/vector/Vlib/file_name.c (revision 0)
+++ lib/vector/Vlib/file_name.c (revision 0)
@@ -0,0 +1,48 @@
+/*!
+ \file file_name.c
+
+ \brief GIS library - Determine GRASS data base / vector file name
+
+ (C) 2009 by the GRASS Development Team
+
+ This program is free software under the GNU General Public License as
+ published by the Free Software Foundation; either version 2 of the
+ License, or (at your option) any later version.
+
+ Read the file COPYING that comes with GRASS
+ for details.
+ */
+
+#include <string.h>
+#include <grass/gis.h>
+#include <grass/Vect.h>
+
+int
+Vect_map_file_name_rel (char result[GPATH_MAX],
+ const struct Map_info *map)
+{
+ return
+ sprintf (result, "%s/%s",
+ GRASS_VECT_DIRECTORY, map->name);
+}
+
+int
+Vect_map_element_file_name_rel (char result[GPATH_MAX],
+ const struct Map_info *map,
+ const char *element)
+{
+ return
+ sprintf (result, "%s/%s/%s",
+ GRASS_VECT_DIRECTORY, map->name, element);
+}
+
+int
+Vect_map_element_file_name_abs (char result[GPATH_MAX],
+ const struct Map_info *map,
+ const char *element)
+{
+ return
+ sprintf (result, "%s/%s/%s/%s/%s/%s",
+ map->gisdbase, map->location, map->mapset,
+ GRASS_VECT_DIRECTORY, map->name, element);
+}
Index: lib/vector/Vlib/build.c

--- lib/vector/Vlib/build.c (revision 36440)
+++ lib/vector/Vlib/build.c (working copy)
@@ -247,7 +247,7 @@
     plus = &(Map->plus);

     /* write out all the accumulated info to the plus file */
- sprintf(buf, "%s/%s", GRASS_VECT_DIRECTORY, Map->name);
+ Vect_map_file_name_rel (buf, Map);
     G__file_name(fname, buf, GV_TOPO_ELEMENT, Map->mapset);
     G_debug(1, "Open topo: %s", fname);
     dig_file_init(&fp);
@@ -398,7 +398,7 @@
     plus = &(Map->plus);

     /* write out rtrees to the sidx file */
- sprintf(buf, "%s/%s", GRASS_VECT_DIRECTORY, Map->name);
+ Vect_map_file_name_rel (buf, Map);
     G__file_name(fname, buf, GV_SIDX_ELEMENT, Map->mapset);
     G_debug(1, "Open sidx: %s", fname);
     dig_file_init(&fp);
Index: lib/vector/Vlib/close.c

--- lib/vector/Vlib/close.c (revision 36440)
+++ lib/vector/Vlib/close.c (working copy)
@@ -77,7 +77,7 @@
   struct stat info;

   /* Delete old support files if available */
- sprintf(buf, "%s/%s", GRASS_VECT_DIRECTORY, Map->name);
+ Vect_map_file_name_rel (buf, Map);

   G__file_name(file_path, buf, GV_TOPO_ELEMENT, G_mapset());
   if (stat(file_path, &info) == 0) /* file exists? */
Index: lib/vector/Vlib/open_nat.c

--- lib/vector/Vlib/open_nat.c (revision 36440)
+++ lib/vector/Vlib/open_nat.c (working copy)
@@ -49,7 +49,7 @@
     G_debug(1, "V1_open_old_nat(): name = %s mapset = %s", Map->name,
       Map->mapset);

- sprintf(buf, "%s/%s", GRASS_VECT_DIRECTORY, Map->name);
+ Vect_map_file_name_rel (buf, Map);
     dig_file_init(&(Map->dig_fp));
     if (update)
   Map->dig_fp.file = G_fopen_modify(buf, GRASS_VECT_COOR_ELEMENT);
Index: lib/vector/Vlib/close_ogr.c

--- lib/vector/Vlib/close_ogr.c (revision 36440)
+++ lib/vector/Vlib/close_ogr.c (working copy)
@@ -73,7 +73,7 @@

     if (strcmp(Map->mapset, G_mapset()) == 0 && Map->support_updated &&
   Map->plus.built == GV_BUILD_ALL) {
- sprintf(elem, "%s/%s", GRASS_VECT_DIRECTORY, Map->name);
+ Vect_map_file_name_rel (elem, Map);
   G__file_name(fname, elem, "fidx", Map->mapset);
   G_debug(4, "Open fidx: %s", fname);
   dig_file_init(&fp);
Index: lib/vector/Vlib/cindex.c

--- lib/vector/Vlib/cindex.c (revision 36440)
+++ lib/vector/Vlib/cindex.c (working copy)
@@ -451,7 +451,7 @@

     plus = &(Map->plus);

- sprintf(buf, "%s/%s", GRASS_VECT_DIRECTORY, Map->name);
+ Vect_map_file_name_rel (buf, Map);
     G__file_name(fname, buf, GV_CIDX_ELEMENT, Map->mapset);
     G_debug(2, "Open cidx: %s", fname);
     dig_file_init(&fp);
@@ -497,7 +497,7 @@

     Plus = &(Map->plus);

- sprintf(buf, "%s/%s", GRASS_VECT_DIRECTORY, Map->name);
+ Vect_map_file_name_rel (buf, Map);
     G__file_name(file_path, buf, GV_CIDX_ELEMENT, Map->mapset);

     if (stat(file_path, &info) != 0) /* does not exist */
Index: lib/vector/Vlib/open_ogr.c

--- lib/vector/Vlib/open_ogr.c (revision 36440)
+++ lib/vector/Vlib/open_ogr.c (working copy)
@@ -122,7 +122,7 @@

     G_debug(3, "V2_open_old_ogr()");

- sprintf(elem, "%s/%s", GRASS_VECT_DIRECTORY, Map->name);
+ Vect_map_file_name_rel (elem, Map);
     dig_file_init(&fp);
     fp.file = G_fopen_old(elem, "fidx", Map->mapset);
     if (fp.file == NULL) {
Index: lib/vector/Vlib/field.c

--- lib/vector/Vlib/field.c (revision 36440)
+++ lib/vector/Vlib/field.c (working copy)
@@ -603,9 +603,7 @@
           Map->format);
     }

- sprintf(file, "%s/%s/%s/%s/%s/%s", Map->gisdbase, Map->location,
- Map->mapset, GRASS_VECT_DIRECTORY, Map->name,
- GRASS_VECT_DBLN_ELEMENT);
+ Vect_map_element_file_name_abs (file, Map, GRASS_VECT_DBLN_ELEMENT);
     G_debug(1, "dbln file: %s", file);

     fd = fopen(file, "r");
@@ -676,9 +680,7 @@

     dbl = Map->dblnk;

- sprintf(file, "%s/%s/%s/%s/%s/%s", Map->gisdbase, Map->location,
- Map->mapset, GRASS_VECT_DIRECTORY, Map->name,
- GRASS_VECT_DBLN_ELEMENT);
+ Vect_map_element_file_name_abs (file, Map, GRASS_VECT_DBLN_ELEMENT);
     G_debug(1, "dbln file: %s", file);

     fd = fopen(file, "w");
Index: lib/vector/Vlib/header.c

--- lib/vector/Vlib/header.c (revision 36440)
+++ lib/vector/Vlib/header.c (working copy)
@@ -88,7 +88,7 @@
     char buf[200];
     FILE *head_fp;

- sprintf(buf, "%s/%s", GRASS_VECT_DIRECTORY, Map->name);
+ Vect_map_file_name_rel (buf, Map);

     head_fp = G_fopen_new(buf, GRASS_VECT_HEAD_ELEMENT);
     if (head_fp == NULL) {
@@ -137,7 +137,7 @@
     Vect_set_thresh(Map, 0.);

     G_debug(1, "Vect__read_head(): vector = %s@%s", Map->name, Map->mapset);
- sprintf(buff, "%s/%s", GRASS_VECT_DIRECTORY, Map->name);
+ Vect_map_file_name_rel (buff, Map);
     head_fp = G_fopen_old(buff, GRASS_VECT_HEAD_ELEMENT, Map->mapset);
     if (head_fp == NULL) {
   G_warning(_("Unable to open header file of vector <%s>"),

--
FSF associate member #7257

2

If it's tested, at least I have no objections.
Not sure if it will give problems when char buf[200] is treated as char result[GPATH_MAX].

Just two remarks. First, please work with latest svn version which is r36445 since 2009-03-21 10:19:54 +0100 i.e. yesterday, because I'm also busy with the vector libs. Second, I have already changed G_find_file2 to G_find_vector2 in Vect_open_new.

Best regards,

Markus M

Ivan Shmakov wrote:

  May I suggest the change below?

  Basically, this one introduces three tiny new functions to be
  used instead of coding the construction of the vector
  map-related filenames explicitly. Like:

- sprintf (buf, "%s/%s", GRASS_VECT_DIRECTORY, map_info->name);
+ Vect_map_file_name_rel (buf, map_info);

  I wasn't sure where to declare these functions, so I've added
  the prototypes to `include/Vect.h'.

Index: include/Vect.h

--- include/Vect.h (revision 36440)
+++ include/Vect.h (working copy)
@@ -218,6 +218,13 @@
int Vect_rewind(struct Map_info *);
int Vect_close(struct Map_info *);
+/* filenames */
+int Vect_map_file_name_rel (char [GPATH_MAX], const struct Map_info *);
+int Vect_map_element_file_name_rel (char [GPATH_MAX],
+ const struct Map_info *, const char *);
+int Vect_map_element_file_name_abs (char [GPATH_MAX],
+ const struct Map_info *, const char *);
+
/* Read/write lines, nodes, areas */
/* Level 1 and 2 */
int Vect_read_next_line(struct Map_info *, struct line_pnts *,
Index: lib/vector/Vlib/file_name.c

--- lib/vector/Vlib/file_name.c (revision 0)
+++ lib/vector/Vlib/file_name.c (revision 0)
@@ -0,0 +1,48 @@
+/*!
+ \file file_name.c
+
+ \brief GIS library - Determine GRASS data base / vector file name
+
+ (C) 2009 by the GRASS Development Team
+
+ This program is free software under the GNU General Public License as
+ published by the Free Software Foundation; either version 2 of the
+ License, or (at your option) any later version.
+
+ Read the file COPYING that comes with GRASS
+ for details.
+ */
+
+#include <string.h>
+#include <grass/gis.h>
+#include <grass/Vect.h>
+
+int
+Vect_map_file_name_rel (char result[GPATH_MAX],
+ const struct Map_info *map)
+{
+ return
+ sprintf (result, "%s/%s",
+ GRASS_VECT_DIRECTORY, map->name);
+}
+
+int
+Vect_map_element_file_name_rel (char result[GPATH_MAX],
+ const struct Map_info *map,
+ const char *element)
+{
+ return
+ sprintf (result, "%s/%s/%s",
+ GRASS_VECT_DIRECTORY, map->name, element);
+}
+
+int
+Vect_map_element_file_name_abs (char result[GPATH_MAX],
+ const struct Map_info *map,
+ const char *element)
+{
+ return
+ sprintf (result, "%s/%s/%s/%s/%s/%s",
+ map->gisdbase, map->location, map->mapset,
+ GRASS_VECT_DIRECTORY, map->name, element);
+}
Index: lib/vector/Vlib/build.c

--- lib/vector/Vlib/build.c (revision 36440)
+++ lib/vector/Vlib/build.c (working copy)
@@ -247,7 +247,7 @@
     plus = &(Map->plus);
      /* write out all the accumulated info to the plus file */
- sprintf(buf, "%s/%s", GRASS_VECT_DIRECTORY, Map->name);
+ Vect_map_file_name_rel (buf, Map);
     G__file_name(fname, buf, GV_TOPO_ELEMENT, Map->mapset);
     G_debug(1, "Open topo: %s", fname);
     dig_file_init(&fp);
@@ -398,7 +398,7 @@
     plus = &(Map->plus);
      /* write out rtrees to the sidx file */
- sprintf(buf, "%s/%s", GRASS_VECT_DIRECTORY, Map->name);
+ Vect_map_file_name_rel (buf, Map);
     G__file_name(fname, buf, GV_SIDX_ELEMENT, Map->mapset);
     G_debug(1, "Open sidx: %s", fname);
     dig_file_init(&fp);
Index: lib/vector/Vlib/close.c

--- lib/vector/Vlib/close.c (revision 36440)
+++ lib/vector/Vlib/close.c (working copy)
@@ -77,7 +77,7 @@
   struct stat info;
    /* Delete old support files if available */
- sprintf(buf, "%s/%s", GRASS_VECT_DIRECTORY, Map->name);
+ Vect_map_file_name_rel (buf, Map);
    G__file_name(file_path, buf, GV_TOPO_ELEMENT, G_mapset());
   if (stat(file_path, &info) == 0) /* file exists? */
Index: lib/vector/Vlib/open_nat.c

--- lib/vector/Vlib/open_nat.c (revision 36440)
+++ lib/vector/Vlib/open_nat.c (working copy)
@@ -49,7 +49,7 @@
     G_debug(1, "V1_open_old_nat(): name = %s mapset = %s", Map->name,
       Map->mapset);
- sprintf(buf, "%s/%s", GRASS_VECT_DIRECTORY, Map->name);
+ Vect_map_file_name_rel (buf, Map);
     dig_file_init(&(Map->dig_fp));
     if (update)
   Map->dig_fp.file = G_fopen_modify(buf, GRASS_VECT_COOR_ELEMENT);
Index: lib/vector/Vlib/close_ogr.c

--- lib/vector/Vlib/close_ogr.c (revision 36440)
+++ lib/vector/Vlib/close_ogr.c (working copy)
@@ -73,7 +73,7 @@
      if (strcmp(Map->mapset, G_mapset()) == 0 && Map->support_updated &&
   Map->plus.built == GV_BUILD_ALL) {
- sprintf(elem, "%s/%s", GRASS_VECT_DIRECTORY, Map->name);
+ Vect_map_file_name_rel (elem, Map);
   G__file_name(fname, elem, "fidx", Map->mapset);
   G_debug(4, "Open fidx: %s", fname);
   dig_file_init(&fp);
Index: lib/vector/Vlib/cindex.c

--- lib/vector/Vlib/cindex.c (revision 36440)
+++ lib/vector/Vlib/cindex.c (working copy)
@@ -451,7 +451,7 @@
      plus = &(Map->plus);
- sprintf(buf, "%s/%s", GRASS_VECT_DIRECTORY, Map->name);
+ Vect_map_file_name_rel (buf, Map);
     G__file_name(fname, buf, GV_CIDX_ELEMENT, Map->mapset);
     G_debug(2, "Open cidx: %s", fname);
     dig_file_init(&fp);
@@ -497,7 +497,7 @@
      Plus = &(Map->plus);
- sprintf(buf, "%s/%s", GRASS_VECT_DIRECTORY, Map->name);
+ Vect_map_file_name_rel (buf, Map);
     G__file_name(file_path, buf, GV_CIDX_ELEMENT, Map->mapset);
      if (stat(file_path, &info) != 0) /* does not exist */
Index: lib/vector/Vlib/open_ogr.c

--- lib/vector/Vlib/open_ogr.c (revision 36440)
+++ lib/vector/Vlib/open_ogr.c (working copy)
@@ -122,7 +122,7 @@
      G_debug(3, "V2_open_old_ogr()");
- sprintf(elem, "%s/%s", GRASS_VECT_DIRECTORY, Map->name);
+ Vect_map_file_name_rel (elem, Map);
     dig_file_init(&fp);
     fp.file = G_fopen_old(elem, "fidx", Map->mapset);
     if (fp.file == NULL) {
Index: lib/vector/Vlib/field.c

--- lib/vector/Vlib/field.c (revision 36440)
+++ lib/vector/Vlib/field.c (working copy)
@@ -603,9 +603,7 @@
           Map->format);
     }
- sprintf(file, "%s/%s/%s/%s/%s/%s", Map->gisdbase, Map->location,
- Map->mapset, GRASS_VECT_DIRECTORY, Map->name,
- GRASS_VECT_DBLN_ELEMENT);
+ Vect_map_element_file_name_abs (file, Map, GRASS_VECT_DBLN_ELEMENT);
     G_debug(1, "dbln file: %s", file);
      fd = fopen(file, "r");
@@ -676,9 +680,7 @@
      dbl = Map->dblnk;
- sprintf(file, "%s/%s/%s/%s/%s/%s", Map->gisdbase, Map->location,
- Map->mapset, GRASS_VECT_DIRECTORY, Map->name,
- GRASS_VECT_DBLN_ELEMENT);
+ Vect_map_element_file_name_abs (file, Map, GRASS_VECT_DBLN_ELEMENT);
     G_debug(1, "dbln file: %s", file);
      fd = fopen(file, "w");
Index: lib/vector/Vlib/header.c

--- lib/vector/Vlib/header.c (revision 36440)
+++ lib/vector/Vlib/header.c (working copy)
@@ -88,7 +88,7 @@
     char buf[200];
     FILE *head_fp;
- sprintf(buf, "%s/%s", GRASS_VECT_DIRECTORY, Map->name);
+ Vect_map_file_name_rel (buf, Map);
      head_fp = G_fopen_new(buf, GRASS_VECT_HEAD_ELEMENT);
     if (head_fp == NULL) {
@@ -137,7 +137,7 @@
     Vect_set_thresh(Map, 0.);
      G_debug(1, "Vect__read_head(): vector = %s@%s", Map->name, Map->mapset);
- sprintf(buff, "%s/%s", GRASS_VECT_DIRECTORY, Map->name);
+ Vect_map_file_name_rel (buff, Map);
     head_fp = G_fopen_old(buff, GRASS_VECT_HEAD_ELEMENT, Map->mapset);
     if (head_fp == NULL) {
   G_warning(_("Unable to open header file of vector <%s>"),

3

Markus Metz <markus.metz.giswork@googlemail.com> writes:

>> May I suggest the change below?

>> Basically, this one introduces three tiny new functions to be used
>> instead of coding the construction of the vector map-related
>> filenames explicitly. Like:

>> - sprintf (buf, "%s/%s", GRASS_VECT_DIRECTORY, map_info->name);
>> + Vect_map_file_name_rel (buf, map_info);

> If it's tested, at least I have no objections.

  Any ideas on how to perform the testing? I'm going to try to
  perform some operations on the vectors in my data sets, but I
  just can't think out anything better than that (to do in a
  reasonable time.)

> Not sure if it will give problems when char buf[200] is treated as
> char result[GPATH_MAX].

  That's the problem of the already existing code, which is going
  to show up when someone will try to work with vector names
  longer than 192 characters. Obviously, 200 should be replaced
  with GPATH_MAX. That should be a separate patch.

  One reason behind these functions was to enable the compiler to
  recognize the errors like that and possibly emit a warning.

> Just two remarks. First, please work with latest svn version which
> is r36445 since 2009-03-21 10:19:54 +0100 i.e. yesterday, because I'm
> also busy with the vector libs.

  ACK.

> Second, I have already changed G_find_file2 to G_find_vector2 in
> Vect_open_new.

  ACK. Thanks.

[...]

--
FSF associate member #7257

4

Ivan Shmakov wrote:

>> Basically, this one introduces three tiny new functions to be used
>> instead of coding the construction of the vector map-related
>> filenames explicitly. Like:

>> - sprintf (buf, "%s/%s", GRASS_VECT_DIRECTORY, map_info->name);
>> + Vect_map_file_name_rel (buf, map_info);

> If it's tested, at least I have no objections.

  Any ideas on how to perform the testing? I'm going to try to
  perform some operations on the vectors in my data sets, but I
  just can't think out anything better than that (to do in a
  reasonable time.)
  

No better ideas here, but maybe some other developers have some ideas (apart from using very long vector names).

> Not sure if it will give problems when char buf[200] is treated as
> char result[GPATH_MAX].

  That's the problem of the already existing code, which is going
  to show up when someone will try to work with vector names
  longer than 192 characters. Obviously, 200 should be replaced
  with GPATH_MAX. That should be a separate patch.
  

Interesting. Maybe fix that first because it is a potential bug, i.e. use GNAME_MAX, GMAPSET_MAX, and GPATH_MAX wherever appropriate? As I understand, your patch is an improvement but not a bugfix, so I would opt for fixing (potential) bugs first, then improve. Maybe wait for a second opinion, I'm still new here.

> Second, I have already changed G_find_file2 to G_find_vector2 in
> Vect_open_new.

  ACK. Thanks.
  

I think it's better if I don't touch these parts anymore as long as you are busy with it, just to avoid confusion, and leave vector name handling to you.

Best regards,

Markus M

5

Markus Metz <markus.metz.giswork@googlemail.com> writes:

[...]

>>> Not sure if it will give problems when char buf[200] is treated as
>>> char result[GPATH_MAX].

>> That's the problem of the already existing code, which is going to
>> show up when someone will try to work with vector names longer than
>> 192 characters. Obviously, 200 should be replaced with GPATH_MAX.
>> That should be a separate patch.

> Interesting. Maybe fix that first because it is a potential bug,
> i.e. use GNAME_MAX, GMAPSET_MAX, and GPATH_MAX wherever appropriate?
> As I understand, your patch is an improvement but not a bugfix, so I
> would opt for fixing (potential) bugs first, then improve. Maybe wait
> for a second opinion, I'm still new here.

  It may make sense.

>>> Second, I have already changed G_find_file2 to G_find_vector2 in
>>> Vect_open_new.

>> ACK. Thanks.

> I think it's better if I don't touch these parts anymore as long as
> you are busy with it, just to avoid confusion, and leave vector name
> handling to you.

  Actually, what I was going to work on is the /raster/ support.
  I've briefly scanned over the vector support code to get some
  clues, and spotted some rough places. Therefore, I don't think
  that I'll be doing much more work on the vector parts of the
  library anytime soon.

--
FSF associate member #7257