libgis: Distinguish dirs and objects in dir creation (#1681)

The G_make_mapset_element() function is used in several places to create
nested directories such as grid3/some_raster3d_name for particular maps/objects.
However, the original purpose of G_make_mapset_element() seems to be creating directories
(such as fcell, vector) for these objects (objects such as elevation or bridges).

This change introduces new API functions which make distinction between these scenarios.
One function is for what most of the original API calls element that is the directory
for objects of the same type. The other is for the objects themselves.

The three new functions replace two existing ones.
Two replace G_make_mapset_element() covering the two cases and one replaces
G_make_mapset_element_tmp() covering only the common directory case
because there is no direct use of this functionality in the current code.
In the new API, dir_object is for the vector/name case and object_group for the vector and fcell cases.
The vector/name case is a single function call with more parameters rather than two function calls
and replaces the use of %s/%s syntax which is what was used before.

Distinguishing the two scenarios allows the code to handle differently a race
condition during creation when another process creates the same directory
between the failed access call and mkdir call.

In case of directory such as fcell or vector, all we asked
for was that the directory exists at the end of the function call which
will be fulfilled even when mkdir happened in another process.

Same race condition for directories such as cell_misc/raster_map_name or vector/vector_map_name
is actual problem and reporting it sooner rather than later is advantageous in
detecting the issue (assuming that the issue is two processes are trying to create map of the same name).
However, this is not the new behavior for some vector files where %s/%s syntax is still used
to create the parent directories without making any distinctions about the purpose of the directory.
This is the behavior which the code had before 3c37460
which was intended to imporve error message, but it changed also the the on-creation-race behavior.

The underlying issue seems to be, at least partially, the unclear naming. Element sometimes refers to
the particular file such as fcell/raster_map_name, but in the G_make functions, it refers to the
directory of these files such as fcell. This new API does not use any great names,
but the idea is to refer to element group directories (such as cell) and specific
element files or directories (raster map piece, sub-directory for a specific vector map).
The hope is that in the new API, words directory, object, and type are used in somewhat common way.

Creation of misc elements (cell_misc and group) is now the two-phase creation of an object which is a dir. Function is internal (double underscore after G), used consistently, and documented such that element is the name of the concrete object, so keeping the name as is.

The documentation of functions is based on the current usage and new names although the terminology is not a new official terminology.

general/g.filename/main.c

@@ -89,7 +89,7 @@ int main(int argc, char *argv[])
                           " <%s> is not the current mapset (%s)",
                           element, flag_create->key, mapset, G_mapset());
         }
-        G_make_mapset_element(element);
+        G_make_mapset_object_group(element);
     }
 
     G_file_name(path, element, name, mapset);

include/grass/defs/gis.h

@@ -542,6 +542,9 @@ char *G_mapset_path(void);
 /* mapset_msc.c */
 int G_make_mapset_element(const char *);
 int G_make_mapset_element_tmp(const char *);
+int G_make_mapset_object_group(const char *);
+int G_make_mapset_dir_object(const char *, const char *);
+int G_make_mapset_object_group_tmp(const char *);
 int G__make_mapset_element_misc(const char *, const char *);
 int G_mapset_permissions(const char *);
 int G_mapset_permissions2(const char *, const char *, const char *);

lib/db/dbmi_base/default_name.c

@@ -111,7 +111,7 @@ int db_set_default_connection(void)
 	db_set_connection(&connection);
 
 	sprintf(buf, "%s/%s/dbf", G_location_path(), G_mapset());
-	G_make_mapset_element("dbf");
+	G_make_mapset_object_group("dbf");
     }
     else if (strcmp(DB_DEFAULT_DRIVER, "sqlite") == 0) {
 	/* Set default values and create sqlite db dir */
@@ -132,7 +132,7 @@ int db_set_default_connection(void)
 	  */
 	connection.databaseName =
 	    "$GISDBASE/$LOCATION_NAME/$MAPSET/sqlite/sqlite.db";
-	G_make_mapset_element("sqlite");
+	G_make_mapset_object_group("sqlite");
 	db_set_connection(&connection);
     }
     else

