[v2,3/6] Move mkdir_recursive to common/filestuff.c

Message ID 20181018223100.20693-4-tom@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey Oct. 18, 2018, 10:30 p.m. UTC
  This moves mkdir_recursive from dwarf-index-cache.c to
common/filestuff.c, and also changes it to return a boolean that says
whether or not it worked.

gdb/ChangeLog
2018-10-18  Tom Tromey  <tom@tromey.com>

	* unittests/mkdir-recursive-selftests.c: New file.
	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
	unittests/mkdir-recursive-selftests.c.
	* dwarf-index-cache.c (mkdir_recursive): Move to
	common/filestuff.c.
	(index_cache::store): Check return value of mkdir_recursive.
	(create_dir_and_check, test_mkdir_recursive): Move to new file.
	(_initialize_index_cache): Don't register test.
	* common/filestuff.h (mkdir_recursive): Declare.
	* common/filestuff.c (mkdir_recursive): Move from
	dwarf-index-cache.c.  Return bool.
---
 gdb/ChangeLog                             |  14 +++
 gdb/Makefile.in                           |   1 +
 gdb/common/filestuff.c                    |  45 +++++++++
 gdb/common/filestuff.h                    |  10 ++
 gdb/dwarf-index-cache.c                   | 114 ++--------------------
 gdb/unittests/mkdir-recursive-selftests.c |  89 +++++++++++++++++
 6 files changed, 165 insertions(+), 108 deletions(-)
 create mode 100644 gdb/unittests/mkdir-recursive-selftests.c
  

Comments

Simon Marchi Oct. 29, 2018, 4 p.m. UTC | #1
On 2018-10-18 6:30 p.m., Tom Tromey wrote:
> This moves mkdir_recursive from dwarf-index-cache.c to

> common/filestuff.c, and also changes it to return a boolean that says

> whether or not it worked.

> 

> gdb/ChangeLog

> 2018-10-18  Tom Tromey  <tom@tromey.com>

> 

> 	* unittests/mkdir-recursive-selftests.c: New file.

> 	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add

> 	unittests/mkdir-recursive-selftests.c.

> 	* dwarf-index-cache.c (mkdir_recursive): Move to

> 	common/filestuff.c.

> 	(index_cache::store): Check return value of mkdir_recursive.

> 	(create_dir_and_check, test_mkdir_recursive): Move to new file.

> 	(_initialize_index_cache): Don't register test.

> 	* common/filestuff.h (mkdir_recursive): Declare.

> 	* common/filestuff.c (mkdir_recursive): Move from

> 	dwarf-index-cache.c.  Return bool.


I just stumbled on a build failure caused by this patch, when building for mingw:

  CXX    unittests/mkdir-recursive-selftests.o
/home/emaisin/src/binutils-gdb/gdb/unittests/mkdir-recursive-selftests.c: In function ‘void selftests::mkdir_recursive::test()’:
/home/emaisin/src/binutils-gdb/gdb/unittests/mkdir-recursive-selftests.c:49:20: error: ‘mkdtemp’ was not declared in this scope
   if (mkdtemp (base) == NULL)
                    ^

The function in the original code was guarded by HAVE_MKDTEMP (the register_test
call still is).  So I guess the function should be put back in an ifdef, or we
should import the mkdtemp gnulib module (I don't remember why I chose not to do
that).

Simon
  

Patch

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 0bb203f45f..4ab0da5c61 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -419,6 +419,7 @@  SUBDIR_UNITTESTS_SRCS = \
 	unittests/optional-selftests.c \
 	unittests/parse-connection-spec-selftests.c \
 	unittests/ptid-selftests.c \
+	unittests/mkdir-recursive-selftests.c \
 	unittests/rsp-low-selftests.c \
 	unittests/scoped_fd-selftests.c \
 	unittests/scoped_mmap-selftests.c \
diff --git a/gdb/common/filestuff.c b/gdb/common/filestuff.c
index dfd86f9fbb..d4bd1a8cb8 100644
--- a/gdb/common/filestuff.c
+++ b/gdb/common/filestuff.c
@@ -447,3 +447,48 @@  is_regular_file (const char *name, int *errno_ptr)
     *errno_ptr = EINVAL;
   return false;
 }
+
+/* See common/filestuff.h.  */
+
+bool
+mkdir_recursive (const char *dir)
+{
+  gdb::unique_xmalloc_ptr<char> holder (xstrdup (dir));
+  char * const start = holder.get ();
+  char *component_start = start;
+  char *component_end = start;
+
+  while (1)
+    {
+      /* Find the beginning of the next component.  */
+      while (*component_start == '/')
+	component_start++;
+
+      /* Are we done?  */
+      if (*component_start == '\0')
+	return true;
+
+      /* Find the slash or null-terminator after this component.  */
+      component_end = component_start;
+      while (*component_end != '/' && *component_end != '\0')
+	component_end++;
+
+      /* Temporarily replace the slash with a null terminator, so we can create
+         the directory up to this component.  */
+      char saved_char = *component_end;
+      *component_end = '\0';
+
+      /* If we get EEXIST and the existing path is a directory, then we're
+         happy.  If it exists, but it's a regular file and this is not the last
+         component, we'll fail at the next component.  If this is the last
+         component, the caller will fail with ENOTDIR when trying to
+         open/create a file under that path.  */
+      if (mkdir (start, 0700) != 0)
+	if (errno != EEXIST)
+	  return false;
+
+      /* Restore the overwritten char.  */
+      *component_end = saved_char;
+      component_start = component_end;
+    }
+}
diff --git a/gdb/common/filestuff.h b/gdb/common/filestuff.h
index e9328f5358..ecfc18d600 100644
--- a/gdb/common/filestuff.h
+++ b/gdb/common/filestuff.h
@@ -122,4 +122,14 @@  typedef std::unique_ptr<DIR, gdb_dir_deleter> gdb_dir_up;
    we're expecting a regular file.  */
 extern bool is_regular_file (const char *name, int *errno_ptr);
 
+
+/* A cheap (as in low-quality) recursive mkdir.  Try to create all the
+   parents directories up to DIR and DIR itself.  Stop if we hit an
+   error along the way.  There is no attempt to remove created
+   directories in case of failure.
+
+   Returns false on failure and sets errno.  */
+
+extern bool mkdir_recursive (const char *dir);
+
 #endif /* FILESTUFF_H */
diff --git a/gdb/dwarf-index-cache.c b/gdb/dwarf-index-cache.c
index 966630b60c..bac98f9db8 100644
--- a/gdb/dwarf-index-cache.c
+++ b/gdb/dwarf-index-cache.c
@@ -45,53 +45,6 @@  index_cache global_index_cache;
 static cmd_list_element *set_index_cache_prefix_list;
 static cmd_list_element *show_index_cache_prefix_list;
 
-/* A cheap (as in low-quality) recursive mkdir.  Try to create all the parents
-   directories up to DIR and DIR itself.  Stop if we hit an error along the way.
-   There is no attempt to remove created directories in case of failure.  */
-
-static void
-mkdir_recursive (const char *dir)
-{
-  gdb::unique_xmalloc_ptr<char> holder (xstrdup (dir));
-  char * const start = holder.get ();
-  char *component_start = start;
-  char *component_end = start;
-
-  while (1)
-    {
-      /* Find the beginning of the next component.  */
-      while (*component_start == '/')
-	component_start++;
-
-      /* Are we done?  */
-      if (*component_start == '\0')
-	return;
-
-      /* Find the slash or null-terminator after this component.  */
-      component_end = component_start;
-      while (*component_end != '/' && *component_end != '\0')
-	component_end++;
-
-      /* Temporarily replace the slash with a null terminator, so we can create
-         the directory up to this component.  */
-      char saved_char = *component_end;
-      *component_end = '\0';
-
-      /* If we get EEXIST and the existing path is a directory, then we're
-         happy.  If it exists, but it's a regular file and this is not the last
-         component, we'll fail at the next component.  If this is the last
-         component, the caller will fail with ENOTDIR when trying to
-         open/create a file under that path.  */
-      if (mkdir (start, 0700) != 0)
-	if (errno != EEXIST)
-	  return;
-
-      /* Restore the overwritten char.  */
-      *component_end = saved_char;
-      component_start = component_end;
-    }
-}
-
 /* Default destructor of index_cache_resource.  */
 index_cache_resource::~index_cache_resource () = default;
 
@@ -160,7 +113,12 @@  index_cache::store (struct dwarf2_per_objfile *dwarf2_per_objfile)
   TRY
     {
       /* Try to create the containing directory.  */
-      mkdir_recursive (m_dir.c_str ());
+      if (!mkdir_recursive (m_dir.c_str ()))
+	{
+	  warning (_("index cache: could not make cache directory: %s\n"),
+		   safe_strerror (errno));
+	  return;
+	}
 
       if (debug_index_cache)
         printf_unfiltered ("index cache: writing index cache for objfile %s\n",
@@ -346,62 +304,6 @@  show_index_cache_stats_command (const char *arg, int from_tty)
 		     indent, global_index_cache.n_misses ());
 }
 