lib/gis/mapset_msc.c

@@ -20,6 +20,8 @@
 #include <grass/glocale.h>
 
 static int make_mapset_element(const char *, const char *);
+static int make_mapset_element_no_fail_on_race(const char *, const char *);
+static int make_mapset_element_impl(const char *, const char *, bool);
 
 /*!
    \brief Create element in the current mapset.
@@ -29,7 +31,11 @@ static int make_mapset_element(const char *, const char *);
    routine can be called even if the element already exists.
    
    Calls G_fatal_error() on failure.
-   
+
+   \deprecated
+   This function is deprecated due to confusion in element terminology.
+   Use G_make_mapset_object_group() or G_make_mapset_dir_object() instead.
+
    \param p_element element to be created in mapset
 
    \return 0 no element defined
@@ -43,12 +49,81 @@ int G_make_mapset_element(const char *p_element)
     return make_mapset_element(path, p_element);
 }
 
+/*!
+    \brief Create directory for group of elements of a given type.
+
+    Creates the specified element directory in the current mapset.
+    It will check for the existence of the element and do nothing
+    if it is found so this routine can be called even if the element
+    already exists to ensure that it exists.
+
+    If creation fails, but the directory exists after the failure,
+    the function reports success. Therefore, two processes creating
+    a directory in this way can work in parallel.
+
+    Calls G_fatal_error() on failure.
+
+    \param type object type (e.g., `cell`)
+
+    \return 0 no element defined
+    \return 1 on success
+
+    \sa G_make_mapset_dir_object()
+    \sa G_make_mapset_object_group_tmp()
+ */
+int G_make_mapset_object_group(const char *type)
+{
+    char path[GPATH_MAX];
+
+    G_file_name(path, NULL, NULL, G_mapset());
+    return make_mapset_element_no_fail_on_race(path, type);
+}
+
+/*!
+    \brief Create directory for an object of a given type.
+
+    Creates the specified element directory in the current mapset.
+    It will check for the existence of the element and do nothing
+    if it is found so this routine can be called even if the element
+    already exists to ensure that it exists.
+
+    Any failure to create it, including the case when it exists
+    (i.e., was created by another process after the existence test)
+    is considered a failure because two processes should not attempt
+    to create two objects of the same name (and type).
+
+    This function is for objects which are directories
+    (the function does not create files).
+
+    Calls G_fatal_error() on failure.
+
+    \param type object type (e.g., `vector`)
+    \param name object name (e.g., `bridges`)
+
+    \return 0 no element defined
+    \return 1 on success
+
+    \sa G_make_mapset_object_group()
+ */
+int G_make_mapset_dir_object(const char *type, const char *name)
+{
+    char path[GPATH_MAX];
+
+    G_make_mapset_object_group(type);
+    G_file_name(path, type, NULL, G_mapset());
+    return make_mapset_element(path, name);
+}
+
 /*!
    \brief Create element in the temporary directory.
 
    See G_file_name_tmp() for details.
 
-   \param p_element element to be created in mapset
+   \param p_element element to be created in mapset (e.g., `elevation`)
+
+   \note
+   Use G_make_mapset_object_group_tmp() for creating common, shared
+   directories which are for multiple concrete elements (objects).
 
    \return 0 no element defined
    \return 1 on success
@@ -61,7 +136,29 @@ int G_make_mapset_element_tmp(const char *p_element)
     return make_mapset_element(path, p_element);
 }
 
-int make_mapset_element(const char *p_path, const char *p_element)
+/*!
+    \brief Create directory for type of objects in the temporary directory.
+
+    See G_file_name_tmp() for details.
+
+    \param type object type (e.g., `cell`)
+
+    \note
+    Use G_make_mapset_object_group_tmp() for creating common, shared
+    directories which are for multiple concrete elements (objects).
+
+    \return 0 no element defined
+    \return 1 on success
+ */
+int G_make_mapset_object_group_tmp(const char *type)
+{
+    char path[GPATH_MAX];
+
+    G_file_name_tmp(path, NULL, NULL, G_mapset());
+    return make_mapset_element_no_fail_on_race(path, type);
+}
+
+int make_mapset_element_impl(const char *p_path, const char *p_element, bool race_ok)
 {
     char path[GPATH_MAX], *p;
     const char *element;
@@ -85,36 +182,56 @@ int make_mapset_element(const char *p_path, const char *p_element)
     while (1) {
 	if (*element == '/' || *element == 0) {
 	    *p = 0;
-	    if (access(path, 0) != 0) { /* directory not yet created */
-		if (G_mkdir(path) != 0)
-		    G_fatal_error(_("Unable to make mapset element %s (%s): %s"),
-				  p_element, path, strerror(errno));
-	    }
-	    if (access(path, 0) != 0)  /* directory not accessible */
-		G_fatal_error(_("Unable to access mapset element %s (%s): %s"),
-			      p_element, path, strerror(errno));
+            char *msg = NULL;
+            if (access(path, 0) != 0) {
+                /* Assuming that directory does not exist. */
+                if (G_mkdir(path) != 0) {
+                    msg = G_store(strerror(errno));
+                }
+            }
+            if (access(path, 0) != 0 || (msg && !race_ok)) {
+                /* Directory is not accessible even after attempt to create it. */
+                if (msg) {
+                    /* Error already happened when mkdir. */
+                    G_fatal_error(_("Unable to make mapset element %s (%s): %s"),
+                                  p_element, path, strerror(errno));
+                }
+                else {
+                    /* Access error is not related to mkdir. */
+                    G_fatal_error(_("Unable to access mapset element %s (%s): %s"),
+                                  p_element, path, strerror(errno));
+                }
+            }
 	    if (*element == 0)
 		return 1;
 	}
 	*p++ = *element++;
     }
 }
 
+int make_mapset_element(const char *p_path, const char *p_element)
+{
+    return make_mapset_element_impl(p_path, p_element, false);
+}
+
+int make_mapset_element_no_fail_on_race(const char *p_path, const char *p_element)
+{
+    return make_mapset_element_impl(p_path, p_element, true);
+}
+
+
 /*!
    \brief Create misc element in the current mapset.
 
-   \param dir directory path
-   \param name element to be created in mapset
+   \param dir directory path (e.g., `cell_misc`)
+   \param name element to be created in mapset (e.g., `elevation`)
 
    \return 0 no element defined
    \return 1 on success
  */
 int G__make_mapset_element_misc(const char *dir, const char *name)
 {
-    char buf[GNAME_MAX * 2 + 1];
-
-    sprintf(buf, "%s/%s", dir, name);
-    return G_make_mapset_element(buf);
+    G_make_mapset_dir_object(dir, name);
 }
 
 static int check_owner(const struct stat *info)

lib/gis/open.c

@@ -112,9 +112,9 @@ static int G__open(const char *element,
 
 	if (mode == 1 || access(path, 0) != 0) {
             if (is_tmp)
-                G_make_mapset_element_tmp(element);
+                G_make_mapset_object_group_tmp(element);
             else
-                G_make_mapset_element(element);
+                G_make_mapset_object_group(element);
 	    close(open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666));
 	}
 

lib/gis/tempfile.c

@@ -122,9 +122,9 @@ void G__temp_element(char *element, int tmp)
     }
 
     if (!tmp)
-        G_make_mapset_element(element);
+        G_make_mapset_object_group(element);
     else
-        G_make_mapset_element_tmp(element);
+        G_make_mapset_object_group_tmp(element);
 
     G_debug(2, "G__temp_element(): %s (tmp=%d)", element, tmp);
 }