-#if GDB_SELF_TEST && defined (HAVE_MKDTEMP)
-namespace selftests
-{
-
-/* Try to create DIR using mkdir_recursive and make sure it exists.  */
-
-static bool
-create_dir_and_check (const char *dir)
-{
-  mkdir_recursive (dir);
-
-  struct stat st;
-  if (stat (dir, &st) != 0)
-    perror_with_name (("stat"));
-
-  return (st.st_mode & S_IFDIR) != 0;
-}
-
-/* Test mkdir_recursive.  */
-
-static void
-test_mkdir_recursive ()
-{
-  char base[] = "/tmp/gdb-selftests-XXXXXX";
-
-  if (mkdtemp (base) == NULL)
-    perror_with_name (("mkdtemp"));
-
-  /* Try not to leave leftover directories.  */
-  struct cleanup_dirs {
-    cleanup_dirs (const char *base)
-      : m_base (base)
-    {}
-
-    ~cleanup_dirs () {
-      rmdir (string_printf ("%s/a/b/c/d/e", m_base).c_str ());
-      rmdir (string_printf ("%s/a/b/c/d", m_base).c_str ());
-      rmdir (string_printf ("%s/a/b/c", m_base).c_str ());
-      rmdir (string_printf ("%s/a/b", m_base).c_str ());
-      rmdir (string_printf ("%s/a", m_base).c_str ());
-      rmdir (m_base);
-    }
-
-  private:
-    const char *m_base;
-  } cleanup_dirs (base);
-
-  std::string dir = string_printf ("%s/a/b", base);
-  SELF_CHECK (create_dir_and_check (dir.c_str ()));
-
-  dir = string_printf ("%s/a/b/c//d/e/", base);
-  SELF_CHECK (create_dir_and_check (dir.c_str ()));
-}
-}
-#endif /*  GDB_SELF_TEST && defined (HAVE_MKDTEMP) */
-
 void
 _initialize_index_cache ()
 {
@@ -456,8 +358,4 @@  _initialize_index_cache ()
 When non-zero, debugging output for the index cache is displayed."),
 			    NULL, NULL,
 			    &setdebuglist, &showdebuglist);
-
-#if GDB_SELF_TEST && defined (HAVE_MKDTEMP)
-  selftests::register_test ("mkdir_recursive", selftests::test_mkdir_recursive);
-#endif
 }
diff --git a/gdb/unittests/mkdir-recursive-selftests.c b/gdb/unittests/mkdir-recursive-selftests.c
new file mode 100644
index 0000000000..d501f8e081
--- /dev/null
+++ b/gdb/unittests/mkdir-recursive-selftests.c
@@ -0,0 +1,89 @@ 
+/* Self tests for scoped_fd for GDB, the GNU debugger.
+
+   Copyright (C) 2018 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+
+#include "common/filestuff.h"
+#include "selftest.h"
+
+namespace selftests {
+namespace mkdir_recursive {
+
+/* Try to create DIR using mkdir_recursive and make sure it exists.  */
+
+static bool
+create_dir_and_check (const char *dir)
+{
+  ::mkdir_recursive (dir);
+
+  struct stat st;
+  if (stat (dir, &st) != 0)
+    perror_with_name (("stat"));
+
+  return (st.st_mode & S_IFDIR) != 0;
+}
+
+/* Test mkdir_recursive.  */
+
+static void
+test ()
+{
+  char base[] = "/tmp/gdb-selftests-XXXXXX";
+
+  if (mkdtemp (base) == NULL)
+    perror_with_name (("mkdtemp"));
+
+  /* Try not to leave leftover directories.  */
+  struct cleanup_dirs {
+    cleanup_dirs (const char *base)
+      : m_base (base)
+    {}
+
+    ~cleanup_dirs () {
+      rmdir (string_printf ("%s/a/b/c/d/e", m_base).c_str ());
+      rmdir (string_printf ("%s/a/b/c/d", m_base).c_str ());
+      rmdir (string_printf ("%s/a/b/c", m_base).c_str ());
+      rmdir (string_printf ("%s/a/b", m_base).c_str ());
+      rmdir (string_printf ("%s/a", m_base).c_str ());
+      rmdir (m_base);
+    }
+
+  private:
+    const char *m_base;
+  } cleanup_dirs (base);
+
+  std::string dir = string_printf ("%s/a/b", base);
+  SELF_CHECK (create_dir_and_check (dir.c_str ()));
+
+  dir = string_printf ("%s/a/b/c//d/e/", base);
+  SELF_CHECK (create_dir_and_check (dir.c_str ()));
+}
+
+}
+}
+
+void
+_initialize_mkdir_recursive_selftests ()
+{
+#if defined (HAVE_MKDTEMP)
+  selftests::register_test ("mkdir_recursive",
+			    selftests::mkdir_recursive::test);
+#endif
+}
+