lib/manage/do_copy.c

@@ -54,7 +54,7 @@ int M_do_copy(int n, const char *old, const char *mapset, const char *new)
     }
     else {
 	for (i = 0; i < list[n].nelem; i++) {
-	    G_make_mapset_element(list[n].element[i]);
+	    G_make_mapset_object_group(list[n].element[i]);
 	    G_file_name(path, list[n].element[i], old, mapset);
 	    if (access(path, 0) != 0) {
 		G_remove(list[n].element[i], new);

lib/raster/close.c

@@ -317,7 +317,7 @@ static int close_new_gdal(int fd, int ok)
 	remove(path);
 
 	/* write 0-length cell file */
-	G_make_mapset_element("cell");
+	G_make_mapset_object_group("cell");
 	G_file_name(path, "cell", fcb->name, fcb->mapset);
 	cell_fd = creat(path, 0666);
 	close(cell_fd);
@@ -326,7 +326,7 @@ static int close_new_gdal(int fd, int ok)
 	    write_fp_format(fd);
 
 	    /* write 0-length fcell file */
-	    G_make_mapset_element("fcell");
+	    G_make_mapset_object_group("fcell");
 	    G_file_name(path, "fcell", fcb->name, fcb->mapset);
 	    cell_fd = creat(path, 0666);
 	    close(cell_fd);
@@ -446,7 +446,7 @@ static int close_new(int fd, int ok)
 	    write_fp_format(fd);
 
 	    /* now write 0-length cell file */
-	    G_make_mapset_element("cell");
+	    G_make_mapset_object_group("cell");
 	    cell_fd =
 		creat(G_file_name(path, "cell", fcb->name, fcb->mapset),
 		      0666);

lib/raster/gdal.c

@@ -395,7 +395,7 @@ static void read_gdal_options(void)
 	G_file_name(path, p, "", G_mapset());
 	st->opts.dir = G_store(path);
 	if (access(path, 0) != 0)
-	    G_make_mapset_element(p);
+	    G_make_mapset_object_group(p);
     }
 
     p = G_find_key_value("extension", key_val);

lib/raster/open.c

@@ -642,7 +642,7 @@ static int open_raster_new(const char *name, int open_mode,
      * since we are bypassing the normal open logic
      * must create the cell element 
      */
-    G_make_mapset_element(cell_dir);
+    G_make_mapset_object_group(cell_dir);
 
     /* mark closed */
     fcb->map_type = map_type;

lib/raster/quant_io.c

@@ -287,7 +287,7 @@ int Rast__quant_export(const char *name, const char *mapset,
     else {
 	sprintf(element, "quant2/%s", mapset);
 	G_remove(element, name);
-	G_make_mapset_element(element);
+	G_make_mapset_object_group(element);
 	if (!(fd = G_fopen_new(element, name)))
 	    return -1;
     }

lib/raster3d/mapset.c

@@ -8,8 +8,5 @@
 
 void Rast3d_make_mapset_map_directory(const char *mapName)
 {
-    char buf[GNAME_MAX + sizeof(RASTER3D_DIRECTORY) + 2];
-
-    sprintf(buf, "%s/%s", RASTER3D_DIRECTORY, mapName);
-    G_make_mapset_element(buf);
+    G_make_mapset_dir_object(RASTER3D_DIRECTORY, mapName);
 }

lib/vector/Vlib/map.c

@@ -163,9 +163,7 @@ int Vect_copy(const char *in, const char *mapset, const char *out)
     }
 
     /* Copy the directory */
-    G_make_mapset_element(GV_DIRECTORY);
-    sprintf(buf, "%s/%s", GV_DIRECTORY, out);
-    G_make_mapset_element(buf);
+    G_make_mapset_dir_object(GV_DIRECTORY, out);
 
     i = 0;
     while (files[i]) {